Skip to content

Remove extra ColumnLayouts#210

Merged
hebasto merged 1 commit into
bitcoin-core:mainfrom
johnny9:column-layouts
Dec 17, 2022
Merged

Remove extra ColumnLayouts#210
hebasto merged 1 commit into
bitcoin-core:mainfrom
johnny9:column-layouts

Conversation

@johnny9
Copy link
Copy Markdown
Collaborator

@johnny9 johnny9 commented Dec 14, 2022

Follow up to #203

Windows
Intel macOS
Apple Silicon macOS
ARM64 Android

Comment thread src/qml/pages/node/NodeSettings.qml Outdated
property alias navMiddleDetail: navbar.middleDetail
property alias navRightDetail: navbar.rightDetail
background: null
implicitWidth: 490
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is required due to an oddity with the StackView initial state. It will be ignored with the anchor properties.

Comment thread src/qml/pages/node/NodeRunner.qml Outdated
Layout.alignment: Qt.AlignCenter
}
detailItem: StorageLocations {
width: Math.min(parent.width, 450)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

there's an intricate issue with this where this will start off fine being a maximum of 450 px width, but if you resize the window to be a smaller width, then resize to be larger, the width of the storage location OptionButtons will become larger than 450px. I assume this would be rare to run into, but the code has an issue (albeit rare) that isn't present with the code on master:
settings-to-long

OnboardingStorageAmount.qml is immune to this because it keeps the nested ColumnLayout method since it is actually appropriate there.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah i need to rework this. I was thinking about these pieces wrong. There is a conflict with the InformationPage column widths.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this issue is fixed now as of f9f46e7, thanks!

@johnny9 johnny9 force-pushed the column-layouts branch 2 times, most recently from 0e2e651 to f9f46e7 Compare December 15, 2022 20:37
@johnny9
Copy link
Copy Markdown
Collaborator Author

johnny9 commented Dec 15, 2022

Update from 0e2e651 to f9f46e7

  • Potential maximum width solution for issue @jarolrod pointed out

Comment thread src/qml/pages/node/NodeSettings.qml Outdated
width: Math.min(parent.width, 490)
anchors.leftMargin: 20
anchors.rightMargin: 20
anchors.topMargin: 30
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the top margin for the columnlayout here doesn't seem to draw the same, PR on left, master on right:
different-margins

Also in general, it appears that the left and right margins are different throughout the views now, but will confirm on macos.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

And the PR NodeSettings isn't max 450 here

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Great catch. I now see Page has its own padding property and is ignoring my anchor values. https://doc.qt.io/qt-6/qml-qtquick-controls2-control.html#padding-prop.

Copy link
Copy Markdown
Collaborator Author

@johnny9 johnny9 Dec 15, 2022

Choose a reason for hiding this comment

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

those padding values apply to just the "contentItem" for Pages

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Ill go through a side-by-side with master as well, just need to setup a second view to compile from

@johnny9
Copy link
Copy Markdown
Collaborator Author

johnny9 commented Dec 15, 2022

Update from f9f46e7 to 4dc32c4:

  • Fix NodeSettings padding/margins by setting the Page padding values instead of using anchors.

@johnny9
Copy link
Copy Markdown
Collaborator Author

johnny9 commented Dec 15, 2022

Master (left) / PR (right)
Screenshot from 2022-12-15 18-02-32

Screenshot from 2022-12-15 18-02-43

Screenshot from 2022-12-15 18-02-52
Screenshot from 2022-12-15 18-03-05
Screenshot from 2022-12-15 18-03-11
Screenshot from 2022-12-15 18-03-19
Screenshot from 2022-12-15 18-03-31
Screenshot from 2022-12-15 18-03-38
Screenshot from 2022-12-15 18-03-52
Screenshot from 2022-12-15 18-04-03
Screenshot from 2022-12-15 18-04-13
Screenshot from 2022-12-15 18-04-24
Screenshot from 2022-12-15 18-04-33

Copy link
Copy Markdown
Contributor

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

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

tACK 4dc32c4

Changes are good and there is no visual difference between master and pr. Tested on macOS and Linux desktop as well as Android on a phone and tablet.

@hebasto hebasto merged commit 19b3a9d into bitcoin-core:main Dec 17, 2022
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
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 9, 2025
tx-signer450 added a commit to tx-signer450/gui-qml that referenced this pull request Oct 20, 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