FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add support for HiDPI in TkAgg on Windows by QuLogic · Pull Request #19167 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

Add support for HiDPI in TkAgg on Windows - #19167

Merged
tacaswell merged 7 commits into
matplotlib:masterfrom
QuLogic:tk-hidpi
May 14, 2021
ConversationCommits (7)ChecksFiles changed
Merged

Add support for HiDPI in TkAgg on Windows#19167
tacaswell merged 7 commits into
matplotlib:masterfrom
QuLogic:tk-hidpi

Conversation

QuLogic commented Dec 23, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

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 commented Dec 23, 2020

Copy link
Copy Markdown
Member Author

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 force-pushed the tk-hidpi branch 2 times, most recently from 0e3cce3 to 52c07fe Compare December 25, 2020 03:48
anntzer reviewed Feb 10, 2021
View reviewed changes
Comment thread lib/matplotlib/backend_bases.py Outdated
QuLogic force-pushed the tk-hidpi branch 2 times, most recently from 5f1b3d1 to aebec04 Compare April 15, 2021 00:53
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 commented Apr 23, 2021

Copy link
Copy Markdown
Member Author

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

QuLogic marked this pull request as ready for review April 23, 2021 03:39
anntzer reviewed Apr 23, 2021
View reviewed changes
Comment thread src/_c_internal_utils.c
anntzer reviewed Apr 23, 2021
View reviewed changes
Comment thread src/_c_internal_utils.c Outdated
anntzer reviewed Apr 23, 2021
View reviewed changes
Comment thread src/_c_internal_utils.c Outdated
anntzer reviewed Apr 23, 2021
View reviewed changes
Comment thread lib/matplotlib/backends/_backend_tk.py Outdated
anntzer reviewed Apr 23, 2021
View reviewed changes
Comment thread src/_tkagg.cpp
richardsheridan reviewed Apr 23, 2021
View reviewed changes
Comment thread src/_tkagg.cpp
{
switch (uMsg) {
case WM_DPICHANGED:
// This function is a subclassed window procedure, and so is run during

richardsheridan Apr 23, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

QuLogic Apr 23, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

richardsheridan Apr 23, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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!

QuLogic Apr 24, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

QuLogic Apr 24, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

QuLogic Apr 27, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

richardsheridan Apr 27, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

jklymak commented May 10, 2021

Copy link
Copy Markdown
Member

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

richardsheridan approved these changes May 10, 2021
View reviewed changes

richardsheridan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented May 10, 2021

Copy link
Copy Markdown
Member

.... 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 commented May 10, 2021

Copy link
Copy Markdown
Contributor

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 removed their request for review May 10, 2021 23:22

QuLogic commented May 11, 2021

Copy link
Copy Markdown
Member Author

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

QuLogic commented May 11, 2021

Copy link
Copy Markdown
Member Author

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 commented May 11, 2021

Copy link
Copy Markdown
Contributor

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

tacaswell merged commit 86d6a0d into matplotlib:master May 14, 2021
QuLogic deleted the tk-hidpi branch May 14, 2021 03:33
QuLogic mentioned this pull request Jul 8, 2021
Closed
tacaswell mentioned this pull request Aug 14, 2021
Closed
daniilS mentioned this pull request Dec 6, 2021
Open
This was referenced Dec 18, 2021
Closed
Merged
QuLogic mentioned this pull request Mar 3, 2023
Closed
QuLogic mentioned this pull request Jun 30, 2023
Open
ksunden mentioned this pull request Sep 11, 2023
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

anntzer anntzer left review comments
jklymak jklymak approved these changes
richardsheridan richardsheridan approved these changes

Assignees

No one assigned

Labels

GUI: tk

Projects

None yet

Milestone

v3.5.0

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL