Refactor/upgrade iced version #1865
Conversation
OBorce
requested review from
erubboli,
azarovh and
ImplOfAnImpl
as code owners
There was a problem hiding this comment.
-
Wallet recovery seems broken in this PR - the recovery dialog stays open after the file pick dialog has been closed and nothing happens.
When creating a new wallet (instead of recover wallet) it generates a new seed phrase for you, if you start editing the seed phrase it will automatically switch from Create to Recover mode. Previously it remained Create. Or maybe we should disable editing of the seed phrase in the Create mode?
IMO this will be very confusing, especially for unexperienced users, and also error-prone.
I'd say let's disable editing in the Create mode.
| row![ | ||
| pick_list.width(100), | ||
| button(Text::new("+")) | ||
| .style(iced::theme::Button::Positive) | ||
| .style(iced::widget::button::success) | ||
| .on_press(WalletMessage::NewAccount), | ||
| tooltip( | ||
| Text::new(iced_aw::Bootstrap::Question.to_string()) | ||
| .font(iced_aw::BOOTSTRAP_FONT), | ||
| Text::new(iced_fonts::Bootstrap::Question.to_string()) | ||
| .font(iced_fonts::BOOTSTRAP_FONT), | ||
| NEW_ACCOUNT_TOOLTIP_TEXT, | ||
| tooltip::Position::Bottom | ||
| ) | ||
| .gap(10) | ||
| .style(iced::theme::Container::Box) | ||
| ] | ||
| .align_items(Alignment::Center) | ||
| .align_y(Alignment::Center) |
There was a problem hiding this comment.
Btw, the triangle symbol of the pick list now overlaps with the text:
Increasing the width of the pick list seems to help (or may be it's better to decrease the width of the button, which is now noticeably wider than before)
There was a problem hiding this comment.
the pick list had a fixed width so the overlap will happen based on the name of the account if someone inputs a very long name or not. I have removed the fixed width, and the widget will calculate it based on the longest name. But now if there is a very long name the button is squashed.
OBorce
force-pushed
the
refactor/upgrade-iced-version
branch
from
72d7da3 to
7e85231
Compare
There was a problem hiding this comment.
-
Wallet recovery seems broken in this PR - the recovery dialog stays open after the file pick dialog has been closed and nothing happens.
Discussed - this is a regression on master that has been fixed in the trezor branch. However I'm wondering when the trezor branch is supposed to be merged. Probably not soon enough, in which case perhaps it makes sense to backport those changes on the current master?
-
As the Model widget was removed from iced_aw no other widget supports the on_esc method so I haven't found a way to dismiss dialogs with esc for now, other than writhing a new low-level widget from scratch.
It's a minor issue I guess, but it's a regression, so IMO it's not very good to leave it as is.
Also, there are other issues with dialogs currently, so a new widget may be the way to go. -
Some dialog issues:
- When creating or recovering a wallet it's possible to select "Create"/"Recover" again while "file open" dialog is open.
- Other dialogs are no longer modal either. E.g. it's possible to select "Encrypt wallet" again while the wallet encryption dialog is already shown.
| iced::widget::column![text_input("Mnemonic", &mnemonic) | ||
| // only enable edit if there is not pre-generated mnemonic | ||
| .on_input_maybe(generated_mnemonic_opt.is_none().then_some(on_mnemonic_change)) | ||
| .padding(15)], | ||
| ) |
There was a problem hiding this comment.
The generated mnemonic is pretty hard to see.
Also, copying it is not easy either - there is no cursor, so it's unclear whether you can select it via keyboard or not
- make the gui wallet not block on scanning the blokchain when recovering a wallet from a seed phrase
OBorce
force-pushed
the
refactor/upgrade-iced-version
branch
from
3c6c9da to
eff8579
Compare
There was a problem hiding this comment.
-
I've proposed some changes in A few fixes in node gui related to the
icedupgrade #1872
I suggest merging that PR into this one, so that we can review the whole thing one more time. -
[discussed] IMO it's better to continue using
Componenteven though it's deprecated, because:
a) having the dialogs' state inside the app state looks pretty bad.
b) there is a chance thatComponentwill be un-deprecated in the future.
This reverts commit 7e85231.
…ment in the stack to block mouse events for elements under a dialog; Implement esc_handler to handle Esc presses in dialogs; Minor visual improvements;
A few fixes in node gui related to the `iced` upgrade
There are 2 changes from the previous behavior:
on_escmethod so I haven't found a way to dismiss dialogs with esc for now, other than writhing a new low-level widget from scratch.