-
Notifications
You must be signed in to change notification settings - Fork 174
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Display toast when seed already exists #586
base: dev
Are you sure you want to change the base?
Conversation
Fixes / required changes: * Issue SeedSigner#585
In the screenshots where toast is working fine, they're 4-tuples and followed immediately by strings... so the toast_thread variable gets reset to None in the else leg. But in your example, yours is followed by a 2tuple so it never drops into the else clause to reset toast_thread. Maybe for 2tuples and 3tuples, just default toast_thread=None, or just once at the top of the loop iteration??? |
Reviewed, tests all pass, works for me on dev-seedsigner, and screenshots generate -- as of 9ec93a1 |
True! Thank you @jdlcdl! I'll update with this change since it seems that there are no unwanted effects. This may be actually useful for more similar uses of the toast overlay in the future |
After introducing this toast overlay, the screenshot generator script would throw an error because of the toast_thread variable was not reseted. Now an initialization toast_thread = None is done at the top of each loop iteration.
Brilliant work, @alvroble! This is a great QoL improvement. Thanks @jdlcdl for fixing the screenshot generator! Hopefully this fixes my toast modification issues as well. @alvroble Are you open to a few minor visual tweaks on the component?
|
Thanks for you comments @easyuxd @jdlcdl! @easyuxd I'm going to try and look into it, although this settings seems to be configured in the base class ToastOverlay, so maybe this implies modifications in the base class or a deeper code refactor; but just saying at first glance. I will check it and try to adapt. |
Just made some tests about the sizes since it really seemed weird the difference between top/left padding. Found out this behavior that changes when there are characters that dip below baseline. For a quick comparision, note the subtle difference: This is due to a piece of code in the TextArea class that adjusts the text height, ensuring no characters are cut off by the UI borders: seedsigner/src/seedsigner/gui/components.py Lines 347 to 349 in 100a974
This seems to imply that a significant refactor is necessary. The same applies more or less to the color proposal, though it may be achievable. Currently, the icon, outline, and text color are bound together in the ToastOverlay. To achieve what you proposed, I could make a small refactor to the toast file (https://github.com/SeedSigner/seedsigner/blob/dev/src/seedsigner/gui/toast.py) and unbind the text color from the icon and outline color. However, I'd like to get feedback from the other devs on this point. |
NACK, unless I'm misunderstanding something. Scenarios:
This proposed toast UX clears up confusion in scenario 1 but completely prevents the user from accomplishing scenario 2. @easyuxd's earlier interstitial UX concept would notify the user if it's scenario 1 but could still provide a path for scenario 2. But the real fix to me seems more like:
|
Note: Separate from this specific functionality, if the toast component could be improved with some refactoring, by all means that's a worthwhile improvement on its own. *caveat: I have not looked into the tweaks discussed here, though I do remember agonizing over how to space out two-line toast messages when we were first implementing the feature. Decisions about pixels below the baseline are always hard to balance. |
As I understand it, it already covers the scenarios you mention. In fact, this branch feels exactly as it would feel w/o the branch EXCEPT for the added info that the seed was already loaded once you land into the seed options menu. For comparisons, it's not using the mnemonic alone, it's actually checking if the pending_seed is already in seeds (so it's checking mnemonic, passphrase, even the 64byte seed that gets fed into bip32... and the INFO message is only being shown if that seed mnemonic/passphrase/root-bip32-wallet had already been loaded). If the nack is about the added INFO, because silently ignoring the duplicate action is preferred, then I can understand that. I've done it myself and I never found it alarming, but I can see that others might load a seed, then load another -- except it's the exact same thing, and wondering why they cannot find their 2 seeds. For those folks... LEARN YOUR FINGERPRINTS, you'll thank me later. |
Okay, then let me back up: @easyuxd why do you say "This can appear jarring and introduces potential confusion" in #585? When I read that issue I assumed there must have been some short-cut UI jump that skipped past some part of the Load Seed flow if the new seed was already loaded and it was that short-cut that was jarring (I couldn't remember if there was anything like that implemented but it seemed possible). But now that I've taken a look at the changes here (and thanks to @jdlcdl's explanation), I just don't see why there's any "potential confusion" at all in the first place. Or does this have to do with resuming an expected flow (e.g. scan a psbt, then opt to load a seed (even though it's already onboard), fails to properly resume psbt flow...?)? |
Thank you for your comments @kdmukai and @jdlcdl ! @kdmukai I believe I don't get what you meant by saying "completely prevents the user from accomplishing scenario 2". As @jdlcdl says, this toast activates in the SeedOptionsView for both scenarios, as the pending_seed variable is used for comparison. The most important use case for this feature, in my opinion, is when a user loads several wallets that share the same seed but use different BIP39 passphrases. The user will be notified if they accidentally load an existing seed + BIP39 passphrase that has already been loaded. So, I believe the approach with this informative toast is simply to make the user's life easier (not at all needed, bit a tiny QoL improvement) while maintaining the current internal management of the seed as it is. The only difference is that, after loading a seed or seed + BIP39 passphrase, in the SeedOptionsView the user will see this toast if the actual seed or seed + BIP39 passphrase was already loaded. @kdmukai I understand what you say about not being a potential confussion right now. The current handling of this case is clear, and the user is taken directly to the SeedOptionsView, but in my opinion this toast might help a distracted user realize that they’re loading an existing seed without disrupting the flow. |
I'd like to first say great work @alvroble on this PR. I appreciate your contribution here whatever the outcome of this PR ends up being. I also love the continued contributions by @easyuxd as usual! I'm going to use the term "seed" below to represent full mnemonic + passphrase combination. I find very little confusion when adding the same seed twice and having the workflow go straight to the seed options menu on second entry. This is especially true when I'm in the habit of comparing/knowing fingerprints when selecting the seed from the list of seeds. I believe the main workflow this PR improves is the one where you enter more than one seed into SeedSigner and are mentally tracking the order seeds entered instead of by fingerprint. If you were to accidentally add the same seed twice it would throw you off and cause confusion about which seed you want to select. For example, if you think you scanned in 4 seeds but now only have 3 unique seeds listed, you'd be confused about which seed you missed (unless you had fingerprints for each seed). All this said I do find the UI of this PR to be slick and not to cause any confusion or additional clicks. I think the improvement is tiny, but still an improvement. I support the direction of this PR and think this kind of UI paradigm would beneficial in many other places in SeedSigner workflows. It provides a slight bit more context in a workflow without interruption. I believe the main hurdle to have this PR merged will be implementation details. Figuring out where and how much to refactor around toast messages. |
Fixes / required changes:
Description
Changes were introduced to display a toast overlay when the user loads an already existing seed. The toast is displayed in the Seed Options view for 5 seconds or until dismissed by user action:
Some timing change had to be made in the toast BaseToastOverlayManagerThread.run method to avoid the previous user action with the buttons to cancel the toast display. An additional 0.1 seconds timer works nice, although I'm not really sure this is the best way to avoid this.
Changes should be made to the screenshot generator fails when adding this view with toast. I haven't managed to get this to work. The screenshot was indeed generated but inmediately after, it crashes. Would love some feedback here.
To do:
This pull request is categorized as a:
Checklist
pytest
and made sure all unit tests pass before sumbitting the PRIf you modified or added functionality/workflow, did you add new unit tests?
I have tested this PR on the following platforms/os:
Note: Keep your changes limited in scope; if you uncover other issues or improvements along the way, ideally submit those as a separate PR. The more complicated the PR the harder to review, test, and merge.