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

Zoom scaling #1586

Closed
wants to merge 2 commits into from
Closed

Zoom scaling #1586

wants to merge 2 commits into from

Conversation

samuelallan72
Copy link

@samuelallan72 samuelallan72 commented Jun 16, 2016

This adds a scale factor based on the screen dpi to scale the zoom size (make it work nicely on high and low dpi screens basically). Reference issue #1585.


This change is Reviewable

@samuelallan72
Copy link
Author

Hmm seems it messes up all the tests relating to zoom and hints...

@The-Compiler
Copy link
Member

So I see various different issues here:

  • The zoom values change on Travis with Xvfb, which I expect to be 96dpi?
  • There are various places (sessions, hints, etc.) where the zoom factor is used but which are not adjusted right now.

As for the second point, if you agree I'd propose postponing this until after the refactoring I do in the qtwebengine branch, as then there should only be one location to change this - what do you think?

@samuelallan72
Copy link
Author

The zoom scale should equal 1 at 96dpi, which wouldn't affect the tests in that case.

Yep, agree - that makes sense.

@The-Compiler
Copy link
Member

The-Compiler commented Sep 21, 2016

The refactoring for QtWebEngine is mostly complete now, and all zooming happens via the AbstractZoom class now.

However, I think QtWebEngine will already adjust the zoom on highdpi, or at least it will with a qApp.setAttribute(Qt.AA_EnableHighDpiScaling) added to app.py - so it might only be necessary to do the adjustments in WebKitZoom.

@swalladge would you like to pick this up again?

@samuelallan72
Copy link
Author

With the current git master, and QT_AUTO_SCREEN_SCALE_FACTOR=1 set in the environment, it seems to work perfectly well now. 100% zoom is normal size on my highdpi screens. It also seems consistent with both backends.

I don't think any zoom scaling is needed any more. :P Maybe the use of that environment variable should be put in the docs or faq if it isn't already?

@The-Compiler
Copy link
Member

Even with the QtWebKit backend, right? Can you try adding qApp.setAttribute(Qt.AA_EnableHighDpiScaling) to app.py and see if it works without an environment variable then?

@samuelallan72
Copy link
Author

Yep, even with QtWebKit. I just tried adding that line (to line 81 of app.py), but it didn't appear to change anything - with either backend or with/without the env variable.

@The-Compiler The-Compiler mentioned this pull request Sep 26, 2016
@The-Compiler
Copy link
Member

The-Compiler commented Sep 26, 2016

I referenced this from #1585 for now, and will take a closer look once I get my new laptop with a HighDPI screen. Thanks for the contribution though!

@The-Compiler
Copy link
Member

So I just enabled Qt::AA_EnableHighDpiScaling in git master - I can't really test it yet (my highdpi laptop will finally arrive this or next week though, I hope...), but I'd love some feedback!

This should be equivalent to setting QT_AUTO_SCREEN_SCALE_FACTOR=1 FWIW and should at least scale stuff up with QtWebEngie.

@samuelallan72
Copy link
Author

To be honest, I don't notice any difference, apart from the fact that it scales nicely without QT_AUTO_SCREEN_SCALE_FACTOR set now.

I find the scaling on my monitor slightly too much, so I set the default zoom to 80%. Your latest commits have also fixed the default zoom being applied to new pages as well. 👍

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.

None yet

3 participants