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.

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

Recovery go to previous word #3458

Merged
merged 1 commit into from
Feb 19, 2024
Merged

Recovery go to previous word #3458

merged 1 commit into from
Feb 19, 2024

Conversation

grdddj
Copy link
Contributor

@grdddj grdddj commented Dec 12, 2023

Fixes #286:

  • allowing to go back to previous word in recovery process

I am creating a bunch of click tests to try all the possible scenarios

@grdddj grdddj requested review from mmilata and Hannsek and removed request for prusnak and matejcik December 12, 2023 12:48
@grdddj grdddj self-assigned this Dec 12, 2023
Copy link
Member

@mmilata mmilata left a comment

Choose a reason for hiding this comment

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

Looking good.

Are you planning to add more tests as part of this PR?

I noticed that in model T you can go and change any previous word, but on TS3 you can only go to the last one, is that intentional?

@Hannsek
Copy link
Contributor

Hannsek commented Dec 15, 2023

I can change any previous word on TS3.

If someone press previous, it goes to the screen where there is delete button and the word:
image

Instead, can it be the screen with multiple options?
image
This is because if I go back, I have to delete at least 2 letters every time I want to change something

On TT, instead this screen
image
go to this
image

(Instead
image
go to
image)

In general, go to screen that has multiple options without deleting single letter.

Ideally, if this screen suggests the same word. See the word machine, there it is right. But instead of bind you can see the suggested bicycle, that's not right.

@grdddj
Copy link
Contributor Author

grdddj commented Jan 2, 2024

I can change any previous word on TS3.

If someone press previous, it goes to the screen where there is delete button and the word: image

Instead, can it be the screen with multiple options? image This is because if I go back, I have to delete at least 2 letters every time I want to change something

On TT, instead this screen image go to this image

(Instead image go to image)

In general, go to screen that has multiple options without deleting single letter.

Ideally, if this screen suggests the same word. See the word machine, there it is right. But instead of bind you can see the suggested bicycle, that's not right.

These things are technically possible, but certainly not easily/cleanly. I would vote for simplicity --- the cost of users having to click delete a couple of times is minimal, considering how very few users will ever encounter this situation.

@grdddj
Copy link
Contributor Author

grdddj commented Jan 2, 2024

Are you planning to add more tests as part of this PR?

Yes, I will add more tests, once we finalize the wanted requirements.

@grdddj
Copy link
Contributor Author

grdddj commented Jan 10, 2024

These things are technically possible, but certainly not easily/cleanly. I would vote for simplicity --- the cost of users having to click delete a couple of times is minimal, considering how very few users will ever encounter this situation.

@Hannsek Want to ask about the current status - are you OK with this, or do you want to do the changes above? I would estimate it at couple hours to 1 day of work.

If we are OK with the current state, I will do some more tests to freeze the functionality and then we are ready to merge.

@Hannsek
Copy link
Contributor

Hannsek commented Jan 10, 2024

Im ok with this for now.
But please change the word delete to previous on TS3 when no letter is input, if not yet changed.

@Hannsek
Copy link
Contributor

Hannsek commented Jan 25, 2024

@grdddj What is missing here?

@grdddj
Copy link
Contributor Author

grdddj commented Jan 29, 2024

@grdddj What is missing here?

I think that nothing really important. The tests I wanted to add do not have a big priority

@Hannsek
Copy link
Contributor

Hannsek commented Jan 29, 2024

So we can merge it?

Copy link

github-actions bot commented Jan 29, 2024

legacy UI changes device test(screens) main(screens)

Copy link

github-actions bot commented Jan 29, 2024

core UI changes device test click test persistence test
Model T test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
Model Safe 3 test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
All main(screens)

@grdddj
Copy link
Contributor Author

grdddj commented Jan 29, 2024

So we can merge it?

I guess so, after the CI will be green and the PR is approved

Copy link
Member

@mmilata mmilata left a comment

Choose a reason for hiding this comment

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

I think it's ready but some tests are failing. Also please remove the TODO lines and/or turn them into GH issue.

@grdddj
Copy link
Contributor Author

grdddj commented Feb 5, 2024

I think it's ready but some tests are failing. Also please remove the TODO lines and/or turn them into GH issue.

Right, sorry about that. Should be rebased and green now, TODOs deleted

@grdddj
Copy link
Contributor Author

grdddj commented Feb 19, 2024

CI is green, ready to merge @mmilata

@mmilata mmilata merged commit 0579ba5 into main Feb 19, 2024
60 of 61 checks passed
@mmilata mmilata deleted the grdddj/recovery_go_back branch February 19, 2024 21:05
@bosomt
Copy link

bosomt commented Feb 22, 2024

Testing on dry run.
Previous button is available even before i enter first word /nothing happens when pressed/
Is that expected ?

Info:

  • Suite version: desktop 24.2.2 (6544eb4cdb6f8982f3f391307e2bff5784db2bcf)
  • Browser: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) TrezorSuite/24.2.2 Chrome/118.0.5993.129 Electron/27.0.4 Safari/537.36
  • OS: MacIntel
  • Screen: 1512x982
  • Device: Trezor T2B1 2.6.5 regular (revision e1f696b)
  • Transport: BridgeTransport 2.0.33

@Hannsek
Copy link
Contributor

Hannsek commented Feb 22, 2024

It is not. Good catch. @grdddj

@grdddj
Copy link
Contributor Author

grdddj commented Feb 26, 2024

It is not. Good catch. @grdddj

That is OK in my perspective, if the button does nothing. Yeah, it would be clearer when not showing it at all, but at the cost of more code.

How do we want it to look/behave like?

@Hannsek
Copy link
Contributor

Hannsek commented Feb 26, 2024

Ideally, it should not be there if no word is present. I'll create an issue.

@bosomt
Copy link

bosomt commented Feb 27, 2024

QA OK

tested dry run on firmware-T2T1-2.6.5-561a697
tested recovery on firmware-T2T1-2.6.5-561a697
tested dry run on firmware-T2B1-2.6.5-561a697
tested recovery on firmware-T2B1-2.6.5-561a697

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Approved
Development

Successfully merging this pull request may close these issues.

Recovery: Add a return button to go back when entering words
5 participants