Skip to content

Resolve 547 (Surface "Create a Seed" early on "In-Memory Seeds")#580

Open
jdlcdl wants to merge 5 commits intoSeedSigner:devfrom
jdlcdl:resolve_547
Open

Resolve 547 (Surface "Create a Seed" early on "In-Memory Seeds")#580
jdlcdl wants to merge 5 commits intoSeedSigner:devfrom
jdlcdl:resolve_547

Conversation

@jdlcdl
Copy link

@jdlcdl jdlcdl commented Jul 19, 2024

Description

resolves #547

Can now "Create a seed" from:

  • Seeds menu with seed(s) loaded instead of going thru "Load a seed", but title is "Create a Seed" and no Address Explorer/Verification offered

Flows supported are:

  • flow to it immediately in Seeds menu w/ seeds loaded... and that other tools are not available.
    SeedsMenuView ToolsViaCreateASeed

  • flow to it from "Load a seed" screen IF NO OTHERS ALREADY LOADED... and that other tools are not available.
    LoadSeedView
    TY Alvroble for the insight that to offer Create would be redundant (it was on the last screen) and confusing since Create and Load are being separated.
    LoadSeedView

  • flow to it from Tools menu, and the other tools ARE available.
    ToolsMenuView
    This is not new, but code changed in this pr does affect this view.

This pull request is categorized as a:

  • New feature
  • Bug fix
  • Code refactor
  • Documentation
  • Other

Checklist

  • I’ve run pytest and made sure all unit tests pass before submitting 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:

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.

@jdlcdl jdlcdl marked this pull request as draft July 19, 2024 21:09
@alvroble
Copy link
Contributor

As of e28308c ACK and tested.

Thank you for this enhancement. I think the "Create a seed" option is currently a bit hidden when seeds are already loaded, and this fixes that 👍.

From a functional point of view, it works as expected. However, one thing worth considering is removing the "Create a seed" option from LoadSeedView, as this button just appears on the previous screen. For consistency, I think it is better to have a clear path to each function in these situations, and having these three paths seems a bit confusing to me:

  • "Tools" --> "New Seed"
  • "Seeds Menu" --> "Create a seed" --> "New Seed"
  • "Seeds Menu" --> "Load a seed" --> "Create a seed" --> "New Seed"

Maybe the first two paths would be sufficient. Of course, this is just my opinion.

@jdlcdl
Copy link
Author

jdlcdl commented Jul 20, 2024

Maybe the first two paths would be sufficient. Of course, this is just my opinion.

I agree. I almost disabled this 3rd path but at last moment thought "What about users who already expect it here?"; at least for one release cycle until they get used to the more direct path.

I'll count your vote to remove it as 1, my own want to do the same as 1/2, and will wait to hear what others think. till then, I have more tests to get done and am currently testing the current release candidate.

Thank you for your review @alvroble!

@jdlcdl jdlcdl marked this pull request as ready for review July 29, 2024 17:52
@jdlcdl
Copy link
Author

jdlcdl commented Jul 29, 2024

Ready for review, but STILL UNRESOLVED: Need opinion from other devs.

To remove "Create a Seed" from the "Load a Seed" menu when other seeds are already loaded? It doesn't need to be here any longer, it was in the previous screen just below "Load a Seed".

@easyuxd
Copy link
Contributor

easyuxd commented Aug 1, 2024

Agree with you both on removing "Create a seed" from the "Load a seed" menu, since these functions are now separated. Great catch, @alvroble and @jdlcdl!

Thanks for making this QoL enhancement a reality, @jdlcdl!

@jdlcdl
Copy link
Author

jdlcdl commented Aug 1, 2024

ty @alvroble and @easyuxd for your thoughts on keeping "Create a seed" and "Load a seed" separated. This particular behavior is implemented as of c3ddd34.

@newtonick
Copy link
Collaborator

tACK. LGTM

@newtonick newtonick added this to the 0.9.0 milestone Aug 30, 2024
@newtonick newtonick added enhancement New feature or request gui labels Oct 25, 2024
@bitcoinprecept bitcoinprecept moved this to 9.0 Needs Code Review in @SeedSigner Development Board Mar 14, 2025
@kdmukai
Copy link
Contributor

kdmukai commented May 20, 2025

Minor request @jdlcdl: Can you update the PR title to be more descriptive?

@jdlcdl jdlcdl changed the title Resolve 547 Resolve 547 (Surface "Create a Seed" early on "In-Memory Seeds") Jun 6, 2025
@newtonick newtonick added the merge conflicts has merge conflicts that need resolution label Oct 13, 2025
@kdmukai
Copy link
Contributor

kdmukai commented Oct 13, 2025

@jdlcdl can you resolve the merge conflicts?

@newtonick newtonick moved this from 0.8.7 Needs Code Review to 0.9.0 Needs Code Review in @SeedSigner Development Board Dec 12, 2025
@newtonick newtonick modified the milestones: 0.8.7, 0.9.0 Dec 13, 2025
@newtonick
Copy link
Collaborator

I'm proposing to close this pull request without merge simply because it's gone stale. Planning to close after 7 days if no feedback saying otherwise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request gui merge conflicts has merge conflicts that need resolution

Projects

Status: 0.9.0 Needs Code Review

Development

Successfully merging this pull request may close these issues.

[Enhancement] Add Create Seed to In-Memory Seeds menu

5 participants