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

Qt5 style fixes #915

Closed
wants to merge 1 commit into from
Closed

Qt5 style fixes #915

wants to merge 1 commit into from

Conversation

rodlie
Copy link
Contributor

@rodlie rodlie commented Sep 5, 2023

Fixes for Qt5 style regressions.

Only tested on Ubuntu with Qt 5.15.3.

Screenshots

Natron 2.5.0 release (Qt4)

Screenshot from 2023-09-05 19-09-20

Natron RB-2.5 (Qt 5.15.3)

Screenshot from 2023-09-05 19-04-05

After pull request

Screenshot from 2023-09-05 19-05-56

Fixes for Qt5 style regressions.
Copy link
Member

@devernay devernay left a comment

Choose a reason for hiding this comment

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

I prefer a cluttered stylesheet to cluttered code (and please add comments in both for things that are hacks, for the day we clean these up)

@@ -98,6 +98,7 @@ AboutWindow::AboutWindow(QWidget* parent)
///filling tabs now

_aboutText = new QTextBrowser(_tabWidget);
_aboutText->setObjectName( QString::fromUtf8("TextBrowser") );
Copy link
Member

Choose a reason for hiding this comment

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

what is the objectname in Qt5? Doesn't it make more sense to use that objectname in the stylesheet as an alternative, rather than cluttering (a tiny bit) the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason QTextBrowser defaults to black text in some widgets, but not all. So adding a objectname makes it easy to style the widgets that needs fixes.

Can of course figure out a better solution, just launched Natron and noticed the issue. Will close and submit a new PR when changed.

Copy link
Member

Choose a reason for hiding this comment

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

can you maybe print some properties of _aboutText to see why it didn't catch the right stylesheet entry? I know @acolwell cares a lot about code clutter ;-)
If that's the only way to go, then let's do it, but in this case I would ask for @acolwell's review as well

Copy link
Contributor Author

@rodlie rodlie Sep 5, 2023

Choose a reason for hiding this comment

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

Agree to avoid code clutter, will find a qss-only fix.

@rodlie rodlie closed this Sep 5, 2023
@rodlie rodlie mentioned this pull request Sep 5, 2023
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.

2 participants