-
Notifications
You must be signed in to change notification settings - Fork 40
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
UI Only AU Snapshot Loading Flow #421
base: main
Are you sure you want to change the base?
Conversation
Concept ack |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't it make more sense to (also) have this part of the onboarding workflow?nvm, I think we shouldn't do this (maybe later)- Also, having it at
Connection
menu would not be something where I would be looking at to find this feature. Maybe it needs anAdvanced
section? - A UI for an incorrect snapshot and/or failure of importing the snapshot is missing
Great to see this come to PR stage... how long is the loading process expected take to take on an avg computer? like in the range of seconds or minutes or hours? |
9fc1985
to
ed27a5a
Compare
With ed27a5a addressed comments and added loading snapshot simulation for better testing and feedback |
ed27a5a
to
5c61613
Compare
That depends on the user's device and the snapshot (signet, testnet, and mainnet) they are importing. Averages are difficult to calculate since every device is different... |
That's a great idea! Will be joining future calls so we can discuss design and UX matters. meanwhile please take the time to test it on your device using one the builds here |
minor tweak with 5c61613 changed the "Import Snapshot" description to "Instant use with background sync" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK
- Load/ Loading instead of Import. Everywhere in the documentation is using this term I'd keep it consistent.
- Import label description "Speed up initial download". This is not entirely true, that would be more the assume-valid feature. The point of assume-utxo is to allow the user to see recent transactions and spend them before having to wait until the full sync is done.
- Once the snapshot load is completed, the last page in figma, a message says that the snapshot file will be removed, perhaps this can be optional or asked to the user before starting the process.
- Last thing, the green ticked icon indicating completion (and background validation), once the validation of the snapshot chain is done, those files related with assume-utxo will be deleted automatically (chain state directory), so there's no way whether a node was "utxo-set" sped up or not. So not sure how long this green indicator would last.. definitely won't be there next restart as explained.
Please also add the figma link of the designers team definitions in the description, I'll add these comments also there.
5c61613
to
a1193fc
Compare
With a1193fc Changed "Import" to "Load" based on @pablomartin4btc suggestion Also updated the description to include figma link and a screenshot of the "Progress Bar" page |
Yeah maybe a button can be added so that the user has the option to delete the snapshot file. This will be especially relevant for mobile (android) users looking to save storage space on their device. |
Great point, maybe the option can be greyed out with a check mark to indicate the snapshot has been fully validated and the node is synced. |
a1193fc
to
f016406
Compare
update f016406 rebase over main |
Thanks for working on this. I just tested locally, and there are a lot of visual details that need tweaking (icon needs to be bigger, title text bold, vertical spacing too tight, line-height too tight, etc). Do you want me to list those out? It's pretty easy to spot them comparing the application to the designs. A larger thing is the page transitions. The idea here is that the snapshot flow is just "one page deep". So instead of this:
It's just this:
So you stay in the same screen, but the state of the screen changes and therefore the info you see, with fade out/in transitions. You can see this in the web prototype. Regarding the point about indicating that the snapshot was validated completely. My assumption was that at that point (snapshot is validated and the user has seen the screen that states so) we replace the "Load snapshot" functionality with the "Create snapshot" functionality. Does that work? |
Also, would it be possible to fix the actions? They are currently not building all the artifacts correctly. Thanks. |
from @jarolrod in discord PM: "the ci error is unrelated to code and just needs to be rerun" |
No worries, I'll make the tweaks and see if they are addressed... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK f016406
Regarding the commits, the first commit adds the files that don't exist yet, I'd move the first commit to the end, becoming the 4th.
The 2nd commit imports the controls that are being updated in the 3rd, so I'd swap these 2 commits.
That's all, I'll review the code when I play around with the testing.
Thanks for taking a look... I'm about to update the commits incorporating Christoph's comments... So please wait before testing? |
f016406
to
e5bbabd
Compare
with e5bbabd squashed the commits into one, addressed spacing issues, and got rid off pages using Stack layout in SnapshotSettings.qml |
Ok, it was not necessary as per my review, the separation made sense, just the order, but up to you. I think for the lint failure you'll need to exclude the |
This introduce the UI flow to load a AssumeUTXO snapshot into the Bitcoin Core App. It modifies the connection seetings and adds a SnapshotSettings file, Icon, and modified profress bar.
e5bbabd
to
3ca75a4
Compare
Thanks, decided to adhere to the CONTIBUTING guidelines more thoroughly ;) For the lint it turns out a couple of svg files had trailing whitespaces, 3ca75a4 addresses that |
Introducing the UI flow to be able to import a AssumeUTXO (AU) snapshot into the Bitcoin Core App.
The user will be able to load an AU snapshot from the "Connection Settings" page as designed here also AU figma
*This is a UI only commit no actual functionality has been coded. Wiring to actually import a AU snapshot will happen in a future PR
Ubuntu 22.04 Screenshots