SeedSigner / seedsigner

Use an air-gapped Raspberry Pi Zero to sign for Bitcoin transactions! (and do other cool stuff)
MIT License
720 stars 168 forks source link

[Enhancement] Dice Roll Menu: Jump from “6” to “del” When Pressing Down #555

Open 2bitpixl opened 6 months ago

2bitpixl commented 6 months ago

When the dice with the number “6” is selected, pressing the down button currently leads to the go-back element “<”. IMG_5660

However, it might be more consistent if the behavior were the same as when the dice with the numbers “4” or “5” is selected. In those cases, pressing down takes you to the “del” element as expected.

fedebuyito commented 2 months ago

Hi, this issue could be solved as it follows, on /src/seedsigner/gui/keyboard.py:

image

Growning "del" button size from 2 to 3 user can navigate from 6th dice when pressing down:

seedsigner_del_button

Regards. -

fedebuyito commented 2 months ago

@jdlcdl May I do the PR or could you tell me how I can to colaborate?

jdlcdl commented 2 months ago

@jdlcdl May I do the PR or could you tell me how I can to colaborate?

But... you already ARE colaborating, and you're off to a fine start.

If you are not already in the "SeedSigner Devs" telegram group (or the main one), links to them are below:

But you don't necessarily have to, since you've worked it out here in this issue (and it appears you've tested the code too). You could then simply fork this repo, create a branch (for example, named "resolves_issue_555" or "dice_roll_menu_navigation" or however you like) so that a single branch contains the changes that you want to make.

Once your changes are ready and you have pushed them to your github fork of seedsigner, you'll be offered a button to "create pull request" so that you can write a short blurb about your changes, including a line like "solves issue #555" which is github-site-magic that will create a link to this issue and also close this issue when your pr finally gets merged. Once submitted, all devs will be able to track your pr and test it themselves. If you need to do more changes after feedback, you'll just keep pushing commits to the same branch and we'll see them as they evolve.

Perhaps, remember to bring-up your pr in the telegram group now-and-again until merged (if you think we've forgotten about it).

Welcome @fedebuyito!, and thank you for your contribution thus far.

fedebuyito commented 2 months ago

Thank you @jdlcdl for your kindly answer and advices. I go to do you recomend me.

Yes, I have tested the code with https://github.com/enteropositivo/seedsigner-emulator/tree/master tool, prior to acquire the hardware. Maybe others could to test the fork on real platform.

See you on telegram group!

kdmukai commented 2 months ago

This is a broader question than just the "DEL" handling for this one screen.

SeedExportXpubCustomDerivationView SeedBIP85SelectChildIndexView SeedMnemonicEntryView SeedAddPassphraseView (2)

The UI / UX has thus far been consistent about the open hanging space on the final line. So I think we should be cautious about changing that behavior on just one of the keyboard screens. That being said, (more below) I don't know that there's an ideal single consistent solution.


There are at least 3 possible paths here:

  1. Keep it as is: can only navigate DOWN to "DEL" if you're on a key above it.
  2. Expand DEL to fill any remaining gap (the solution presented here, and could be applied to other keyboard screens)
  3. Where possible, move DEL to one of the side KEY1, KEY2, KEY3 buttons.
  4. Keep the physical layout (DEL does not reach the far edge) but change the DOWN behavior to land on it anyway.

I dislike the idea of spanning DEL across an entire keyboard. The way it's implemented above, it looks like the screen's main Call to Action (e.g. a "Done" button; tagging in @easyuxd). At a minimum it needs to be a shorter button height to at least slightly differentiate it from our normal menu buttons.

But even with that change, I just don't think the very minor UX gain is worth the worse UI presentation.

Could also use 3.) on this screen instead. I wouldn't mind shrinking the dice keys a bit to create the horizontal room to put DEL on KEY2.


Note: 4.) has some annoying unsolvable inconsistencies. On the dice screen, if you go DOWN from 6, you'd get a diagonal move to DEL. Okay. But let's say that was an accidental move. So you press UP to go back. Do you land back on 6?

What if you do the same, but on purpose. You click DEL and now you want to navigate up to 4. But you get annoyed that UP from DEL now jumps diagonally to land you on 6.

I can't recall how it was implemented when it spans 2+ keys. My guess is that it remembers your entry column and returns you there.


That being said, the first two screenshots (custom derivation path, bip85 child index) could also use solution 2.) since there's room to use the middle (KEY2) button. DEL as KEY2 is definitely a better UX and faster. Only downside is that we can't use that for mnemonic word entry nor passphrase entry. So we'd have two different DEL button UXs.

The 3rd screenshot (mnemonic word entry) could expand DEL to fill the gap to the right edge. That feels like an easy net win.

The 4th screenshot (passphrase entry) is basically hopeless because of the third row.

fedebuyito commented 2 months ago

Trying to apply additional issues @kdmukai proposes, I achieved to fit del button on those screens, regarding to approach n.2 he suggets (keeping one single DEL button UXs):

seedsigner_del_custom

easyuxd commented 2 months ago

I think this is a solid enhancement.

@fedebuyito: You've done a good job of keeping the Del change consistent across the input screens. Users will benefit from the larger Del key and the improved navigation that comes with it.

I would love to see some before-and-after screenshots if you're able. The video you posted is helpful for interactions but a GIF is hard to navigate and pause.

@kdmukai: I agree with your hesitancy around a larger Delete key looking like a CTA. Although looking at this closer, the Delete key not having a fill helps to de-emphasize it despite its increased size. No confusion on my part (not sure what others think?) but of course I defer to you because you designed these input screens and know them best.

fedebuyito commented 2 months ago

Thank you for your comments @easyuxd and @kdmukai.

Yes, sure. Here they go (BEFORE/AFTER) screens: (Some ones keep the same, but code needed touched)

ToolsDiceEntropyEntryScreen (issue first one): 1_BEFORE 1_AFTER

ToolsCoinFlipEntryScreen (this one I missed on gif animation): 2_BEFORE 2_AFTER

SeedMnemonicEntryScreen: 3_BEFORE 3_AFTER

SeedBIP85SelectChildIndexScreen: 4_BEFORE 4_AFTER

SeedExportXpubCustomDerivationScreen: 5_BEFORE 5_AFTER

SeedAddPassphraseScreen (x5): 6_BEFORE 6_AFTER

7_BEFORE 7_AFTER

8_BEFORE 8_AFTER

9_BEFORE 9_AFTER

10_BEFORE 10_AFTER

Would are more involucred screens(?)

Regards. -