UI Only Custom Datadir Display #397

D33r-Gee · 2024-04-11T17:14:44Z

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.

johnny9

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

src/qml/controls/OptionButton.qml

src/qml/models/options_model.cpp

pablomartin4btc

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

src/qml/components/StorageLocations.qml

D33r-Gee · 2024-04-15T16:16:05Z

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

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?

pablomartin4btc · 2024-04-15T16:43:57Z

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

To be clear we don't want to display the default datadir path here, correct?

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)?

However do we want to display the default path in StorageSettings.qml?

I think so, whatever it is set default or custom.

cc @GBKS

D33r-Gee · 2024-04-16T17:18:45Z

To be clear we don't want to display the default datadir path here, correct?

As of figma, and the web prototype (link above) no, but it won't hurt to show it, I'm not sure.

@GBKS and @johnny9 what your thoughts regarding the above?

johnny9 · 2024-04-17T02:05:23Z

To be clear we don't want to display the default datadir path here, correct?

As of figma, and the web prototype (link above) no, but it won't hurt to show it, I'm not sure.

@GBKS and @johnny9 what your thoughts regarding the above?

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

GBKS · 2024-04-17T08:45:53Z

Just tested on desktop (Mac) and mobile (Android).

I agree with Johnny's rationale about ditching the Recommended label.

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.

mouxdesign · 2024-04-17T13:54:29Z

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?

pablomartin4btc

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?
- bitcoin-qt ("Reduce storage" looks wrong)
- bitcoin-qt signet ("Store all data" looks wrong)

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

pablomartin4btc · 2024-04-23T20:41:07Z

Just tested on desktop (Mac) and mobile (Android).

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

I agree with Johnny's rationale about ditching the Recommended label.

Ok, please check the next page when user has to select the amount of space to store, there's also a "Recommended" highlighted label.

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

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.

pablomartin4btc · 2024-04-23T20:44:47Z

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?

I think that should be after accepting (clicking on next) the following page "Reduce store"/ "Store all data" (before starting the initial download sync).

GBKS · 2024-04-24T09:00:21Z

How about one of these two options for the error message?

pablomartin4btc · 2024-04-24T22:35:37Z

How about one of these two options for the error message?

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)

GBKS · 2024-04-25T06:46:08Z

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

D33r-Gee · 2024-05-01T14:32:56Z

Rebased over main dfc0023 to 2fbb27a

D33r-Gee · 2024-05-01T20:32:13Z

@GBKS and @pablomartin4btc Thanks for the feedback regarding the storage amount and the error messaging.
those will be addressed in a different PR since this is only for datadir display

Meanwhile please me know your thoughts on the latest update 8e8aadf?

MarnixCroes

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

D33r-Gee · 2024-05-01T21:18:47Z

8e8aadf I cannot repro the double-click issue anymore #399 friendly ping @pablomartin4btc

That's great to hear...

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

Could you post a screenshot please?

pablomartin4btc

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.

MarnixCroes · 2024-05-01T21:46:55Z

Could you post a screenshot please?

during onboarding:

after onboarding, at settings

D33r-Gee · 2024-05-07T17:18:26Z

Thanks @pablomartin4btc for testing and reviewing

I noticed another bug which is: if the user clicks on custom...

addressed this with the latest push from 2a301e1 to 61e775b

Now when a user cancels after having picked a custom datadir it reverts back to default

GBKS · 2024-05-08T12:52:04Z

Just tested the latest build, which I believe is 61e775b , on MacOS and Android.

Nice to see this come together. Just have some small comments:

Tapping anywhere in the directory option should activate it. This does not seem to work when tapping the light grey directory display box inside the item. Does it maybe intercept the click/tap event and prevent it from propagating to its parent?
On the settings page, the directory name seems to have the wrong color and too much spacing to the option title above it

Regarding the point about consistency for showing the directory box, I'd show it if it has a value. The default one can always be visible. And if the user has set one for the custom option, it can also be shown independent of whether that option is still selected or not. And if the user goes to the directory picker and cancels out of it, the previously picked option can remain active. That seems intuitive to me.

Quite the long thread, hope I didn't miss anything. Maybe good to address things like the error message in a separate PR to keep things manageable.

D33r-Gee · 2024-05-08T16:18:44Z

Thanks @GBKS for testing and reviewing...

This:

Tapping anywhere in the directory option should activate it. This does not seem to work when tapping the light grey directory display box inside the item. Does it maybe intercept the click/tap event and prevent it from propagating to its parent?

Is addressed with the latest push b941932

On the settings page, the directory name seems to have the wrong color

what would be the correct color?

And if the user goes to the directory picker and cancels out of it, the previously picked option can remain active. That seems intuitive to me.

this use case may be problematic since if the user cancels it currently reverts back to default, are you suggesting that if there was a previously selected folder then the custom option should remain selected even after the user cancels a secondary attempt at setting the custom datadir?

D33r-Gee · 2024-05-10T15:55:32Z

with the latest push from b941932 to 0c46cae addressed @GBKS comment regarding the proper color for datadir display, by adding a new state ("DIRECTORY") in ValueInput.qml

Next is addressing this:

And if the user goes to the directory picker and cancels out of it, the previously picked option can remain active. That seems intuitive to me.

D33r-Gee · 2024-05-10T16:14:23Z

new commit 855cea5 addressing this use case

And if the user goes to the directory picker and cancels out of it, the previously picked option can remain active. That seems intuitive to me.

Now when the user has picked/accepted a custom datadir and then decides to change it but cancels the selection it stays with the custom datadir previously selected unless they click on the default option

D33r-Gee · 2024-05-16T17:38:38Z

rebased new head is ce6866a

johnny9

Don't overload ValueInput for non input items. Just use CoreText or Label

src/qml/models/options_model.h

src/qml/models/options_model.cpp

src/qml/components/StorageSettings.qml

src/qml/controls/ValueInput.qml

D33r-Gee · 2024-05-20T22:28:55Z

Updated from ce6866a to 2e2fe79

Main changes were implementing @johnny9 and @pablomartin4btc feedback...

RE: customDataDir bool, decided to go with a dynamic setting of dataDir since the bool options was giving me grief...

Please let me know your thoughts?

johnny9

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/");"

D33r-Gee · 2024-05-21T18:06:20Z

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/");"

Thanks for discovering this... Found the cause of the problem, I mistakenly left the added const to the getter function. I removed them and now it builds properly for both Android and Linux

johnny9

ACK 7341bdf

MarnixCroes

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

MarnixCroes · 2024-05-22T10:42:43Z

FTR, can repro the double-click in background issue on latest commit. not sure why previously I couldn't repro #397 (review)

GBKS · 2024-05-22T12:38:51Z

The MacOS workflow runs seem to fail for this PR (also for 401). Would it be possible to fix those to simplify testing?

D33r-Gee · 2024-05-22T15:12:50Z

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

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.

D33r-Gee · 2024-05-22T15:16:23Z

The MacOS workflow runs seem to fail for this PR (also for 401). Would it be possible to fix those to simplify testing?

Hmmm... well that's problematic... will discuss with the other devs and see what can be done... Thanks for pointing it out!

pablomartin4btc

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.

pablomartin4btc · 2024-05-23T14:31:19Z

src/qml/models/options_model.cpp

@@ -156,11 +158,46 @@ QUrl OptionsQmlModel::getDefaultDataDirectory()
    return QUrl::fromLocalFile(path);
 }

-void OptionsQmlModel::setCustomDataDirArgs(QString path)
+bool OptionsQmlModel::setCustomDataDirArgs(QString path)


what the Args suffix in setCustomDataDirArgs stands for? Also because we have the option -datadir passed from the command line.

The Args suffix will come in play when the actual backend is connected since it will modify the gArgs so that the new datadir is set properly.

pablomartin4btc · 2024-05-23T14:41:50Z

src/qml/models/options_model.cpp

+{
+    if (new_data_dir != m_dataDir) {
+        m_dataDir = new_data_dir;
+        if (!getCustomDataDirString().isEmpty() && (new_data_dir != getDefaultDataDirString())) {


nit: why not having something like proposed here? It'd be easier to follow I think.

Suggested change

if (!getCustomDataDirString().isEmpty() && (new_data_dir != getDefaultDataDirString())) {

if (!isCustomDataDirSet) {

Thanks for your suggestion... after testing it became clear that not depending on a new bool would be the way to go. And so implemented yours and Johnny9 suggestions as part of a new dataDir variable.

pablomartin4btc · 2024-05-23T15:39:53Z

Regarding the point about consistency for showing the directory box, I'd show it if it has a value. The default one can always be visible. And if the user has set one for the custom option, it can also be shown independent of whether that option is still selected or not. And if the user goes to the directory picker and cancels out of it, the previously picked option can remain active. That seems intuitive to me.

@GBKS: Agreed. Just found another case (ref.: "Another UX nit").

D33r-Gee · 2024-05-27T20:08:48Z

c2e73b2 rebased over main

D33r-Gee mentioned this pull request Apr 11, 2024

Add data directory option to storage settings BitcoinDesign/Bitcoin-Core-App#102

Open

johnny9 suggested changes Apr 12, 2024

View reviewed changes

src/qml/controls/OptionButton.qml Outdated Show resolved Hide resolved

src/qml/models/options_model.cpp Outdated Show resolved Hide resolved

src/qml/models/options_model.cpp Outdated Show resolved Hide resolved

src/qml/models/options_model.cpp Outdated Show resolved Hide resolved

pablomartin4btc reviewed Apr 14, 2024

View reviewed changes

src/qml/components/StorageLocations.qml Outdated Show resolved Hide resolved

src/qml/components/StorageLocations.qml Show resolved Hide resolved

src/qml/components/StorageLocations.qml Show resolved Hide resolved

src/qml/components/StorageLocations.qml Show resolved Hide resolved

pablomartin4btc mentioned this pull request Apr 15, 2024

Bugfix custom datadir doubleClick - Follow-up #392 #399

Open

D33r-Gee force-pushed the qml-custom-datadir-display branch 2 times, most recently from 36467ea to dfc0023 Compare April 16, 2024 20:06

D33r-Gee requested review from pablomartin4btc and johnny9 April 16, 2024 20:09

GBKS mentioned this pull request Apr 17, 2024

Bitcoin Core App Design Call #84 BitcoinDesign/Meta#661

Closed

pablomartin4btc reviewed Apr 23, 2024

View reviewed changes

D33r-Gee force-pushed the qml-custom-datadir-display branch from dfc0023 to 2fbb27a Compare May 1, 2024 14:31

D33r-Gee force-pushed the qml-custom-datadir-display branch from 2fbb27a to 8e8aadf Compare May 1, 2024 17:45

D33r-Gee requested a review from pablomartin4btc May 1, 2024 20:32

MarnixCroes reviewed May 1, 2024

View reviewed changes

pablomartin4btc reviewed May 1, 2024

View reviewed changes

D33r-Gee force-pushed the qml-custom-datadir-display branch from 2a301e1 to 61e775b Compare May 7, 2024 17:15

D33r-Gee force-pushed the qml-custom-datadir-display branch from 61e775b to b941932 Compare May 8, 2024 16:05

D33r-Gee force-pushed the qml-custom-datadir-display branch from b941932 to 0c46cae Compare May 10, 2024 15:51

D33r-Gee force-pushed the qml-custom-datadir-display branch from 0c46cae to 855cea5 Compare May 10, 2024 16:08

D33r-Gee force-pushed the qml-custom-datadir-display branch from 855cea5 to ce6866a Compare May 16, 2024 17:29

johnny9 reviewed May 17, 2024

View reviewed changes

D33r-Gee force-pushed the qml-custom-datadir-display branch from ce6866a to 2e2fe79 Compare May 20, 2024 22:24

johnny9 approved these changes May 21, 2024

View reviewed changes

johnny9 suggested changes May 21, 2024

View reviewed changes

D33r-Gee force-pushed the qml-custom-datadir-display branch from 2e2fe79 to 7341bdf Compare May 21, 2024 18:03

johnny9 approved these changes May 22, 2024

View reviewed changes

MarnixCroes approved these changes May 22, 2024

View reviewed changes

pablomartin4btc approved these changes May 23, 2024

View reviewed changes

D33r-Gee added 2 commits May 27, 2024 10:59

qml: added getting custom datadir for display

b648cbb

qml: UI only display datadir functionality

c2e73b2

D33r-Gee force-pushed the qml-custom-datadir-display branch from 2a56d6b to c2e73b2 Compare May 27, 2024 18:01

	if (!getCustomDataDirString().isEmpty() && (new_data_dir != getDefaultDataDirString())) {
	if (!isCustomDataDirSet) {

UI Only Custom Datadir Display #397

Are you sure you want to change the base?

UI Only Custom Datadir Display #397

Conversation

D33r-Gee commented Apr 11, 2024 • edited

johnny9 left a comment

Choose a reason for hiding this comment

pablomartin4btc left a comment

Choose a reason for hiding this comment

D33r-Gee commented Apr 15, 2024

pablomartin4btc commented Apr 15, 2024

D33r-Gee commented Apr 16, 2024

johnny9 commented Apr 17, 2024

GBKS commented Apr 17, 2024

mouxdesign commented Apr 17, 2024

pablomartin4btc left a comment

Choose a reason for hiding this comment

pablomartin4btc commented Apr 23, 2024

pablomartin4btc commented Apr 23, 2024

GBKS commented Apr 24, 2024

pablomartin4btc commented Apr 24, 2024

GBKS commented Apr 25, 2024

D33r-Gee commented May 1, 2024

D33r-Gee commented May 1, 2024 • edited

MarnixCroes left a comment

Choose a reason for hiding this comment

D33r-Gee commented May 1, 2024 • edited

pablomartin4btc left a comment

Choose a reason for hiding this comment

MarnixCroes commented May 1, 2024

D33r-Gee commented May 7, 2024

GBKS commented May 8, 2024

D33r-Gee commented May 8, 2024

D33r-Gee commented May 10, 2024 • edited

D33r-Gee commented May 10, 2024

D33r-Gee commented May 16, 2024

johnny9 left a comment

Choose a reason for hiding this comment

D33r-Gee commented May 20, 2024

johnny9 left a comment • edited

Choose a reason for hiding this comment

D33r-Gee commented May 21, 2024

johnny9 left a comment

Choose a reason for hiding this comment

MarnixCroes left a comment

Choose a reason for hiding this comment

MarnixCroes commented May 22, 2024

GBKS commented May 22, 2024

D33r-Gee commented May 22, 2024

D33r-Gee commented May 22, 2024

pablomartin4btc left a comment • edited

Choose a reason for hiding this comment

pablomartin4btc May 23, 2024

Choose a reason for hiding this comment

D33r-Gee May 27, 2024

Choose a reason for hiding this comment

pablomartin4btc May 23, 2024

Choose a reason for hiding this comment

D33r-Gee May 27, 2024

Choose a reason for hiding this comment

pablomartin4btc commented May 23, 2024

D33r-Gee commented May 27, 2024

D33r-Gee commented Apr 11, 2024 •

edited

D33r-Gee commented May 1, 2024 •

edited

D33r-Gee commented May 1, 2024 •

edited

D33r-Gee commented May 10, 2024 •

edited

johnny9 left a comment •

edited

pablomartin4btc left a comment •

edited