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

fix: 150% and 200% zoom levels prevent the user from logging in #13689

Merged

Conversation

caybro
Copy link
Member

@caybro caybro commented Feb 22, 2024

What does the PR do

The long story:

  • since Qt treats the various scale factors in a multiplicative way (see https://www.qt.io/blog/2016/01/26/high-dpi-support-in-qt-5-6 for explanation) and there's no way to get the screen's baseline scale factor programatically, we also have to export QT_SCREEN_SCALE_FACTORS to something that's not equal to 0 or 1 to force the monitor scale factor to 100% and then compensate for it when exporting our own scale
    value using QT_SCALE_FACTOR
  • make the UI slider values go in 25% steps, allowing for more fine grained control; with 100% we fallback to the Qt's native handling of highdpi
  • raised the maximum to 300% since on highres displays, one wouldn't be able to go over the implicit maximum of 200% (due to the internal scaling being 2x)
  • scale our main window's minimum width/height so that we don't overflow the monitor's available space
  • modernize the ConfirmAppRestartModal to use StatusDialog
  • use the new Utils.restartApplication() when changing the UI language as well
  • remove some dead code

In the (very) long term, we should take a different approach of scaling our app independently of Qt, just taking the monitor Screen.devicePixelRatio into account, similar to what other apps like Telegram do

Fixes #13484

Affected areas

AppearanceView

Screenshot of functionality (including design for comparison)

  • I've checked the design and this PR matches it
Zaznam.obrazovky.z.2024-02-23.09-34-50.webm

@caybro caybro linked an issue Feb 22, 2024 that may be closed by this pull request
@status-im-auto
Copy link
Member

status-im-auto commented Feb 22, 2024

Jenkins Builds

Click to see older builds (6)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 4276b93 #1 2024-02-22 15:25:12 ~6 min tests/nim 📄log
✔️ 4276b93 #1 2024-02-22 15:26:38 ~7 min macos/aarch64 🍎dmg
✔️ 4276b93 #1 2024-02-22 15:29:33 ~10 min tests/ui 📄log
✔️ 4276b93 #1 2024-02-22 15:31:30 ~12 min macos/x86_64 🍎dmg
✔️ 4276b93 #1 2024-02-22 15:36:15 ~17 min linux/x86_64 📦tgz
✔️ 4276b93 #1 2024-02-22 15:53:19 ~34 min windows/x86_64 💿exe
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 2bb65c0 #2 2024-02-22 17:27:21 ~4 min macos/aarch64 🍎dmg
✔️ 2bb65c0 #2 2024-02-22 17:28:59 ~5 min tests/nim 📄log
✔️ 2bb65c0 #2 2024-02-22 17:30:48 ~7 min macos/x86_64 🍎dmg
✔️ 2bb65c0 #2 2024-02-22 17:33:09 ~9 min tests/ui 📄log
✔️ 2bb65c0 #2 2024-02-22 17:38:12 ~15 min linux/x86_64 📦tgz
✔️ 2bb65c0 #2 2024-02-22 17:49:43 ~26 min windows/x86_64 💿exe
✔️ 5f3ec5f #3 2024-02-27 09:12:40 ~4 min macos/aarch64 🍎dmg
✔️ 5f3ec5f #3 2024-02-27 09:14:38 ~6 min tests/nim 📄log
✔️ 5f3ec5f #3 2024-02-27 09:15:53 ~8 min macos/x86_64 🍎dmg
✔️ 5f3ec5f #3 2024-02-27 09:18:57 ~11 min tests/ui 📄log
✔️ 5f3ec5f #3 2024-02-27 09:23:21 ~15 min linux/x86_64 📦tgz
✔️ 5f3ec5f #3 2024-02-27 09:38:04 ~30 min windows/x86_64 💿exe

@jrainville
Copy link
Member

This is awesome. I just tested and it works perfect for me. 150% used to be disgustingly too big, but now it's pretty decent on my high def monitor. Thanks!

@caybro caybro force-pushed the 13484-150-and-200-zoom-levels-prevent-the-user-from-logging-in branch from 4276b93 to 2bb65c0 Compare February 22, 2024 17:22
@caybro caybro marked this pull request as ready for review February 23, 2024 08:30
@caybro
Copy link
Member Author

caybro commented Feb 23, 2024

This is awesome. I just tested and it works perfect for me. 150% used to be disgustingly too big, but now it's pretty decent on my high def monitor. Thanks!

Great to hear! Can you pls give it a quick try on Windows as well, if you can? :)

Copy link
Contributor

@alexjba alexjba left a comment

Choose a reason for hiding this comment

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

Nice investigation! Works well on Mac and Windows if the window is large enough. One remark: Would be nice to multiply the minimum window width/height with the zoom level in main.qml

@caybro
Copy link
Member Author

caybro commented Feb 23, 2024

Nice investigation! Works well on Mac and Windows if the window is large enough. One remark: Would be nice to multiply the minimum window width/height with the zoom level in main.qml

Yeah I'm doing that but now I realized I should rather do it with the scaleFactor that we set from UI, not the overall Screen.dpr

@caybro
Copy link
Member Author

caybro commented Feb 26, 2024

@jrainville pretty pls 🙏

@Michal-Status Michal-Status added this to the 2.28.0 Beta milestone Feb 26, 2024
Copy link
Member

@jrainville jrainville left a comment

Choose a reason for hiding this comment

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

Looks and works well! Just a questions about the warns

ui/app/AppLayouts/Profile/views/AppearanceView.qml Outdated Show resolved Hide resolved
Copy link

@Michal-Status Michal-Status left a comment

Choose a reason for hiding this comment

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

200% zoom level does not prevent from logging in on the resolution the bug was initially reported on. Switching zooms in 25% increments works well for all of the zoom levels implemented. However the issue still occurs on higher zoom levels. Combining higher zoom levels (especially newly implemented 300%) with a lower screen resolution and size causes similar issues as the original one.

- TLDR: we were scaling twice, resulting in ginourmous pixel values

The long story:
- since Qt treats the various scale factors in a multiplicative way (see
https://www.qt.io/blog/2016/01/26/high-dpi-support-in-qt-5-6 for
explanation) and there's no way to get the screen's baseline scale
factor programatically, we also have to export `QT_SCREEN_SCALE_FACTORS`
to something that's not equal to `0` or `1` to force the monitor scale
factor to `100%` and then compensate for it when exporting our own scale
value using `QT_SCALE_FACTOR`
- make the UI slider values go in `25%` steps, allowing for more fine
grained control; with `100%` we fallback to the Qt's native handling of
highdpi
- raised the maximum to `300%` since on highres displays, one wouldn't
be able to go over the implicit maximum of `200%` (due to the internal
scaling being 2x)
- scale our main window's minimum width/height so that we don't overflow
the monitor's available space
- modernize the `ConfirmAppRestartModal` to use `StatusDialog`
- use the new `Utils.restartApplication()` when changing the UI language
as well
- remove some dead code

In the (very) long term, we should take a different approach of scaling
our app independently of Qt, just taking the monitor
`Screen.devicePixelRatio` into account, similar to what other apps like
Telegram do

Fixes #13484
@caybro caybro force-pushed the 13484-150-and-200-zoom-levels-prevent-the-user-from-logging-in branch from 2bb65c0 to 5f3ec5f Compare February 27, 2024 09:07
@caybro caybro merged commit 630da7c into master Feb 27, 2024
8 checks passed
@caybro caybro deleted the 13484-150-and-200-zoom-levels-prevent-the-user-from-logging-in branch February 27, 2024 09:39
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.

150% and 200% zoom levels prevent the user from logging in
5 participants