mixxx icon indicating copy to clipboard operation
mixxx copied to clipboard

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

Open JoergAtGithub opened this issue 2 years ago • 7 comments

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.

JoergAtGithub avatar Dec 17 '23 13:12 JoergAtGithub

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.

ronso0 avatar Dec 17 '23 14:12 ronso0

Still valid according to the Wiki: https://wiki.qt.io/Technical_FAQ#How_can_my_stylesheet_account_for_custom_properties.3F

uklotzde avatar Dec 17 '23 14:12 uklotzde

The Stackoverflow link should be replaced by the Wiki link.

uklotzde avatar Dec 17 '23 14:12 uklotzde

This work is completed and ready for test and review

JoergAtGithub avatar Jan 20 '24 20:01 JoergAtGithub

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

ronso0 avatar Jan 20 '24 22:01 ronso0

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

JoergAtGithub avatar Jan 20 '24 23:01 JoergAtGithub

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)

fwcd avatar Jan 20 '24 23:01 fwcd

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

JoergAtGithub avatar Feb 26 '24 23:02 JoergAtGithub

Sure, thanks for the reminder!

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

ronso0 avatar Feb 27 '24 00:02 ronso0

Thank you!

ronso0 avatar Feb 27 '24 00:02 ronso0

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.

ronso0 avatar Feb 27 '24 14:02 ronso0

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

JoergAtGithub avatar Feb 27 '24 14:02 JoergAtGithub

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

ronso0 avatar Feb 27 '24 14:02 ronso0