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

Add support for HiDPI in TkAgg on Windows #19167

Merged
merged 7 commits into from May 14, 2021
Merged

Conversation

QuLogic
Copy link
Member

@QuLogic QuLogic commented Dec 23, 2020

PR Summary

At the moment, Tk does not support updating the 'scaling' value when the monitor pixel ratio changes, or the window is moved to a different one. Thus this needs a patch in the C extension to catch that window message and propagate it to Python. Due to thread locking issues, this needs to be done via a polled queue.

This is waiting for the pixel ratio refactor, #19126.

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • [n/a] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

@QuLogic
Copy link
Member Author

QuLogic commented Dec 23, 2020

I have not tested on macOS, but this may work if Tk supports it. I tried on Linux and it doesn't seem to work, but I don't have a true HiDPI system to test.

@QuLogic QuLogic force-pushed the tk-hidpi branch 2 times, most recently from 0e3cce3 to 52c07fe Compare December 25, 2020 03:48
@QuLogic QuLogic force-pushed the tk-hidpi branch 2 times, most recently from 5f1b3d1 to aebec04 Compare April 15, 2021 00:53
@QuLogic QuLogic changed the title Add initial support for HiDPI in TkAgg on Windows Add support for HiDPI in TkAgg on Windows Apr 23, 2021
@QuLogic
Copy link
Member Author

QuLogic commented Apr 23, 2021

After quite a bit of trial-and-error, I think this now fully supports HiDPI as much as Windows does.

@QuLogic QuLogic marked this pull request as ready for review April 23, 2021 03:39
src/_c_internal_utils.c Outdated Show resolved Hide resolved
src/_c_internal_utils.c Outdated Show resolved Hide resolved
{
switch (uMsg) {
case WM_DPICHANGED:
// This function is a subclassed window procedure, and so is run during
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider using Tcl_QueueEvent or Tcl_EvalEx to queue up a call to a tkinter-registered python function to avoid the polling loop. Failing that, I think this all occurs in the main thread, so you can use a deque instead of a SimpleQueue.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is not possible to call any Tk functions due to the Tkinter-internal Tcl thread lock. I'm fairly certain I tried Tcl_QueueEvent as well, but can do so again.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was under the impression that these windowprocs are called within Tcl_DoOneEvent, usually here:

https://github.com/python/cpython/blob/1e9f0933095403b215c2c4a0be7915d034ff7026/Modules/_tkinter.c#L2938

so I was expecting it to hold the tcl_lock already. If that's not the case... sorry for the distraction!

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually no, I think I tried Tk_QueueEvent which does not work because of said locks. Looking at the API for Tcl_QueueEvent, I'm not sure how it could be used. It takes a pointer a function to call later, which would still be my code and unable to (un)lock anything in the Tkinter library.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that Tk/Tcl is threaded on Windows, so it doesn't go through that route, but this one:
https://github.com/python/cpython/blob/1e9f0933095403b215c2c4a0be7915d034ff7026/Modules/_tkinter.c#L2931

As an alternative, I tried calling Tcl_SetVar on a variable named per-window, and this might work without crashing. I need to do a bit more testing to be sure, since concurrency issues can be heisenbugs.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I used a Tk variable now; this is nice as it cleans up the queue polling.

I think what happened originally is that I tried the variable approach from Python, and that failed due to the competing locks which I only found out about some time later.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like it, way better than the original and less magical than what I was imagining.

@QuLogic QuLogic force-pushed the tk-hidpi branch 2 times, most recently from 649606a to d22bdd1 Compare April 27, 2021 04:51
@tacaswell tacaswell added this to the v3.5.0 milestone Apr 29, 2021
@jklymak jklymak requested a review from anntzer May 10, 2021 19:32
@jklymak
Copy link
Member

jklymak commented May 10, 2021

@richardsheridan are you OK with this now? I can "officially" approve for you if you are... Thanks!

Copy link
Contributor

@richardsheridan richardsheridan left a comment

Choose a reason for hiding this comment

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

Didn't know you were waiting on an explicit approval from me! I'm happy with the new way the DPI change is triggered and with the change overall.

@jklymak
Copy link
Member

jklymak commented May 10, 2021

.... I don't think we were, but wanted to make sure you still thought it was on the right track. This still needs another person to approve.

@anntzer
Copy link
Contributor

anntzer commented May 10, 2021

I can't really claim any expertise on the topic, but I can report that on my (non-hidpi) laptop, this results in toolbar icons being clearly too big (by ~30%, just eyeballing it).

@anntzer anntzer removed their request for review May 10, 2021 23:22
@QuLogic
Copy link
Member Author

QuLogic commented May 11, 2021

Good point; it's 24 points, but I should probably scale that down to be whatever the equivalent is.

@QuLogic
Copy link
Member Author

QuLogic commented May 11, 2021

The buttons are back to the previous size, though the spacers are a bit smaller just because it's a rounder number.

On Windows, the toolbar is a bit bigger due to choosing a larger font. But the original font is so small, I think this is fine.

@anntzer
Copy link
Contributor

anntzer commented May 11, 2021

Indeed, the new version doesn't show the issue.

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

Successfully merging this pull request may close these issues.

None yet

5 participants