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

Set default zoom of webview based on DPI. #1080

Merged
merged 2 commits into from Mar 13, 2019
Merged

Conversation

yiding
Copy link
Contributor

@yiding yiding commented Feb 9, 2019

Control zoom factor directly rather than using hardcoded zoom levels.

In order to ensure that the webview starts out scaled appropriately to the
current font DPI, we set default zoom level based on current DPI, e.g.:

96 DPI - 100% zoom
120 DPI - 125% zoom

Since this will allow zoom levels beyond the predefined set, I also change the
logic for zooming to simply add or remove 10% to the current scaling.

Note that if we want to still maintain the predefined zoom levels, I can rewrite this to multiply the predefined zoom levels by the zoom level derived from the current DPI.

@yiding yiding requested a review from trollixx as a code owner February 9, 2019 19:21
@trollixx
Copy link
Member

trollixx commented Feb 9, 2019

Thank you for the contribution, very interesting! Does this work better on High-DPI screens? What about moving window between screens with different DPI or scaling factors?

The list of zoom levels is inspired by what Firefox and Chrome do. Also maintaining specific zoom values should make it easier to save and restore sessions in the future.

@yiding
Copy link
Contributor Author

yiding commented Feb 9, 2019

Does this work better on High-DPI screens?

This works better for high dpi screens where the user is using font dpi scaling (vs full screen scaling). Font DPI scaling usually looks better than full screen scaling when scaling by fractional amounts. For full screen scaling (e.g. scaling by 200% using the display control panel), this shouldn't change anything. I'm only talking about in the context of windows, I'm not sure whether windows use DPI based scaling or not (I should probably check).

What about moving window between screens with different DPI or scaling factors?

I'm not familiar with how QT surfaces this information, and I've never had much hope that this works correctly on linux. I think this is somewhat orthogonal to this diff though.

The list of zoom levels is inspired by what Firefox and Chrome do. Also maintaining specific zoom values should make it easier to save and restore sessions in the future.

Let me do that then.

@yiding
Copy link
Contributor Author

yiding commented Feb 14, 2019

Alright I've updated this to apply the DPI based scaling with the zoom-level style scaling. Seems to behave sensibly on linux in KDE5 and Gnome3 when font DPI scaling is used, and when screen-scaling is used.

@trollixx
Copy link
Member

@yiding thanks, I'll review and test over the weekend.

Copy link
Member

@trollixx trollixx left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, caught a bad cold. A few minor remarks below.

src/libs/browser/webview.h Show resolved Hide resolved
src/libs/browser/webview.cpp Outdated Show resolved Hide resolved
src/libs/browser/webview.cpp Outdated Show resolved Hide resolved
trollixx
trollixx previously approved these changes Mar 13, 2019
@trollixx
Copy link
Member

@yiding could you rebase on top of master?

Also change `defaultZoomLevel()`'s `const int &` return type to just `int`.

Test Plan:
On Linux:
In KDE, forced DPI to 96 and 120. Restarting the app shows initial
webview scaled to 1x and 1.25x respectively.
In Gnome 3, use Tweaks to set font scaling to 1.25, restart the app.
@trollixx trollixx merged commit b106938 into zealdocs:master Mar 13, 2019
@trollixx
Copy link
Member

Thank you!

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

Successfully merging this pull request may close these issues.

None yet

2 participants