-
Notifications
You must be signed in to change notification settings - Fork 41
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 Custom Datadir Display #397
UI Only Custom Datadir Display #397
Conversation
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.
Based on both the figma and the web prototype, the path should only be displayed on the custom selection after the user has made a choice.
https://lively-kashata-cfde7e.netlify.app/screen/first-use/storage-location?state=default
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.
tACK 0025e91
-
Noticed the same as @johnny9, proposed a change below (1) which also fixes the prefix of the
customDir
"file://
" before the file dialog gets open or if the user cancels it. -
Currently when the user cancels to set a custom datadir the custom option is still checked when perhaps we should go back to the default datadir being checked. So I proposed another change to that (2) with a caveat where if the user sets the custom, then wants to change it again but cancels it, it remains there in the custom checked.
-
We need to tackle the double click issue at some point. I spent some time with it but couldn't solved it yet.
-
First time I run it I got the crash reported by @MarnixCroes on DRAFT: Custom Datadir Wiring #390, and more often I get another errors related with Gtk image in the console, which I reported (3.) already in DRAFT: Custom Datadir Wiring #390 as well, both errors are produced as soon as I open the file dialog.
-
crash error
** Gtk:ERROR:../../../../gtk/gtkpathbar.c:1342:set_button_image_get_info_cb: assertion failed: (cancellable == data->button_data->cancellable) Bail out! Gtk:ERROR:../../../../gtk/gtkpathbar.c:1342:set_button_image_get_info_cb: assertion failed: (cancellable == data->button_data->cancellable) Aborted (core dumped)
-
Gtk image warning
(bitcoin-qt:1496162): Gtk-CRITICAL **: 20:32:43.950: gtk_image_set_from_gicon: assertion 'GTK_IS_IMAGE (image)' failed
-
To be clear we don't want to display the default datadir path here, correct? However do we want to display the default path in StorageSettings.qml? |
As of figma, and the web prototype (link above) no, but it won't hurt to show it, I'm not sure. Also, sorry if I'm adding confusion here but on a previous PR #390 there was a "Recommended" highlighted label on the default choice when I reviewed it, was that dropped (if ever discussed)?
I think so, whatever it is set default or custom. cc @GBKS |
36467ea
to
dfc0023
Compare
I don't have a strong opinion about this so i would defer to the current figma. I do think the "Recommended" highlight is redundant as "Default" has the same meaning so I would remove that to match the figma |
Just tested on desktop (Mac) and mobile (Android). I agree with Johnny's rationale about ditching the Something I noticed is that the directory display of the custom option disappears if I select the default one again (center). It feels unexpected, could we keep that visible? I could go either way on showing the default directory. Might be more consistent if we just show it, so users know what they are changing from. For the rightmost screenshot, could we switch to a vertical layout for the directory field? So the directory goes underneath the label? It's a bit awkward to have it squeezed on the right side. |
Storage error message: What would be the error message that a user would see if they choose to proceed but their device does not have sufficient storage? |
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.
re tACK dfc0023
Verified my previous feedback got addressed. Usability seems more consistent.
Some observations (perhaps we need to address them with a new "onboarding related" PR):
-
Storage space estimation looks wrong during onboarding (e.g. starting the app simply with
bitcoin-qt
vs.bitcoin-qt -signet
, please expand to see the screenshots) - Also, should we keep "Recommended" highlighted here?
-
Using
-datadir
behaviour doesn't trigger the onboarding (neither combined with-chain=
option - e.g.-signet
with no-datadir
triggers the onboarding when signet subdir doesn't exist on default datadir structure but setting-datadir
option +-signet
with no subdire doesn't) and it should when that custom datadir is empty otherwise:./src/qt/bitcoin-qt -datadir="/home/pablo/custom" Warning: Ignoring XDG_SESSION_TYPE=wayland on Gnome. Use QT_QPA_PLATFORM=wayland to run on Wayland anyway. Running initialization in thread Warning: Disk space for "/home/pablo/custom/blocks" may not accommodate the block files. Approximately 540 GB of data will be stored in this directory.
@D33r-Gee, could you please try to investigate root of the crash mentioned at the end of my previous review? It seems it happens at the very first time you run bitcoin-qt
(need to remove datadir and bitcoin config dir) using the file dialog of course, otherwise it doesn't happen. Most of the time the user would get this error message in the terminal:
(bitcoin-qt:756208): Gtk-CRITICAL **: 10:57:38.210: gtk_image_set_from_gicon: assertion 'GTK_IS_IMAGE (image)' failed
@GBKS for future reference, please add the commit version you've tested if possible so we can verify if it's the latest or a previous one.
Ok, please check the next page when user has to select the amount of space to store, there's also a "Recommended" highlighted label.
I've proposed to hide it when a user goes back to default datadir to avoid confusion, as in when a user sees the page for the first time won't see the custom datadir until the user clicks on it and set it. Is it necessary if that's not the datadir that's going to be used?
I'd show it but I've also thought if we can add an external link, not sure if with an information icon or something, to the bitcoin datadir documentation. |
I think that should be after accepting (clicking on next) the following page "Reduce store"/ "Store all data" (before starting the initial download sync). |
As discussed on the designers call today, just keep in mind that the validation should be on the next page: (crossed 9GB in red as it's incorrect) |
Can we detect how much space is available on each location and show it? And can we estimate the required space needed (the mock-up uses 500GB as a placeholder, which is meant to be replaced with something accurate). |
dfc0023
to
2fbb27a
Compare
2fbb27a
to
8e8aadf
Compare
@GBKS and @pablomartin4btc Thanks for the feedback regarding the storage amount and the error messaging. Meanwhile please me know your thoughts on the latest update 8e8aadf? |
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.
8e8aadf
I cannot repro the double-click issue anymore #399
friendly ping @pablomartin4btc
one issue/inconsistency:
the selected custom data dir is not updated/displayed correctly during the onboarding, but it as at settings:
onboarding -> select custom data dir -> next -> click Detailed settings
-> default data dir is displayed -> next -> next -> arrive at home screen -> go to settings
-> storage
-> data directory displays the custom data dir
That's great to hear...
Could you post a screenshot please? |
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.
8e8aadf I cannot repro the double-click issue anymore #399 friendly ping @pablomartin4btc
Ok, thanks for letting me know... I'll do another test on the latest version of this PR.
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.
Seems to be an issue with building the Android target
"qml/models/options_model.cpp:186:55: error: no matching member function for call to 'replace'
m_custom_datadir_string = m_custom_datadir_string.replace("content://com.android.externalstorage.documents/tree/primary%3A", "/storage/self/primary/");"
2e2fe79
to
7341bdf
Compare
Thanks for discovering this... Found the cause of the problem, I mistakenly left the added |
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 7341bdf
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.
tested lgtm 7341bdf
one nit/UX thing: select/have a custom data dir -> select default data dir -> select custom data dir -> it prompts the folder location selection again.
for me it would be more intuitive to not prompt the storage location for one click -> only when double-clicking. when clicking on the custom datadir again
FTR, can repro the double-click in background issue on latest commit. not sure why previously I couldn't repro #397 (review) |
The MacOS workflow runs seem to fail for this PR (also for 401). Would it be possible to fix those to simplify testing? |
Thanks for testing and the feedback. That's a good point, will add it to the polishing features alongside the error messaging. One thing to keep in mind is that on mobile "double-tapping" is uncommon so we'll have to play with it to see what feels most natural. |
Hmmm... well that's problematic... will discuss with the other devs and see what can be done... Thanks for pointing it out! |
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.
tACK 7341bdf
Another UX nit: when a user selects a custom datadir and accepts it, clicks again on custom datadir to open the file dialog, creates a different custom dir but cancels the dialog, it goes wrongly to tick/ check the default datadir. Perhaps we can correct that on a follow-up.
Standing issues to follow-up:
- Custom Datadir double click (check workaround at Bugfix custom datadir doubleClick - Follow-up #392 #399).
- custom data dir -> select default data dir -> select custom data dir -> it prompts the folder location selection again (reported by @MarnixCroes)
- not sure about differentiation between 1 click (for just tick the custom dir) or double to open a file dialog, perhaps adding a button/ icon to open the dialog but would break this design, I'd leave this to the design experts.
I still get this error the first time I run QT and open the File Dialog... could be something related with my initial env, after the first time it's not reproducible, leaving the comment here to follow-up later:
edit: (sorry, forgot to add the error...)
(bitcoin-qt:1496162): Gtk-CRITICAL **: 20:32:43.950: gtk_image_set_from_gicon: assertion 'GTK_IS_IMAGE (image)' failed
@GBKS: Agreed. Just found another case (ref.: "Another UX nit"). |
2a56d6b
to
c2e73b2
Compare
c2e73b2 rebased over main |
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.
it can happen that the GUI completely freezes
can't do anything, need to close the app.
can repro most of the time, but not 100%
maybe a bit of an edge case, as it's not the usual user flow for how to repro
repro steps
- run with resetguisettings
- at Storage location dialog click
custom
- click back (so don't select a location) (can also repro when selecting the location)
- click on default
- then double-click on
custom
qmlguifreeze.webm
c2e73b2
to
7a8fb19
Compare
Couldn't repro on WSL Ubuntu 22.04. Kept on double/triple clicking and nothing froze... However was able to approximately repro the "double click" (#399) from my experience the first click opens the dialog and if it opens away from the |
ok please ignore my previous comment about freezing. |
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.
tACK 7a8fb19 on Ubuntu desktop
the double-click issue can be fixed later imo
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.
reACK 7a8fb19
Verified that the following issue has been fixed:
Another UX nit: when a user selects a custom datadir and accepts it, clicks again on custom datadir to open the file dialog, creates a different custom dir but cancels the dialog, it goes wrongly to tick/ check the default datadir. Perhaps we can correct that on a follow-up.
Looks like the mac os artifact issue has been addressed... Could you please confirm on your end when you get the chance, @GBKS? |
ACK 7a8fb19. Looks and feels great, just tested again on MacOS. Regarding the follow-ups in the description about double-clicking to save the path - why would users want to save the path in this specific situation? |
It was an idea in case a user wants to quickly copy the |
I like this thinking about secondary actions and I think we'll have a lot of opportunity to implement them throughout the app for more regular and power users (via tap-and-hold and right-click menus on mobile and desktop respectively). They are not always easy to discover, but repeat users should appreciate them. |
This pull request builds upon #392 and introduces enhancements to display the data directory information within the UI. This functionality encompasses both default and custom data directory paths, fulfilling the UI requirements for user-defined data directory selection initiated in #273.
Also the custom datadir is not persistent at the moment it will be once the back end wiring is added.
Ubuntu 22.04 Screenshots
Android Screenshots
As a potential follow-up enhancement, consider incorporating mechanisms for saving the data directory path. This could be achieved through:
Double-click functionality: Allow users to save the displayed path by simply double-clicking on it. This provides a convenient and intuitive method for desktop environments.
Dedicated button: For mobile use cases or scenarios where double-clicking might not be feasible, introduce a dedicated "Save Path" button. This ensures a clear and accessible action for users on various devices.