Improve DPI Scaling on Windows More and Fix More Related Bugs - #4577
Conversation
Submitting as draft for CI testing; more documentation will come later.
|
It's been a long journey, with lots of comorbid issues here, but I finally got down to the bottom of everything, so this is ready for review. I recognize that this PR is big both in terms of code and conceptually, but I feel that explaining all the individual changes and refactors as separate pieces of work would be a lot harder. If you have suggestions as to if/how to split this PR, let me know. Reviewing in pieces is also appreciated and fine with me. I appreciate y'all's time to sort this piece of work out. |
freakboy3742
left a comment
There was a problem hiding this comment.
A couple of typos, and possible areas for cleanup - but on the whole, this looks great. It's definitely pushing the bounds of my Winforms experience, and I don't have the ability to test the second monitor handling - but I can't spot any issues in my manual testing.
In terms of the "scope" of the PR - I can't really see many opportunities to split it up. Maybe the font scaling stuff could be in a standalone PR... but I agree those are sufficiently co-morbid that it's likely going to end up more confusing trying to review them separately.
| print( | ||
| "WARNING: Your Windows version doesn't support DPI Awareness setting. " | ||
| "We recommend you upgrade to at least Windows 10 version 1703." | ||
| ) |
There was a problem hiding this comment.
I think we can kill this branch. It's been 9 years since the release of Windows 10 v1703, and the only version of Windows 10 earlier than this that have any semblance of support is v1607 - and even that is only in LTSC ESC as of October this year. I'd be comfortable replacing this with a startup check in toga_winforms/__init__.py and documenting the minimum supported service level in the docs.
| import threading | ||
|
|
||
| import System.Windows.Forms as WinForms | ||
| from Microsoft.Win32 import SystemEvents |
There was a problem hiding this comment.
With this being removed - can we remove the reference to the assembly in toga_winforms/__init__.py?
There was a problem hiding this comment.
Yep, I think so. Done!
| self.pfn_subclass = ws.SUBCLASSPROC(self._subclass_proc) | ||
| self.native.HandleCreated += WeakrefCallable(self.winforms_handle_created) | ||
| self.native.HandleDestroyed += WeakrefCallable(self.winforms_handle_destroyed) | ||
| self._set_subclass() |
There was a problem hiding this comment.
Is this needed here? It's also called in the winforms_handle_created handler.
There was a problem hiding this comment.
Status quo is what was done in Table’s win32 subclass. Im not familiar enough with win32 subclassing to say how these type of magical incantations work exactly, so I copied table setup and am not sure about your question.
should we ask Oliver-Leigh? Would it be appropriate to tag him here?
There was a problem hiding this comment.
I did some more research on this topic... turns out a WinForms Form can internally switch between different Win32 windows when changes occur.
So... this _set_subclass sets the subclass for the handle to the initial Win32 Window WinForms uses, and whenever some change in the Form results in WinForms deciding to create a new Win32 Window, we immediately attach our subclass to the new Win32 Window in HandleCreated, thus this is needed twice.
This is as many details I can provide, aftering exhausting my options for doing research on this. So if more details are needed having someone with Win32 expertise would be helpful.
There was a problem hiding this comment.
Ok - but I would have thought the first handle will be created almost immediately, which is why it's effectively a duplicate.
If @Oliver-Leigh has any thoughts, I'm happy to revisit this; but otherwise, I'm not overly concerned about this though. It's a fairly minor overhead if it is redundant.
There was a problem hiding this comment.
It looks like it's right - you need the call self._set_subclass() to set the subclass for the existing handle, and then you need the event listener because the handle of the form changes during the lifetime of the instance. I think the first handle is created when the form is instantiated, although I'd have to look at the .NET source code to confirm this.
On a broader note, I think that all the Win32 subclassing should be made more Pythonic in the long run. It's a bit ad-hoc at the moment. Maybe we need a new issue for this.
There was a problem hiding this comment.
On a broader note, I think that all the Win32 subclassing should be made more Pythonic in the long run. It's a bit ad-hoc at the moment. Maybe we need a new issue for this.
If there's any we can do to abstract the creation process here, that's a big +1 from me. If you've got ideas about this already, then feel free to open an issue.
Co-authored-by: Russell Keith-Magee <russell@keith-magee.com>
Co-authored-by: Russell Keith-Magee <russell@keith-magee.com>
Co-authored-by: Russell Keith-Magee <russell@keith-magee.com>
Co-authored-by: Russell Keith-Magee <russell@keith-magee.com>
Co-authored-by: Russell Keith-Magee <russell@keith-magee.com>
|
I've made all the requested changes and replied to the another question inline; this is ready for another review. Since some small refactors were done here and there in the latest iterations of this, and the issue filer of #4486 was able to reproduce some issues more frequently than me, but he won't be available until July 28, I suggest reviewing this PR, having it sit on ice for a few days while we wait for testing before merge. |
freakboy3742
left a comment
There was a problem hiding this comment.
I've added some documentation and some small cleanups; and I've snuck in a fix for the StackTrace dialog that seems to be affected by these changes - that implementation can almost certainly be improved (making it adaptive to DPI changes), but the existing implementation wasn't working at all so I'll take the improvement.
Otherwise, I think this is good to land. It's a significant improvement; if @Molrn is able to find any additional problems when they're able to test, we can revisit then.
Fixes #4486 (or at least significantly makes it more reliable).
All the issues, debugging, and reproducers are documented below. If you can suggest a way to split this PR up, I'd be interested to complete such a refactor. Alternatively, you can also provide feedback in small pieces if you want.
Issues resolved
When you have 2 monitors with differing DPIs and you drag a window rapidly between them:
@Molrn's machine, rather than my VM.From a static analysis of the currrent code, I also found DPI scaling hack in L25 (removed line 25) of window.py:
Things changed
I took some care to get rid of the initial_dpi_scale hack first by switching to pixel-based font sizing. All font sizes are created with a unit of pixels now throughout the codebase, including the initial default font which is explicitly converted to pixels. This allows us to get rid of the hack properly, and also makes sure that tests for the work done below are not impacted by any font scaling bugs.
Then, I completely removed any usages of .NET events (
DisplaySettingsChanged,LocationChanged,Resize), and went straight to Win32 to use WM_DPICHANGED. We have to use the Win32 event, not the managed DpiChanged event, as the latter is not reliable (see #2155). We also use the Win32 API GetDpiForWindow to get the initial dpi of the window, and then rely on the WPARAM of WM_DPICHANGED to deliver new DPI values. This was the most reliable approach from experiments.I also took the chance to separate concerns a bit more, i.e. I refactored the changing of menubar/toolbar fonts and widget fonts into a new update_fonts function. This allowed me to further refactor the pipeline on a DPI resize to increase reliability.
The LPARAM of WM_DPICHANGED contains a suggested rectangle for how to reposition and resize our window, so a SetWindowPos was added to the WM_DPICHANGED routine to address window resizing issues. This only fixes .NET Framework; .NET Core intentionally suppresses scaling window size when we do not use AutoScale, so we return 0 on WM_GETDPISCALEDSIZE to bypass that handling, More details inline.
Now, moving a window from a smaller to larger DPI caused the window to bloat up by a significant amount of size. Turns out, WinForms's ClientSize property can get out of sync with the Size property, so I used Win32's AdjustWindowRectExForDpi to work out the decor sizes directly.
The MinimumSize of the window also constrains the new size to be applied, so we do
self.native.MinimumSize = WinSize(0, 0)beforeSetWindowPos, so that when we move to a smaller DPI screen the window can scale down appropriately.Test plan
Since we now use a Win32 event that directly passes in the new DPI as a parameter, we can just directly send the new DPI to the Win32 event to mock a dpi change. To make sure the window is resized properly, we also pass in a mocked LPARAM as the suggested rectangle, and ensure the window is resized properly.
Notably, since the decor sizes does not change, we have to mock the decor size function to use the older size.
For the probe for retrieving font sizes, the existing SizeInPoints approach is preserved, which seems to work properly with our Pixel based font sizing; it also automatically scales out the DPI. However, our mock Win32 dpi changes does not actually change the underlying DPI used in calculating SizeInPoints. Thus, the `scale_change logic in asserting new font sizes is preserved.
The font sizing change also caused a small change to the Canvas reference image, which was updated.
There's also a small (preexisitng) menubar and toolbar sizing issue in tests when running tests on real scales of 150%+, documented inline in tests.
Enjoy!
PR Checklist:
Assisted-by: GitHub Copilot, Google Gemini, Google AI Mode