[Enhancement] Add exit dialog when entering passphrase #563

alvroble · 2024-06-30T12:05:52Z

Enhancement made:

Issue Behavior of back button confusing when entering password #510
Updated test suite accordingly

Description

When entering a long passphrase, one can push accidentally the back button, so an exit dialog is added to prevent the accidentally loss of all the work to type in a long passphrase.

A new Warning Screen view is added:

The actual dialog may be a bit too sober and could be modified to a more friendly "Are you sure you want to cancel?"

This pull request is categorized as a:

Checklist

I’ve run pytest and made sure all unit tests pass before sumbitting the PR

If you modified or added functionality/workflow, did you add new unit tests?

No, I’m a fool
Yes
N/A

I have tested this PR on the following platforms/os:

Raspberry Pi OS Manual Build
SeedSigner OS on a Pi0/Pi0W board
Other

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.

Fixes / required changes: * Issue SeedSigner#510 'Behavior of back button confusing when entering password' * Updated test suite accordingly

alvroble · 2024-07-05T15:51:41Z

Checks passed 😀✅

jdlcdl · 2024-07-06T10:30:55Z

Since a release is around the corner (currently targeting Monday July 8th for finalization of all prs), please consider joining the SeedSigner Community group on telegram and/or the SeedSigner New Devs group to argue for this pr.

And thank you for contributing.

jdlcdl · 2024-07-06T11:28:58Z

src/seedsigner/views/seed_views.py

+
+        selected_menu_num = self.run_screen(
+            WarningScreen,
+            title="Exit",


suggestion: "Abandon Passphrase?" but only if it would fit.

else perhaps: title "Abandon Edting" with text = "Exiting will abandon the BIP-39 Passphrase"

It's just a suggestion.

Thanks @jdlcdl.

I agree with this suggestion for both title and text, and leaving button texts as proposed.

Let's see if other contributors comment on this, if not then I'll push the changes you proposed (after checking that new title fits).

I tested your suggestion. It fits and looks quite better.

I also had the idea of switching the button order. This way, if you accidentally press the button, you will continue editing instead of exiting, which aligns with the spirit of this issue.

Since still waiting for feedback from others, please feel free to adjust to the best pr that you can (text changes and button preferences as you like) w/ another commit and push.

I like the text just above, and the thought of a deliberate exit by "down" and "press" to avoid accidentally abandoning the long passphrase.

In the psychology of the user what happens we the don't know what is BIP-39? Do we really need to add BIP-39.
Are we building for ourself or for the next billion users? my two sats.

Thank you for your comment, @dulcedu. I believe the use of BIP numbers throughout the UX is something that should be considered globally. In this particular case, I think it's good practice to keep "BIP-39 Passphrase" to maintain consistency with the overall UX design.

I think this point could unleash a broader debate to balance between design accessibility to a wide audience and adhering to best practices.

I appreciate the thoughts on BIP-39, and its 100% fair perspective on it being a technical detail. There are unfortunately several kinds of passwords/phrases that can be implemented with wallets / private keys, so I think it makes sense to include it for clarity somewhere in the UI.

jdlcdl · 2024-07-06T11:44:13Z

As of 59bcfda:

ACK for the concept because what's the point of a bip39 passphrase if its not a strong one? and this is useful for that
ACK on the code review
ACK tested on rpi0 dev device
ACK on one more new contributor to the next release...

But I still think improvements might be made on the title/text,
AND I'd like to have input from other devs.

Thank you Alvroble!

alvroble · 2024-07-06T12:04:04Z

Thank you for the comments, @jdlcdl! I thought this would be a good first issue to learn about SeedSigner dev environment.

I'd love to join the Telegram groups and get more involved around here. How can I join?

jdlcdl · 2024-07-06T12:15:52Z

Duh, I didn't even include the links. :p

https://t.me/seedsigner_new_devs for new devs, most appropriate for dev discussions
https://t.me/joinchat/GHNuc_nhNQjLPWsS for the main SeedSigner Community discussions.

alvroble · 2024-07-07T10:10:52Z

The new proposed screen is the following:

The texts were improved as @jdlcdl suggested
The button position was swapped to avoid (again) accidentally exiting

jdlcdl · 2024-07-07T11:18:13Z

w/o altering my comments above,

I repeat my ACK tested at 1670bf6

easyuxd · 2024-07-07T15:35:28Z

Love this, @alvroble. Solid enhancement that will help reduce mistakes.

Looking at the other messaging screens, I’d recommend content like this to align with the rest of the UX, specifically the “Discard Seed” dialog:

We use "discard" elsewhere, and "keep/discard passphrase" clarifies the specific actions and aligns with the rest of the content.

Thanks for your contribution!

alvroble · 2024-07-07T18:01:33Z

Thanks @easyuxd for the comment.

I agree to mantain the UX alignment. However, maybe a better text fo the "Continue editing" button would be "Edit passphrase" instead of "Keep passhprase", because when you push that button you are not specifically keeping the passphrase you were writing, you are just going back to continue editing it. What do you think?

easyuxd · 2024-07-07T18:25:40Z

@alvroble
Agreed, "Edit" makes more sense here. Good catch!

alvroble · 2024-07-07T18:33:18Z

@easyuxd
Updated in the new commit. Thanks for the feedback!

kdmukai · 2024-07-08T20:08:38Z

Looking pretty good!

Minor note on the warning text: "Discard passphrase and go back" has two small issues:

The top title is already "Discard passphrase?" so it's repetitive to reiterate the same phrase in the body.
"and go back": I'm sure most people will understand based on the context, but it would be reasonable for a user to think, "I want to go BACK to entering my long passphrase".

So... I'm not sure what text should go in the body.

src/seedsigner/views/seed_views.py

alvroble · 2024-07-08T20:31:38Z

Thanks @kdmukai! I understand, it's kind of tricky because of the context and limited space.

Maybe something like: "Your current passphrase entry will be erased. Proceed?"

kdmukai · 2024-07-08T21:18:56Z

src/seedsigner/gui/screens/seed_screens.py


            elif input == HardwareButtonsConstants.KEY_PRESS and self.top_nav.is_selected:
                # Back button clicked
-                return self.top_nav.selected_button
+                return self.passphrase, self.top_nav.selected_button


I'm pretty uncomfortable with this change to return a tuple and especially the downstream changes it creates in the related flow-based tests.

We have type hints for what we expect View.run_screen to return:
https://github.com/SeedSigner/seedsigner/blob/dev/src/seedsigner/views/view.py#L104

Since this is python, obviously we're not restricted to just those two types.

It's not terrible to break the existing convention in this way, but I still strongly dislike it. Though admittedly it's probably more on aesthetic / esoteric grounds; I doubt this would cause problems for us in the future. That being said, the flow-based tests are only possible because we held extreme discipline throughout the code in plenty of other areas.

(and we have a matching type hint for FlowStep that's no longer completely accurate: https://github.com/SeedSigner/seedsigner/blob/dev/tests/base.py#L109)

But I can't think of an alternate solution, either. Well, not a good one, at least.

Very minor improvement: return a dict instead of a tuple?

# in the Screen: # Completed passphrase entry... return dict(passphrase=self.passphrase) # BACK button selected... return dict(passphrase=self.passphrase, is_back_button=True)

And then the View would ask:

if "is_back_button" in ret_dict: ...

There's barely a difference using a dict vs tuples, but when in doubt I prefer the self-documenting nature of having dict keys.

Ultimately Screen probably should have returned something like a Response object that could handle a more complex use case like this. But that would be a huge refactor. But returning a dict here feels like a half-step in that direction and so is another very small nod in favor of dict over tuple.

AT THE VERY LEAST this Screen should include a TODO comment noting its unusual return value approach.

Thanks, @kdmukai. I just pushed d8b7672 following your suggestion for a dict approach and a TODO comment noting the unusual return value.

Maybe it's okay with that, or maybe we should include another comment regarding the type hints in the files you mentioned.

easyuxd · 2024-07-08T21:29:03Z

Maybe something like: "Your current passphrase entry will be erased. Proceed?"

I like this language. I do think it's a good practice to repeat the action, at least in the H1 and the CTAs -- it should be clear to users what their action is without having to read the whole screen, with the body copy as added context.

alvroble · 2024-07-09T15:40:56Z

Currently working in the dict approach as @kdmukai suggested. Regarding the warning screen, the text I proposed fits in three lines, which doesn't look quite right. Since we are already asking in the title, I removed the "Proceed?" to make it better looking. @easyuxd what do you think?

* Screen return values were updated to a more structured dict format * Test flows were updated accordingly * TODO comment was added suggesting a future refactor to return a Response object in order to support more complex use cases

jdlcdl · 2024-07-09T21:37:40Z

as of d7efbb3
ACK tested

@alvroble, great job on being responsive and flexible with others, and thank you.

alvroble · 2024-07-11T21:42:57Z

Just merged and tested after resolving some conflicts with the current dev branch

newtonick · 2024-07-14T01:56:31Z

tACK

closes #510

Add exit dialog when entering passphrase

59bcfda

Fixes / required changes: * Issue SeedSigner#510 'Behavior of back button confusing when entering password' * Updated test suite accordingly

jdlcdl reviewed Jul 6, 2024

View reviewed changes

Modified dialog text and button order

1670bf6

Updated dialog text and accordingly better variable naming

530e334

kdmukai reviewed Jul 8, 2024

View reviewed changes

src/seedsigner/views/seed_views.py Outdated Show resolved Hide resolved

kdmukai reviewed Jul 8, 2024

View reviewed changes

src/seedsigner/views/seed_views.py Show resolved Hide resolved

kdmukai reviewed Jul 8, 2024

View reviewed changes

alvroble added 2 commits July 9, 2024 17:53

Modified return value from tuple to dict

d8b7672

* Screen return values were updated to a more structured dict format * Test flows were updated accordingly * TODO comment was added suggesting a future refactor to return a Response object in order to support more complex use cases

Updated dialog text

d7efbb3

Merge dev into passphrase-exit-dialog and resolved conflicts

6c550e4

newtonick merged commit 0f26772 into SeedSigner:dev Jul 14, 2024
2 checks passed

kdmukai added a commit to kdmukai/seedsigner that referenced this pull request Jul 14, 2024

Fix to accomodate changes from SeedSigner#563

6d114f7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Enhancement] Add exit dialog when entering passphrase #563

[Enhancement] Add exit dialog when entering passphrase #563

alvroble commented Jun 30, 2024 •

edited

Loading

alvroble commented Jul 5, 2024 •

edited

Loading

jdlcdl commented Jul 6, 2024

jdlcdl Jul 6, 2024 •

edited

Loading

alvroble Jul 6, 2024

alvroble Jul 6, 2024

jdlcdl Jul 6, 2024 •

edited

Loading

dulcedu Jul 12, 2024

alvroble Jul 12, 2024

SeedSigner Jul 12, 2024

jdlcdl commented Jul 6, 2024 •

edited

Loading

alvroble commented Jul 6, 2024

jdlcdl commented Jul 6, 2024

alvroble commented Jul 7, 2024

jdlcdl commented Jul 7, 2024

easyuxd commented Jul 7, 2024

alvroble commented Jul 7, 2024

easyuxd commented Jul 7, 2024

alvroble commented Jul 7, 2024

kdmukai commented Jul 8, 2024

alvroble commented Jul 8, 2024

kdmukai Jul 8, 2024

alvroble Jul 9, 2024

easyuxd commented Jul 8, 2024

alvroble commented Jul 9, 2024

jdlcdl commented Jul 9, 2024

alvroble commented Jul 11, 2024

newtonick commented Jul 14, 2024

[Enhancement] Add exit dialog when entering passphrase #563

[Enhancement] Add exit dialog when entering passphrase #563

Conversation

alvroble commented Jun 30, 2024 • edited Loading

Description

Checklist

alvroble commented Jul 5, 2024 • edited Loading

jdlcdl commented Jul 6, 2024

jdlcdl Jul 6, 2024 • edited Loading

Choose a reason for hiding this comment

alvroble Jul 6, 2024

Choose a reason for hiding this comment

alvroble Jul 6, 2024

Choose a reason for hiding this comment

jdlcdl Jul 6, 2024 • edited Loading

Choose a reason for hiding this comment

dulcedu Jul 12, 2024

Choose a reason for hiding this comment

alvroble Jul 12, 2024

Choose a reason for hiding this comment

SeedSigner Jul 12, 2024

Choose a reason for hiding this comment

jdlcdl commented Jul 6, 2024 • edited Loading

alvroble commented Jul 6, 2024

jdlcdl commented Jul 6, 2024

alvroble commented Jul 7, 2024

jdlcdl commented Jul 7, 2024

easyuxd commented Jul 7, 2024

alvroble commented Jul 7, 2024

easyuxd commented Jul 7, 2024

alvroble commented Jul 7, 2024

kdmukai commented Jul 8, 2024

alvroble commented Jul 8, 2024

kdmukai Jul 8, 2024

Choose a reason for hiding this comment

alvroble Jul 9, 2024

Choose a reason for hiding this comment

easyuxd commented Jul 8, 2024

alvroble commented Jul 9, 2024

jdlcdl commented Jul 9, 2024

alvroble commented Jul 11, 2024

newtonick commented Jul 14, 2024

alvroble commented Jun 30, 2024 •

edited

Loading

alvroble commented Jul 5, 2024 •

edited

Loading

jdlcdl Jul 6, 2024 •

edited

Loading

jdlcdl Jul 6, 2024 •

edited

Loading

jdlcdl commented Jul 6, 2024 •

edited

Loading