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 Custom Datadir #392

Merged
merged 5 commits into from Apr 8, 2024
Merged

Conversation

D33r-Gee
Copy link
Contributor

This pull request is a focused iteration of #390, intended to isolate and test the UI frontend elements. Backend functionality has been intentionally excluded to streamline the testing, review, and merge process.

Ubuntu Screenshots

Screenshot 2024-03-14 124000
Screenshot 2024-01-29 074759
Screenshot 2024-01-29 074734

Prerequisites

For testing this pull request, ensure you have the following Qt modules installed:

  • On Ubuntu 22.04:
sudo apt-get install qtdeclarative5-dev
sudo apt-get install qtquickcontrols2-5-dev
sudo apt-get install qml-module-qtquick-controls2
sudo apt install qml-module-qtquick-dialogs
  • For Android:
    • Make sure you delete the prior depends folder (i.e. depends/aarch64-linux-android) and rebuild them.

Implementation Details

This introduces FileDialog class for a user-friendly selection experience.

  • QML Backend
    • Updated options_model files to account for custom data directory configuration and placeholders for custom datadir wiring.
    • Added @johnny9 depends patches to allow static building
  • QML Frontend
    • Updated StorageLocations.qml to allow for custom data directory selection.

Follow up PR will add display of the custom datadir in StorageSettings.qml

Copy link
Contributor

@johnny9 johnny9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the README at https://github.com/bitcoin-core/gui-qml/tree/main/src/qml with the additional packages that need to be installed.

src/qml/components/StorageLocations.qml Outdated Show resolved Hide resolved
@D33r-Gee
Copy link
Contributor Author

Update the README at https://github.com/bitcoin-core/gui-qml/tree/main/src/qml with the additional packages that need to be installed.

Good call @johnny9! Just pushed a new commit with the updated doc.

Now it's pretty much equivalent (give and take) with @jarolrod's #273

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.

tACK 3f56460

It works as expected, left some code-related nits.

There's a specific case that I mentioned (3rd. bullet point) in the related PR #390, which I now discovered it's due to starting QT desktop and afterwards it's closed run the QML (on the same network env - e.g. -signet). I'll create an issue to discuss the best way to solve it.

image

nit: 3rd. commit (f3d251f) title length could be shorter, as in the mentioned above related PR , so it can be seen at first glance at the commits list without having to click on the ..., or during reviewing the "files changed" tab section also trying to select one from the commits list.

image

image

src/qml/components/StorageLocations.qml Outdated Show resolved Hide resolved
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.

double-clicking on Custom opens the storage selection in the background.
single-click works ok

openbackground.webm

649c2a5 you added qml-module-qtquick-dialogs, the other 3 qtdeclarative5-dev qtquickcontrols2-5-dev qml-module-qtquick-controls2 should be added to the instructions as well?

@D33r-Gee
Copy link
Contributor Author

D33r-Gee commented Apr 2, 2024

double-clicking on Custom opens the storage selection in the background. single-click works ok

Thanks @MarnixCroes, good catch... wasn't able to reproduce on WSL Ubuntu 22.04... Found a potential workaround by adding a timer, will implemented in a future PR since it's little out of scope for this one.

649c2a5 you added qml-module-qtquick-dialogs, the other 3 qtdeclarative5-dev qtquickcontrols2-5-dev qml-module-qtquick-controls2 should be added to the instructions as well?

those are actually found here in the qml/README.md:

sudo apt install qtdeclarative5-dev qtquickcontrols2-5-dev

and here:
sudo apt install qml-module-qtquick2 qml-module-qtquick-controls qml-module-qtquick-controls2 qml-module-qtquick-dialogs qml-module-qtquick-layouts qml-module-qtquick-window2 qml-module-qt-labs-settings

Copy link
Contributor

@johnny9 johnny9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 649c2a5

Tested static mobile and desktop builds as well as dynamic linux builds and the FileDialog triggers properly with no QML errors.

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.

tACK 649c2a5

I've managed to reproduce the issue reported by @MarnixCroes, on Ubuntu 22.04 it takes a 3 or 4 attempts, but eventually, it occurs. Tried to fix it using visible: true/ false but couldn't solve it, it would require more investigation and could be done on a follow-up.

nits (not blockers):

  • Since this is UI only changes, I'd leave commit ffd5201 out, it doesn't change anything in terms of user experience and if we find an issue there, we'd need to revert it (or fix it on top). So in StorageLocations.qml I'd comment the lines making those calls to the optionsModel adding another comment on top of each like "// TODO when model supports it" or similar.
  • I'd make the new custom datadir path selected from the file dialog visible in 2 places:
    • under the custom datadir field as it's shown in figma:

      image

    • somewhere in the "Storage settings" page but after onboarding process I think (this definitely as a follow-up) but this needs to be discussed with designers (#bitcoin-core-app channel on "Bitcoin Design" discord) because I don't see any reference in figma (unless I missed it)/ cc: @GBKS. It's important here to mention that would be good also to show here the -datadir passed as an argument to QT, which won't be persisted in Bitcoin-Qt-*.conf as the custom data dir, or even the default datadir if that's the case (eg on Ubuntu home/${user}/.bitcoin) so the user is aware at all times where the data is being taken/ saved at all times.

      Screenshot from 2024-04-05 16-51-25

@hebasto hebasto merged commit c3fb557 into bitcoin-core:main Apr 8, 2024
8 of 9 checks passed
@GBKS
Copy link
Contributor

GBKS commented Apr 9, 2024

Already merged, but I just tested on Android and MacOS and the UI works fine. Nice work on this, sounds like file picking was quite the heavy lift to get done.

I agree with the UI proposals up there and will prep respective designs.

@GBKS
Copy link
Contributor

GBKS commented Apr 11, 2024

I created a follow-up issue for the directory display in storage settings here. Would be great to get your input on the question I have in that issue.

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.

None yet

6 participants