gui-qml icon indicating copy to clipboard operation
gui-qml copied to clipboard

UI Only Custom Datadir Display

Open D33r-Gee opened this issue 10 months ago • 23 comments

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

datadir_desktop

Android Screenshots

datadir_mobile_720

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.

D33r-Gee avatar Apr 11 '24 17:04 D33r-Gee

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?

D33r-Gee avatar Apr 15 '24 16:04 D33r-Gee

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

image

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

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

cc @GBKS

pablomartin4btc avatar Apr 15 '24 16:04 pablomartin4btc

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?

D33r-Gee avatar Apr 16 '24 17:04 D33r-Gee

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

johnny9 avatar Apr 17 '24 02:04 johnny9

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

image

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.

GBKS avatar Apr 17 '24 08:04 GBKS

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?

mouxdesign avatar Apr 17 '24 13:04 mouxdesign

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 avatar Apr 23 '24 20:04 pablomartin4btc

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

pablomartin4btc avatar Apr 23 '24 20:04 pablomartin4btc

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

image

GBKS avatar Apr 24 '24 09:04 GBKS

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:

image

(crossed 9GB in red as it's incorrect)

pablomartin4btc avatar Apr 24 '24 22:04 pablomartin4btc

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

GBKS avatar Apr 25 '24 06:04 GBKS

Rebased over main dfc0023 to 2fbb27a

D33r-Gee avatar May 01 '24 14:05 D33r-Gee

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

D33r-Gee avatar May 01 '24 20:05 D33r-Gee

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?

D33r-Gee avatar May 01 '24 21:05 D33r-Gee

Could you post a screenshot please?

during onboarding:

Screenshot from 2024-05-01 23-45-12

Screenshot from 2024-05-01 23-45-19

after onboarding, at settings Screenshot from 2024-05-01 23-45-28

MarnixCroes avatar May 01 '24 21:05 MarnixCroes

@MarnixCroes Thanks for the screenshots... I see what you mean... Looking into it

D33r-Gee avatar May 01 '24 21:05 D33r-Gee

Thanks @pablomartin4btc for testing it

I'm not sure about showing the default path when the user has selected the custom one but not a blocker.

I could go either way, however it feels consistent to show the default datadir regardless since if the user is going through onboarding there's a high chance it might be their first time installing Bitcoin Core, and knowing where the default data is store might be helpful. Any thoughts @GBKS?

RE: @MarnixCroes comment about displaying the default datadir instead of custom, it seems StorageSettings is not refreshing until the node launches so it's not registering the change from default to custom datadir, I believe that this get addressed with #284. Going to test to confirm on separate branch...

D33r-Gee avatar May 06 '24 14:05 D33r-Gee

with the latest commit 2a301e1 addressed @MarnixCroes comment about the datadir not displaying properly during onboarding. Turns out made the mistake of not using the setter function for the customDataDir bool.

Please let me know if it displays correctly for ya'll @pablomartin4btc @johnny9 @GBKS @MarnixCroes ?

D33r-Gee avatar May 06 '24 19:05 D33r-Gee

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

D33r-Gee avatar May 07 '24 17:05 D33r-Gee

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

image

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.

GBKS avatar May 08 '24 12:05 GBKS

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 avatar May 08 '24 16:05 D33r-Gee

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 avatar May 10 '24 15:05 D33r-Gee

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 avatar May 10 '24 16:05 D33r-Gee

rebased new head is ce6866a9adf408625f6506497f9e78b6b663cadf

D33r-Gee avatar May 16 '24 17:05 D33r-Gee

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?

D33r-Gee avatar May 20 '24 22:05 D33r-Gee

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

D33r-Gee avatar May 21 '24 18:05 D33r-Gee

FTR, can repro the double-click in background issue on latest commit. not sure why previously I couldn't repro https://github.com/bitcoin-core/gui-qml/pull/397#pullrequestreview-2034467887

MarnixCroes avatar May 22 '24 10:05 MarnixCroes

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

GBKS avatar May 22 '24 12:05 GBKS

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 avatar May 22 '24 15:05 D33r-Gee

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!

D33r-Gee avatar May 22 '24 15:05 D33r-Gee