Skip to content

make GUIUtil::ThemedLabel UI form friendly#358

Closed
jarolrod wants to merge 1 commit into
bitcoin-core:masterfrom
jarolrod:themedlabel-forms
Closed

make GUIUtil::ThemedLabel UI form friendly#358
jarolrod wants to merge 1 commit into
bitcoin-core:masterfrom
jarolrod:themedlabel-forms

Conversation

@jarolrod
Copy link
Copy Markdown
Contributor

@jarolrod jarolrod commented Jun 7, 2021

This allows the GUIUtil::ThemedLabel class able to be used in UI form files. This is accomplished by introducing a form file friendly constructor which only takes in a QWidget* parent.

You cannot pass in the platform_style when initializing a GUIUtil::ThemedLabel in a UI form file. This introduces a setPlatformStyle function to be able to pass in the platform_style at a later point.

Additionally; because we are not initializing the class with a platform_style, we need to implement a check in updateThemedPixmap so that it only executes when the m_platform_style has been set. A segfault would occur without this conditional as a ChangeEvent occurs on application start

This allows this class to be used in the form file for #330 for an elegant solution.

This makes the GUIUtil::ThemedLabel class able to be used in UI form
files. This is accomplished by introducing a form file friendly
constructor which only takes in a `QWidget* parent`.

You cannot pass in the `platform_style` when initializing a
GUIUtil::ThemedLabel in a UI form file. This introduces a
`setPlatformStyle` function to be able to pass in the `platform_style`
at a later point.

Additionally; because we are not initializing the class with a
`platform_style`, we need to implement a check in `updateThemedPixmap`
to only occur when the `m_platform_style` has been set. A segfault would
occur without this conditional as a `ChangeEvent` occurs on application
start
@hebasto hebasto added the UI All about "look and feel" label Jun 7, 2021
Comment thread src/qt/guiutil.h

private:
const PlatformStyle* m_platform_style;
const PlatformStyle* m_platform_style = nullptr;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

{nullptr}

@hebasto hebasto changed the title qt: make GUIUtil::ThemedLabel UI form friendly make GUIUtil::ThemedLabel UI form friendly Jun 12, 2021
Copy link
Copy Markdown
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

@jarolrod

I think we should adopt your suggestion on IRC to move the ThemedLabel into its own module for the following reasons:

Also, it seams reasonable to me to drop this pr, and review both commits in #330, because without actual usage of a promoted widget in a *.ui file it is not easy to test it.

Comment thread src/qt/guiutil.cpp
Comment on lines +797 to +800
{
QLabel{parent};
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
{
QLabel{parent};
}
: QLabel{parent} {}

Comment thread src/qt/guiutil.cpp
Comment on lines +819 to 820
}
void ThemedLabel::changeEvent(QEvent* e)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

style nit: Add an empty line as a separator?

@jarolrod
Copy link
Copy Markdown
Contributor Author

jarolrod commented Jul 5, 2021

This was meant to be a part of #330. A simpler approach was chosen for that PR and this is no longer worth the effort as the issue has been fixed.

Note: If there is ever a reason to make the ThemedLabel class UI friendly, the approach should be to do so in its own class file; moving it away from GUIUtil

@jarolrod jarolrod closed this Jul 5, 2021
@bitcoin-core bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

UI All about "look and feel"

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants