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

perf: Speed Up Opening of Settings Dialog #4193

Merged
merged 6 commits into from Nov 27, 2022

Conversation

Nerixyz
Copy link
Contributor

@Nerixyz Nerixyz commented Nov 27, 2022

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

I noticed that opening the settings dialog is really slow.

Two things jump out:

  1. A lot of time is spent inside updateScale.
  2. Calling setWindowFlags results in style recalculations (Before, After).

I'm not sure if my fix to reduce calls to updateScale is correct. It would be nice if people could test this (e.g. with different zoom-levels and on high-dpi). If my fix turns out incorrect, I'd like to add a comment in the code as to why the change-callback must be run when creating the window (the second call to updateScale as added in d796517 to fix #1266).

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@kornes
Copy link
Contributor

kornes commented Nov 27, 2022

There is also this call

this->updateScale();

triggered a LOT by WM_SHOWWINDOW, at minimum it could be changed to check if nativeScale_ actually changed, but it could be also dropped from here i think, because WM_DPICHANGED handler is already calling updateScale()

another pr should properly fix #1266 without this double scalling fuckery :D

Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

Just changelog entry rewording, other than that the changes are solid 👍
thank you

CHANGELOG.md Outdated Show resolved Hide resolved
@Nerixyz
Copy link
Contributor Author

Nerixyz commented Nov 27, 2022

triggered a LOT by WM_SHOWWINDOW

I noticed that as well. I changed it to only call updateScale if the scale actually changed when this event is dispatched. This requires some further testing.

@Nerixyz
Copy link
Contributor Author

Nerixyz commented Nov 27, 2022

triggered a LOT by WM_SHOWWINDOW

I noticed that as well.

but it could be also dropped from here i think, because WM_DPICHANGED handler is already calling updateScale()

Probably, though, maybe WM_DPICHANGED doesn't get fired in some instances?

another pr should properly fix #1266 without this double scalling fuckery :D

True, I think there might be more low-hanging fruits.

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@kornes
Copy link
Contributor

kornes commented Nov 27, 2022

Probably, though, maybe WM_DPICHANGED doesn't get fired in some instances?

WM_DPICHANGED covers moving between screens and current screen change, so im 80% sure it could be removed from here, but i would leave it, once chatterino upgrades to qt6 it can be revisited with new dpi oriented apis

PR looks good to me 👍

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@pajlada pajlada merged commit a16342f into Chatterino:master Nov 27, 2022
@Nerixyz Nerixyz deleted the perf/settings-dialog branch November 27, 2022 19:19
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.

Tabs are too wide after changing zoom
3 participants