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

Improved DPI Scaling on Windows and Fixed related Bugs #2155

Merged
merged 128 commits into from
Nov 11, 2024

Conversation

proneon267
Copy link
Contributor

@proneon267 proneon267 commented Oct 15, 2023

While investigating the scaling problems encountered in #1930, I found that the call to SetProcessDpiAwarenessContext is erroneous. Hence, all the toga apps are always running in the DPI Unaware mode.

This PR fixes the call and also checks if it was successful or not.

Note that this PR only has changes that fix the call to SetProcessDpiAwarenessContext, so that the call works. It doesn't fix any of the scaling issues currently present in toga. In particular, the test script from #1930, still shows the scaling bugs:

"""
My first application
"""
import toga
from toga.style import Pack
from toga.style.pack import COLUMN, ROW


class HelloWorld(toga.App):
    def startup(self):
        """
        Construct and show the Toga application.

        Usually, you would add your application to a main content box.
        We then create a main window (with a name matching the app), and
        show the main window.
        """
        main_box = toga.Box(style=Pack(direction="column"))

        self.main_window = toga.MainWindow(title=self.formal_name)
        self.main_window.content = main_box
        self.main_window.show()
        self.main_window.position = (0, 0)

        main_box.add(
            toga.Label(text=f"Window Size: {self.main_window.size}"),
            toga.Label(text=f"Window Position: {self.main_window.position}"),
            toga.Label(text=f"dpi_scale: {self.main_window._impl.dpi_scale}"),
            toga.Label(
                text=f"DpiX: {self.main_window._impl.native.CreateGraphics().DpiX}"
            ),
        )

def main():
    return HelloWorld()

At 125% scaling:
image

As you can see, the DpiX and dpi_scale will always be 96 and 1.0 respectively. Furthermore, in the DPI Aware mode, the app menu has a disproportionately larger size compared to the rest of the window elements.

Both of these bugs, indicate that there are problems in the scaling on windows. But those are for separate PRs.

Also, if required, I can add the code to correctly detect the actual system dpi.

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@proneon267
Copy link
Contributor Author

Though not my original intention, but I have fixed all dpi scaling bugs that I had encountered. Earlier, the scaling operations on windows were essentially no-ops. But the latest commit fixes the bugs, and the dpi based scaling operations are now being done properly.

At 125% scaling:
image
This correctly detects the dpi for scaling.

Now, all that remains is to add an event handler to detect dpi change while the app is running and scale correctly.

@proneon267 proneon267 changed the title Fixed bug in calling of SetProcessDpiAwarenessContext Fixed bugs related to dpi scaling on windows Oct 16, 2023
@proneon267
Copy link
Contributor Author

proneon267 commented Oct 18, 2023

I have added an event handler to detect DPI changes while the app is running. I have also modified Scalable class to:

  • Correctly get the latest DPI scale factor in real time
  • Remove dependency on a Control object
  • Allow other APIs to use the scale_in() and scale_out() methods
  • Reduce the number of calls to get the latest DPI value

The latest commit detects the DPI changes of the Primary Screen only. But this will be fixed in #1930, where the DPI scale factor of each screen will be detected individually and will be used to do the scaling.

I have tested the latest commit and it correctly detects new DPI change and scales the elements accordingly.

However, the fonts' don't seem to be using the latest DPI value and as such they are not being scaled when a new DPI change is detected while the app is running. This needs to be fixed.

Copy link
Member

@mhsmith mhsmith left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this, I think you're on the right track.

The latest commit detects the DPI changes of the Primary Screen only. But this will be fixed in #1930, where the DPI scale factor of each screen will be detected individually and will be used to do the scaling.

WinForms has some per-window events for detecting DPI changes, which I think would allow this to be fixed without requiring Toga to be aware of multiple screens. That would allow us to fix all the DPI issues together in this PR, so I've added a "fixes" link to the top comment.

A few more comments:

winforms/src/toga_winforms/app.py Outdated Show resolved Hide resolved
winforms/src/toga_winforms/app.py Outdated Show resolved Hide resolved
winforms/src/toga_winforms/app.py Outdated Show resolved Hide resolved
winforms/src/toga_winforms/app.py Outdated Show resolved Hide resolved
@proneon267
Copy link
Contributor Author

WinForms has some per-window events for detecting DPI changes, which I think would allow this to be fixed without requiring Toga to be aware of multiple screens.

I have tested them previously and was thinking about using DpiChanged event & DeviceDpiNew: https://learn.microsoft.com/en-us/dotnet/api/system.windows.forms.dpichangedeventargs.devicedpinew?view=windowsdesktop-7.0#system-windows-forms-dpichangedeventargs-devicedpinew

But, none of these events trigger when the system DPI changes. Only the SystemEvents.DisplaySettingsChanged event is triggered consistently when system DPI changes.

I am searching for a proper way to address this and will let you know as soon as I find a viable solution.

@proneon267
Copy link
Contributor Author

I have added support so that the font scales when the DPI changes while the app is running. I have also added Screen as an optional dependency to Scalable class, so that the DPI can be found for the current screen, without the need for the API in #1930, while still allowing other APIs to use scale_in and scale_out methods.

The following is the same run of the test script with the latest commit:
At 125% scaling:
image

At 175% scaling:
image

The fonts scale correctly and detect the DPI changes while the app is running.

@proneon267
Copy link
Contributor Author

When the app is scaled from 125% -> 100% -> 175%, then the menu bar appears to be clipped, due to incorrect calculation of main_window.context box:
Screenshot (20)
This needs to be fixed.

@proneon267
Copy link
Contributor Author

proneon267 commented Oct 22, 2023

Nevermind. The bug was caused due to the faulty implementation of WeakRef introduced in #2066 and reported in #2163.

self.native.Resize += WeakrefCallable(self.winforms_Resize)

After removing the WeakRef wrapping, the resizing event handler appears to fire consistently when the DPI scale is changed. Here is the app after fixing the bug:
image
So, this bug will be resolved automatically when #2163 is addressed. Looks like the weakref calls in different places are causing problems. I wonder if this is also leading to failing of the tests on windows testbed.

@freakboy3742
Copy link
Member

So, this bug will be resolved automatically when #2163 is addressed. Looks like the weakref calls in different places are causing problems. I wonder if this is also leading to failing of the tests on windows testbed.

To be clear - the tests are not failing on main at present. If they're failing in this PR, it's either an unintended side effect of something in this PR, or an error that isn't 100% reproducible.

The latter does happen sometimes - it's the nature of running a GUI test that sometimes, the GUI doesn't respond quite quickly enough, which results in a test failure. Re-running the test will (usually) fix these problems; however, I've just re-run the tests, and the same problem is occurring, which suggests it likely isn't a transient problem - it's an unintended side effect.

From a quick inspection, I can't see any obvious connection between this PR's changes and OptionContainer. However, I can confirm that when I run the optioncontainer tests, I see the problem locally, and if I remove the Weakref usage from OptionContainer, the problem remains.

@proneon267
Copy link
Contributor Author

In that case, I'll search further and report back what is causing the tests to fail.

@proneon267
Copy link
Contributor Author

proneon267 commented Oct 23, 2023

Turns out the bugs were related to Hwnds being created at inappropriate times.

As discussed in #2155 (comment), the Hwnds are being created even before the Application instance is created.

I have fixed the current bug by initializing and disposing a graphics context at the time of widget Hwnd creation.

But, I recon more bugs related to Hwnd will be encountered in the future due to the way toga app execution flow works. But that's for the future.

@proneon267 proneon267 changed the title Fixed bugs related to dpi scaling on windows Fixed Bugs related to DPI Scaling on Windows Oct 23, 2023
@mhsmith
Copy link
Member

mhsmith commented Nov 4, 2024

You should be able to test this by setting up a situation where some widgets are set to their minimum size, which is smaller than the window, and then checking that they scale correctly. For example:

toga.Box(
    style=Pack(direction="row"),
    children=[
        toga.Box(style=Pack(flex=1)),
        toga.Button(text="hello"),
        toga.Button(text="world"),
        toga.Box(style=Pack(flex=1)),
    ]
)

When the scale factor increases, the buttons should get bigger, and the spacer boxes should get smaller.

This was a bad idea, because it assumes that the window's physical size remains unchanged, which we've already found wasn't happening on @proneon267's machine. But the tests still passed for him because they didn't actually contain any assertions, only comparisons with no assert keyword.

I'll replace this with a different approach.

@mhsmith
Copy link
Member

mhsmith commented Nov 4, 2024

Actually, disregard my previous comment (apart from the tests having no assertions) -- the window's physical size will always remain unchanged, because it's only a mock DPI change, not a real one.

@mhsmith
Copy link
Member

mhsmith commented Nov 5, 2024

I'm working on the tests at the moment, so you don't need to push any more changes.

@proneon267
Copy link
Contributor Author

Thanks for helping :) and apologies for causing any trouble.

@mhsmith
Copy link
Member

mhsmith commented Nov 6, 2024

I had earlier found somewhere in Dotnet source code that DpiChanged event is conditionally turned off from firing when DPI settings are changed.

I see some mention of that here, and the relevant .NET code is indeed very difficult to understand. It looks like the events may only be enabled if you've set some metadata on the executable, but this isn't an option for Toga, unless we can set the metadata at runtime before .NET reads it.

They mention DpiChangedBeforeParent and DpiChangedAfterParent as possibly being more reliable. But let's continue with your current approach for now.

DpiChangedBeforeParent and DpiChangedAfterParent are not firing either.

@proneon267
Copy link
Contributor Author

DpiChangedBeforeParent and DpiChangedAfterParent are not firing either.

I think all these newer DPI related events will not be triggered unless DPI awareness is specified in a manifest file, which I haven't been able to figure out.

Btw, the new test looks great :)

@mhsmith
Copy link
Member

mhsmith commented Nov 10, 2024

@proneon267: here are the main reasons I had to change the tests. Hopefully this will help you write better tests in future PRs:

  • Calling the private event handler methods directly should be avoided in tests, because it doesn't prove that the app actually subscribed to the event. I changed it to trigger the events instead. For DisplaySettingsChanged this could only be done using reflection, which is why I looked into using the DpiChanged event instead, but that didn't work, as I mentioned in my previous comment.

  • The tests covered every possible upward DPI change, which took rather a long time, but no downward changes at all. I've changed it to always use the real DPI as a starting point (which in CI will be 100%), and to change both upwards and downwards to all the other options.

  • Asserting that the scaled up size is merely bigger than the original was a very weak test. I've replaced it with actual sizes and positions which are as specific as possible.

  • Parameterizing flex_direction was a good idea, but it became too complicated as I added more assertions. Instead, I used a single layout with fixed sizes and padding in both horizontal and vertical dimensions.

  • The PR manipulates font sizes, but there were no tests of that.

  • The PR manipulates the menu bar and toolbar, but there were no tests of that.

As a result of this testing, I made the following changes to the implementation:

  • I found that fonts are scaled according to the DPI scale of the primary screen at the time the app is started, not when each window is created.

  • Updating fonts by calling refresh on the interface layer of every widget did one full layout pass for every call, which meant it took more than a second to react to a DPI change on a moderately complex layout such as examples/window. I've changed it to call refresh at the implementation layer to update every widget's preferred size, followed by a single call at the interface layer to calculate the layout.

  • The menu bar was resizing itself when its font changed, but the toolbar was not. I found this was because we were using ToolStripMenuItem, which is only supposed to be used inside a menu, not directly on a toolbar. Replacing it with ToolStripButton fixed the scaling, and also fixed a long-standing bug where the blue hover rectangle on toolbar buttons was missing its bottom border.

@mhsmith
Copy link
Member

mhsmith commented Nov 10, 2024

On doing a deeper inspection, I have found that when dpi awareness is enabled, the following things happen:

Object Operates in Scaling Required
Form.Position dpi in-dependent coordinates No
Form.Size dpi dependent coordinates Yes
Screen.Bounds dpi in-dependent coordinates No

I'm pretty sure that the WinForms APIs all operate in physical pixels, so any inconsistencies here are caused by Toga itself.

And there is a problem with the existing design: it assumes there's a single CSS-pixel coordinate system covering all screens. But that isn't possible when different screens have different DPIs. Imagine you have four screens laid out like this:

    -------
    |  1  |
    -------
------- -------
|  2  | |  3  |
------- -------
    -------
    |  4  |
    -------

If screens 2 and 3 have the same physical resolution, but different DPIs, then what is the distance between screens 1 and 4 in CSS pixels? There are two answers, both equally valid.

I'd like to avoid this PR getting even bigger than it already is, so here's what I've done for now:

  • The interface layer code requires Window.position, Window.screen_position and Screen.origin to be in the same coordinate system, so I've scaled them all against the DPI of the primary screen.
  • Window.size is still scaled according to the current screen’s DPI. Apps may use this property to implement “breakpoints” in the layout, especially if we add an on_resize event (#2364), so it's important for it to be consistent with the scaling of the window's content.
  • Screen.size is scaled according to the screen’s own DPI, as it may be useful for apps to determine which screen is physically the biggest.

So on the primary screen, and any other screen with the same DPI, everything will work correctly. This is an improvement over the current main branch, which doesn't scale Screen.size and Screen.origin at all. But on a screen with a different DPI to the primary one, the position and size properties will be in different scales, so you won’t be able to position windows relative to the bottom or right edges of the screen or of other windows.

I doubt many people are doing things like this, so it's not urgent to fix, but in the future we can implement a proper solution, which I think would look like this:

  • Window.position: define to be relative to the current screen's origin and DPI, and move the calculation of that to the backend. Although this is technically backward-incompatible, it’s unlikely that any app will care. Even the "move" commands in examples/window will continue to work; the coordinate system will just change when the window moves onto a different screen.
  • Window.screen_position: deprecate, since it's now the same as Window.position.
  • Screen.origin: Since there will be no other absolute coordinates in the public API, the only thing an app can do with this information is find out the relative direction of each screen. So the implementation of this property can continue to use the DPI of the primary screen, but the documentation should say that the scale is undefined.

I've updated the examples/window app to help test this area. Note that the "screen edge" commands are always slightly inaccurate because they don't take the thickness of the title bar and invisible borders into account, but that's not relevant to this PR.

@mhsmith
Copy link
Member

mhsmith commented Nov 10, 2024

@freakboy3742: We could use a third pair of eyes on this, so please test this PR if you can. As we discussed, it has so many commits that it should be squash merged.

It would be a good idea to wait for @proneon267 to test it as well, since he's using a different version of Windows to me and he's discovered bugs in my work before.

@proneon267
Copy link
Contributor Author

@mhsmith I have tested this PR, and everything works properly without any glitches or bugs. Also, thanks for the testing tips, I will keep them in mind while implementing future PRs. This PR wouldn't have come to this stage without your immense help. Thank you :)

@freakboy3742
Copy link
Member

I've taken this for a drive. I'm not sure I'm fully up to speed with all the edge cases I should be looking for, but as far as I can make out, the scaling adaptation all seemed to work as expected, except for the "invisible window border" issue you mentioned. That's definitely an oddity, but if the Windows native API considers "window position 0,0" to include space for invisible border elements, then I'm not sure we should be bending over backwards to fix it.

We've discussed the origin/position changes in person, but for the record - I agree that's the general direction we should head. As nice as the idea of "absolute window positioning" is, it clearly doesn't work for the multi-DPI case unless the underlying window system operates entirely in CSS pixels (or any other consistent coordinate space); and given that the use case is pretty eclectic to start with, I'm happy to lose the feature in the name of having a solution that works reliably.

@mhsmith mhsmith merged commit 75806d2 into beeware:main Nov 11, 2024
40 checks passed
@mhsmith
Copy link
Member

mhsmith commented Nov 11, 2024

OK, thanks to both of you for your help. I've merged this PR, and created #2947 to track the remaining issue.

@proneon267
Copy link
Contributor Author

@mhsmith Apologies for pinging.

After updating 2 different PRs to the latest main branch, there is now intermittent failure of test_system_dpi_change.

I am guessing the issue might be because of the find_event() function being present below test_system_dpi_change. Could you also please confirm the cause?

@mhsmith
Copy link
Member

mhsmith commented Nov 11, 2024

It looks like you deleted the @property decorator above container_probe in winforms/tests_backend/window.py.

I am guessing the issue might be because of the find_event() function being present below test_system_dpi_change

I don't think so – although that function contains Windows-specific code, it's only called from within test_system_dpi_change, which is evidently being xfailed as it should be.

The failure isn't intermittent – it happens at the same point every time, on both x86_64 and ARM64 runners. And it looks like it's specific to this PR, as it didn't happen on the main branch nor in #2926.

I'm not familiar enough with this PR to suggest any specific cause. But since the xfail is done by code within the test rather than a decorator, the test's fixtures will still be created and destroyed, so it's possible the crash is related to the fixtures somehow.

Previous crashes with no message on macOS have often been because of Objective-C reference count errors (beeware/rubicon-objc#256).

@proneon267
Copy link
Contributor Author

You were right, the crash were related to the PRs' implementation and not on test_system_dpi_change. Thank you for helping.

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.

Text fuzzy on Windows with per-monitor scaling
3 participants