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

Remove unnecessary unpolish operation of the style, before polish the new style #12445

Merged
merged 2 commits into from
Feb 27, 2024

Conversation

JoergAtGithub
Copy link
Member

Using the profiler I noticed that ~11% of the main thread calculation time was consumed for updating push-button widgets (while one deck playing and the others CUE button slowly blinking).
I found out that removing that removing the style->unpolish() call before every style->polish() call has no visual effect, but impacts the CPU time significiantly.

I guess this code is either not needed on todays Qt anymore (I use the official Mixxx buildenv with Qt 5.15.8) or it's a platform specific behavior.

@ronso0
Copy link
Member

ronso0 commented Dec 17, 2023

Oh 11%..
Reverted to draft. Let's first confirm on all platforms, official builds and local ones, to not introduce another regression five to twelve.

@uklotzde
Copy link
Contributor

@uklotzde
Copy link
Contributor

uklotzde commented Dec 17, 2023

The Stackoverflow link should be replaced by the Wiki link.

@JoergAtGithub JoergAtGithub marked this pull request as ready for review January 20, 2024 20:55
@JoergAtGithub
Copy link
Member Author

This work is completed and ready for test and review

@ronso0
Copy link
Member

ronso0 commented Jan 20, 2024

All skins still looking good.

For reference: QStyleSheetStyle::polish(QWidget *w) in Qt 5.12.7

I'm still worried to merge it that late before the release.
@mixxxdj/developers could someone please test on macOS that skin styles are still good?
Especially those for native and custom widget properties:

  • hover: style of pushbuttons, e.g. in Deere
  • pressed: orange bg + black icon of pressed Loop In button in LateNight

@JoergAtGithub
Copy link
Member Author

Yes, I aggree, that this is too risky short before a release

@fwcd
Copy link
Member

fwcd commented Jan 20, 2024

Yeah, let's move any risky changes to 2.4.1. Both we and users will have a much easier time pinpointing regressions between 2.4.0 and 2.4.1 than in the large batch of changes between 2.3.x and 2.4.0.

(I'd argue that we should probably release more often in general. Running a dev/beta for too long will always risk having users "out of the loop" of changes that might only regress something in certain scenarios. I consider #3761 a good example of how platform-specific bugs can linger for years if there are too few users actually battle-testing the program)

@daschuer daschuer added this to the 2.4.1 milestone Feb 11, 2024
@JoergAtGithub
Copy link
Member Author

@ronso0 Can we merge this now? The longer we test it, the better.

@ronso0
Copy link
Member

ronso0 commented Feb 27, 2024

Sure, thanks for the reminder!

"The longer we test it, the better." also applies to #11526 btw 😉

@ronso0 ronso0 merged commit 62f3c82 into mixxxdj:2.4 Feb 27, 2024
13 checks passed
@ronso0
Copy link
Member

ronso0 commented Feb 27, 2024

Thank you!

@JoergAtGithub JoergAtGithub deleted the noStyleUnpolish branch February 27, 2024 00:23
@ronso0
Copy link
Member

ronso0 commented Feb 27, 2024

Oh no, didn't pay attention: This should have gone to main.
I doubt 2.4.1 gets sufficient testing before release, for example if we fix a somewhat severe bug and release soon after.

@JoergAtGithub
Copy link
Member Author

The outcome of the discussion above was, that it should go into 2.4.1 - wasn't it?

@ronso0
Copy link
Member

ronso0 commented Feb 27, 2024

Yes, but reconsidering I came to the conclusion I just posted.
Anyway, if there are issues then there will be 2.4.2 : )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants