Skip to content

Introducing the node settings views #203

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

Merged
merged 2 commits into from
Dec 14, 2022

Conversation

jarolrod
Copy link
Member

@jarolrod jarolrod commented Dec 10, 2022

This introduces the views that would allow for settings configuration after the user has finished onboarding and the node has started to run.

NodeRuner NodeSettings
Screen Shot 2022-12-10 at 12 58 07 AM Screen Shot 2022-12-10 at 12 58 12 AM

For several reasons this also entangles the About and Developer options and entangles them into their own independent SwipeView, which means that we only need to instantiate SettingsAbout. There isn't a use case for the Developer options to be shown independently.

Follow-ups:

  • The caret-right icon used within the settings rows is too large because I'm just using NavButton, the width and height should be adjusted to match closer to what the design file specifies
  • This uses a StackView to move through the settings pages but on linux, the animation between pages isn't how we'd want it to be; it's fine on macOS. The animation should match our SwipeViews. Note: I didn't use SwipeView because while you can set an index to go to, it flips through all previous pages to get there which is visually distracting.

Windows
Intel macOS
Apple Silicon macOS
ARM64 Android

@hebasto
Copy link
Member

hebasto commented Dec 10, 2022

Rebase needed.

@jarolrod
Copy link
Member Author

updated from e7763aa to 942a3b4

changes: rebased over master

ColumnLayout {
spacing: 0
width: parent.width
ColumnLayout {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like one of these ColumnLayouts can be removed

Copy link
Member Author

Choose a reason for hiding this comment

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

this allows for the table to expand but be a max width of 450, we use the same nested approach in the InformationPage control; open to better ways of accomplishing this

Copy link
Collaborator

@johnny9 johnny9 Dec 14, 2022

Choose a reason for hiding this comment

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

Can use width: Math.min(parent.width, 450)

@jarolrod
Copy link
Member Author

jarolrod commented Dec 14, 2022

updated from 942a3b4 to 06235ca (pr203.02 -> pr203.03, diff)

changes:

  • rebased over changes on base pr
  • addressed @johnny9 suggestions -1

jarolrod and others added 2 commits December 14, 2022 03:03
Currently, the parent SwipeView of the AboutOptions page needs to have
an id of introductions. We abuse the knowledge that in our codebase we
will have that available, but this limits when and where the
AboutOptions can be used. This entangles the About and Developer options
so that we can avoid this, and have these pages truly be reusable.

Co-Authored-By: Johnny Carlson <[email protected]>
@jarolrod
Copy link
Member Author

jarolrod commented Dec 14, 2022

updated from from 06235ca to 6037471 (pr203.03 -> pr203.04, diff)

changes: rebased over changes on master

ColumnLayout {
spacing: 0
anchors.fill: parent
ColumnLayout {
Copy link
Collaborator

@johnny9 johnny9 Dec 14, 2022

Choose a reason for hiding this comment

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

Here's another double ColumnLayout. I think we should reduce the layers here or use a different top level Item since using another Layout component isn't necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe we can leave it to a follow-up pr to address the nested columnlayouts throughout the code?

Copy link
Collaborator

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

Changes look good. Built and ran with not qml warnings/errors and views render as expected. Will address ColumnLayout comments in a follow up PR.

@hebasto hebasto merged commit b80cbe7 into bitcoin-core:main Dec 14, 2022
@jarolrod jarolrod deleted the node-settings-view branch December 15, 2022 15:29
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 8, 2025
Currently, the parent SwipeView of the AboutOptions page needs to have
an id of introductions. We abuse the knowledge that in our codebase we
will have that available, but this limits when and where the
AboutOptions can be used. This entangles the About and Developer options
so that we can avoid this, and have these pages truly be reusable.

Github-Pull: bitcoin-core#203
Rebased-From: 6ac40e8

Co-Authored-By: Johnny Carlson <[email protected]>
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 8, 2025
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 9, 2025
Currently, the parent SwipeView of the AboutOptions page needs to have
an id of introductions. We abuse the knowledge that in our codebase we
will have that available, but this limits when and where the
AboutOptions can be used. This entangles the About and Developer options
so that we can avoid this, and have these pages truly be reusable.

Github-Pull: bitcoin-core#203
Rebased-From: 6ac40e8

Co-Authored-By: Johnny Carlson <[email protected]>
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 9, 2025
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 9, 2025
Currently, the parent SwipeView of the AboutOptions page needs to have
an id of introductions. We abuse the knowledge that in our codebase we
will have that available, but this limits when and where the
AboutOptions can be used. This entangles the About and Developer options
so that we can avoid this, and have these pages truly be reusable.

Github-Pull: bitcoin-core#203
Rebased-From: 6ac40e8

Co-Authored-By: Johnny Carlson <[email protected]>
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 9, 2025
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.

3 participants