Skip to content
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.

Sign up for GitHub

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

[Enhancement] Add exit dialog when entering passphrase #563

Merged
merged 6 commits into from
Jul 14, 2024
Merged

[Enhancement] Add exit dialog when entering passphrase #563

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
59bcfda
Add exit dialog when entering passphrase
alvroble Jun 30, 2024
1670bf6
Modified dialog text and button order
alvroble Jul 7, 2024
530e334
Updated dialog text and accordingly better variable naming
alvroble Jul 7, 2024
d8b7672
Modified return value from tuple to dict
alvroble Jul 9, 2024
d7efbb3
Updated dialog text
alvroble Jul 9, 2024
6c550e4
Merge dev into passphrase-exit-dialog and resolved conflicts
alvroble Jul 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/seedsigner/gui/screens/seed_screens.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -850,11 +850,11 @@ def _run(self):
self.hw_button3.is_selected = True
self.hw_button3.render()
self.renderer.show_image()
return self.passphrase
return self.passphrase, None

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


# Check for keyboard swaps
if input == HardwareButtonsConstants.KEY1:
Expand Down
44 changes: 38 additions & 6 deletions src/seedsigner/views/seed_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,19 +323,51 @@ def __init__(self):


def run(self):
ret = self.run_screen(seed_screens.SeedAddPassphraseScreen, passphrase=self.seed.passphrase)
ret_passphrase, ret_btn = self.run_screen(seed_screens.SeedAddPassphraseScreen, passphrase=self.seed.passphrase)

if ret == RET_CODE__BACK_BUTTON:
return Destination(BackStackView)

# The new passphrase will be the return value; it might be empty.
self.seed.set_passphrase(ret)
if len(self.seed.passphrase) > 0:
self.seed.set_passphrase(ret_passphrase)
if ret_btn == RET_CODE__BACK_BUTTON:
if len(self.seed.passphrase) > 0:
return Destination(SeedAddPassphraseExitDialogView)
else:
return Destination(BackStackView)
elif len(self.seed.passphrase) > 0:
return Destination(SeedReviewPassphraseView)
else:
return Destination(SeedFinalizeView)


class SeedAddPassphraseExitDialogView(View):
EXIT = ("Exit", None, None, "red")
CONTINUE = "Continue editing"

def __init__(self):
super().__init__()
self.seed = self.controller.storage.get_pending_seed()


def run(self):
passphrase = self.seed.passphrase

button_data = [self.EXIT, self.CONTINUE]

selected_menu_num = self.run_screen(
WarningScreen,
title="Exit",
Copy link

@jdlcdl jdlcdl Jul 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

SeedAddPassphraseExitDialogView

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.

Copy link

@jdlcdl jdlcdl Jul 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

status_headline=None,
text=f"Please confirm that you want to exit",
show_back_button=False,
button_data=button_data,
)

if button_data[selected_menu_num] == self.EXIT:
self.seed.set_passphrase("")
return Destination(SeedFinalizeView)

elif button_data[selected_menu_num] == self.CONTINUE:
return Destination(SeedAddPassphraseView)


class SeedReviewPassphraseView(View):
"""
Expand Down
1 change: 1 addition & 0 deletions tests/screenshot_generator/generator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ def test_generate_screenshots(target_locale):
seed_views.SeedMnemonicInvalidView,
seed_views.SeedFinalizeView,
seed_views.SeedAddPassphraseView,
seed_views.SeedAddPassphraseExitDialogView,
seed_views.SeedReviewPassphraseView,

(seed_views.SeedOptionsView, dict(seed_num=0)),
Expand Down
2 changes: 1 addition & 1 deletion tests/test_flows_psbt.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ def load_seed_into_decoder(view: scan_views.ScanView):
FlowStep(psbt_views.PSBTSelectSeedView, button_data_selection=psbt_views.PSBTSelectSeedView.SCAN_SEED),
FlowStep(scan_views.ScanSeedQRView, before_run=load_seed_into_decoder),
FlowStep(seed_views.SeedFinalizeView, button_data_selection=seed_views.SeedFinalizeView.PASSPHRASE),
FlowStep(seed_views.SeedAddPassphraseView, screen_return_value="abc"),
FlowStep(seed_views.SeedAddPassphraseView, screen_return_value=("abc", None)),
FlowStep(seed_views.SeedReviewPassphraseView, button_data_selection=seed_views.SeedReviewPassphraseView.DONE),
FlowStep(seed_views.SeedOptionsView, is_redirect=True),
FlowStep(psbt_views.PSBTOverviewView),
Expand Down
9 changes: 7 additions & 2 deletions tests/test_flows_seed.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,14 @@ def test_passphrase_entry_flow(self):
FlowStep(MainMenuView, button_data_selection=MainMenuView.SCAN),
FlowStep(scan_views.ScanView, before_run=load_seed_into_decoder), # simulate read SeedQR; ret val is ignored
FlowStep(seed_views.SeedFinalizeView, button_data_selection=seed_views.SeedFinalizeView.PASSPHRASE),
FlowStep(seed_views.SeedAddPassphraseView, screen_return_value="muhpassphrase"),
FlowStep(seed_views.SeedAddPassphraseView, screen_return_value=("muhpassphrase",RET_CODE__BACK_BUTTON)),
FlowStep(seed_views.SeedAddPassphraseExitDialogView, button_data_selection=seed_views.SeedAddPassphraseExitDialogView.EXIT),
FlowStep(seed_views.SeedFinalizeView, button_data_selection=seed_views.SeedFinalizeView.PASSPHRASE),
FlowStep(seed_views.SeedAddPassphraseView, screen_return_value=("muhpassphrase",RET_CODE__BACK_BUTTON)),
FlowStep(seed_views.SeedAddPassphraseExitDialogView, button_data_selection=seed_views.SeedAddPassphraseExitDialogView.CONTINUE),
FlowStep(seed_views.SeedAddPassphraseView, screen_return_value=("muhpassphrase",None)),
FlowStep(seed_views.SeedReviewPassphraseView, button_data_selection=seed_views.SeedReviewPassphraseView.EDIT),
FlowStep(seed_views.SeedAddPassphraseView, screen_return_value="muhpassphrase2"),
FlowStep(seed_views.SeedAddPassphraseView, screen_return_value=("muhpassphrase2",None)),
FlowStep(seed_views.SeedReviewPassphraseView, button_data_selection=seed_views.SeedReviewPassphraseView.DONE),
FlowStep(seed_views.SeedOptionsView),
])
Expand Down
2 changes: 1 addition & 1 deletion tests/test_flows_tools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ def load_seed_into_decoder(view: scan_views.ScanView):
self.run_sequence(
sequence=[
FlowStep(seed_views.SeedFinalizeView, button_data_selection=seed_views.SeedFinalizeView.PASSPHRASE),
FlowStep(seed_views.SeedAddPassphraseView, screen_return_value="mypassphrase"),
FlowStep(seed_views.SeedAddPassphraseView, screen_return_value=("mypassphrase",None)),
FlowStep(seed_views.SeedReviewPassphraseView, button_data_selection=seed_views.SeedReviewPassphraseView.DONE),
FlowStep(seed_views.SeedOptionsView, is_redirect=True),
FlowStep(seed_views.SeedExportXpubScriptTypeView),
Expand Down
Loading