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

UI Only AU Snapshot Loading Flow #421

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

D33r-Gee
Copy link
Contributor

@D33r-Gee D33r-Gee commented Sep 18, 2024

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

Screenshot 2024-09-18 142821
Screenshot 2024-09-18 142834
Screenshot 2024-09-26 111410
Screenshot 2024-09-18 142910
Screenshot 2024-09-18 142930

@jarolrod
Copy link
Member

Concept ack

Copy link
Contributor

@MarnixCroes MarnixCroes left a 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 an Advanced section?
  • A UI for an incorrect snapshot and/or failure of importing the snapshot is missing

src/qml/components/ConnectionSettings.qml Outdated Show resolved Hide resolved
src/qml/pages/settings/SettingsProgressBar.qml Outdated Show resolved Hide resolved
@yashrajd
Copy link

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?
We should probably discuss the designs in one of the design community calls.

@D33r-Gee
Copy link
Contributor Author

With ed27a5a addressed comments and added loading snapshot simulation for better testing and feedback

@D33r-Gee
Copy link
Contributor Author

how long is the loading process expected take to take on an avg computer?

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...
Once we test the functionality (which this PR does not address, being UI only) it will become clearer how long the import process takes.

@D33r-Gee
Copy link
Contributor Author

We should probably discuss the designs in one of the design community calls.

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

@D33r-Gee
Copy link
Contributor Author

minor tweak with 5c61613 changed the "Import Snapshot" description to "Instant use with background sync"

Copy link
Contributor

@pablomartin4btc pablomartin4btc left a 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.

cc @fjahr, @Sjors, @GBKS

@D33r-Gee
Copy link
Contributor Author

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

@D33r-Gee
Copy link
Contributor Author

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

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.

@D33r-Gee
Copy link
Contributor Author

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

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.

Thoughts @yashrajd @GBKS?

@D33r-Gee D33r-Gee changed the title UI Only AU Snapshot Import Flow UI Only AU Snapshot Loading Flow Sep 26, 2024
@D33r-Gee
Copy link
Contributor Author

update f016406 rebase over main

@GBKS
Copy link
Contributor

GBKS commented Oct 1, 2024

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:

Settings -> Snapshot -> Loading snapshot -> Done

It's just this:

Settings -> Snapshot

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?

@GBKS
Copy link
Contributor

GBKS commented Oct 1, 2024

Also, would it be possible to fix the actions? They are currently not building all the artifacts correctly. Thanks.

@D33r-Gee
Copy link
Contributor Author

D33r-Gee commented Oct 1, 2024

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"

@D33r-Gee
Copy link
Contributor Author

D33r-Gee commented Oct 1, 2024

Do you want me to list those out?

No worries, I'll make the tweaks and see if they are addressed...

Copy link
Contributor

@pablomartin4btc pablomartin4btc left a 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.

@D33r-Gee
Copy link
Contributor Author

D33r-Gee commented Oct 2, 2024

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?

@D33r-Gee
Copy link
Contributor Author

D33r-Gee commented Oct 3, 2024

with e5bbabd squashed the commits into one, addressed spacing issues, and got rid off pages using Stack layout in SnapshotSettings.qml

@pablomartin4btc
Copy link
Contributor

with e5bbabd squashed the commits into one

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 QML resources as it's done for QT in get_pathspecs_exclude_whitespace (test/lint/test_runner/src/main.rs): "src/qt/res/src/*.svg",

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.
@D33r-Gee
Copy link
Contributor Author

D33r-Gee commented Oct 3, 2024

the separation made sense

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

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

Successfully merging this pull request may close these issues.

6 participants