Improved DPI Scaling on Windows and Fixed related Bugs

[proneon267]

• Fixes #2135
• Fixes #1004
• Fixes #1038

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:

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:

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

[proneon267]

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: 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]

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.

[proneon267]

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]

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:

At 175% scaling:

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

[proneon267]

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: This needs to be fixed.

[proneon267]

Nevermind. The bug was caused due to the faulty implementation of WeakRef introduced in #2066 and reported in #2163. https://github.com/beeware/toga/blob/7a46d410be4da0f0b3aee622cbfccb282b87973f/winforms/src/toga_winforms/window.py#L36 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: 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]

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]

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

[proneon267]

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

As discussed in https://github.com/beeware/toga/pull/2155#discussion_r1365672659, 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.

[mhsmith]

FYI: it's better to allow the person who posted a comment to decide when to resolve it.

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

By "system DPI", do you mean the setting in your screenshots above? Or the one I mentioned in #2135, which doesn't take effect until you log out and in again?

[proneon267]

By "system DPI", do you mean the setting in your screenshots above? Or the one I mentioned in #2135, which doesn't take effect until you log out and in again?

The setting in the above screenshots, which is applied instantly.

[proneon267]

On further testing, it seems the stack trace dialog fonts are not being scaled. This needs to be fixed.

[proneon267]

I have made the stack trace dialogs to scale with DPI change. Here is test app: At 125% scalling:

At 175% scaling:

Do let me know if anything else isn't being scaled on DPI changes.

[mhsmith]

Thanks; I'm focusing on finishing the next Toga release right now, so it'll be a few days before I have time to look at this.

[proneon267]

No Worries. Take your time :)

[proneon267]

@freakboy3742 could you please take a quick look at why the windows testbed fails? No failure messages is generated. Does it fail because of missing test coverage?

[freakboy3742]

@freakboy3742 could you please take a quick look at why the windows testbed fails? No failure messages is generated. Does it fail because of missing test coverage?

Yes. With the completion of the audit, we now require 100% branch coverage in our testing - or a very good explanation of why branch coverage isn't possible.

For example, in this example we make an exception for the case where Webkit isn't installed. We need webkit to be installed so that we can... test webkit. The logic of the failure mode is reasonably simple, so we allow that branch to be uncovered by the test suite.

If you can make a similar case for why there's no practical way to test specific lines and branches that are being reported, we may allow the use of #pragma: no cover or #pragma: no branch (as appropriate).

Given the subject of this PR, it's possible that some the branches may be untestable, as we can't really change the DPI of the display during test conditions (at least, I'm not aware of any way to do this). However, if there's any way to test a branch (even it means "faking it" in some way), then we should do so.

[proneon267]

Thanks! I'll check what can be tested & what can be faked, and report back.

[freakboy3742]

FTR: Two approaches we've used to fake behaviours like this:

1. inject a mock somewhere appropriate (assuming that doesn't interfere with the behavior you're actually trying to test)
2. Directly invoke the API endpoint. There's a couple of places in the iOS code, for example, where there's no way to programmatically cause an event to occur (e.g., "swipe left"), so we programmatically invoke the API that is triggered when a swipe left occurs. Technically, we're not testing that the code responds to a swipe action... but if you accept that a swipe action calls a specific API, we can test that API responds in the way we expect. It's not perfect, but it's an acceptable workaround in difficult circumstances.

The second seems most likely option here - introduce a probe method to "change screen DPI" which is a no-op on macOS etc; but on windows, directly pokes the API that would be invoked if the screen resolution changed. You'll get code coverage of the "on DPI change" branches; coming up with a test that validates the behavior works when DPI isn't actually changing might be a little more complicated.

[proneon267]

On further testing, the window toolbars were not scaling. I have them to scale on DPI change. Here is the test app: At 175% with the bug:

At 175% with the bug fixed:

[proneon267]

Hello, I have some doubts regarding completing the test coverage. It complains for missing code coverage of: https://github.com/proneon267/toga/blob/407156e1781b4444d16c71ea344458a6755feff9/winforms/src/toga_winforms/app.py#L71-L75

    else:
        print(
            "WARNING: Your Windows version doesn't support DPI Awareness setting.  "
            "We recommend you upgrade to at least Windows 10 Build 1703."
        )

In update_scale method: https://github.com/proneon267/toga/blob/407156e1781b4444d16c71ea344458a6755feff9/winforms/src/toga_winforms/widgets/base.py#L25-L26

        if screen is None:
            screen = Screen.PrimaryScreen

In scale_in method: https://github.com/proneon267/toga/blob/407156e1781b4444d16c71ea344458a6755feff9/winforms/src/toga_winforms/widgets/base.py#L46-L47

        if not hasattr(Scalable, "dpi_scale"):
            self.update_scale()

In scale_out method: https://github.com/proneon267/toga/blob/407156e1781b4444d16c71ea344458a6755feff9/winforms/src/toga_winforms/widgets/base.py#L52-L53

        if not hasattr(Scalable, "dpi_scale"):
            self.update_scale()

In scale_font method: https://github.com/proneon267/toga/blob/407156e1781b4444d16c71ea344458a6755feff9/winforms/src/toga_winforms/widgets/base.py#L65-L66

        if not hasattr(Scalable, "dpi_scale"):
            self.update_scale()

How should I test these branches for complete code coverage?

Also, is code coverage dependent on tests? For example: I have in winforms/src/app.py: https://github.com/proneon267/toga/blob/407156e1781b4444d16c71ea344458a6755feff9/winforms/src/toga_winforms/app.py#L364-L375

    def winforms_DisplaySettingsChanged(self, sender, event):
        for window in self.interface.windows:
            window._impl.update_scale()
            if window._impl.toolbar_native is not None:
                window._impl.update_toolbar_font_scale()
            if isinstance(window._impl, MainWindow):
                window._impl.update_menubar_font_scale()
            for widget in window.widgets:
                widget.refresh()
            window._impl.resize_content()
            if hasattr(window._impl, "current_stack_trace_dialog_impl"):
                window._impl.current_stack_trace_dialog_impl.resize_content()

and in winforms/tests_backend/app.py, I have:

def trigger_dpi_change_event(self):
        self.app._impl.winforms_DisplaySettingsChanged(None, None)

In the tesbed testbed/tests/test_app.py, I have:

async def test_system_dpi_change(monkeypatch, app, app_probe):
    app.main_window.toolbar.add(app.cmd1, app.cmd2)
    stack = io.StringIO()
    traceback.print_stack(file=stack)
    dialog_result = app.main_window.stack_trace_dialog(
        "Stack Trace",
        "Some stack trace",
        stack.getvalue(),
        retry=False,
        on_result=None,
    )
    app_probe.trigger_dpi_change_event()
    app.main_window.toolbar.clear()

The test coverage remains the same, even if I do assert_called_* on the functions called in winforms_DisplaySettingsChanged by mocking window._impl.

Moreover, it complains about missing test coverage for: https://github.com/proneon267/toga/blob/407156e1781b4444d16c71ea344458a6755feff9/winforms/src/toga_winforms/window.py#L82-L88

    def update_toolbar_font_scale(self):
        if self.toolbar_native is not None:
            self.toolbar_native.Font = WinFont(
                self.original_toolbar_font.FontFamily,
                self.scale_font(self.original_toolbar_font.Size),
                self.original_toolbar_font.Style,
            )

https://github.com/proneon267/toga/blob/407156e1781b4444d16c71ea344458a6755feff9/winforms/src/toga_winforms/app.py#L28-L35

    def update_menubar_font_scale(self):
        # Directly using self.native.MainMenuStrip.Font instead of
        # original_menubar_font makes the menubar font to not scale down.
        self.native.MainMenuStrip.Font = WinFont(
            self.original_menubar_font.FontFamily,
            self.scale_font(self.original_menubar_font.Size),
            self.original_menubar_font.Style,
        )

However, they are called in winforms_DisplaySettingsChanged.

I am confused about how the code coverage is calculated.

[proneon267]

Maybe I will be guided better if I push the commit for the tests I have written so far.

[freakboy3742]

Hello, I have some doubts regarding completing the test coverage. It complains for missing code coverage of: https://github.com/proneon267/toga/blob/407156e1781b4444d16c71ea344458a6755feff9/winforms/src/toga_winforms/app.py#L71-L75

    else:
        print(
            "WARNING: Your Windows version doesn't support DPI Awareness setting.  "
            "We recommend you upgrade to at least Windows 10 Build 1703."
        )

This is an example of a line that we'd allow a #pragma: no-cover on. We don't have any capacity to test old windows versions; and the logic here isn't complex.

In update_scale method: https://github.com/proneon267/toga/blob/407156e1781b4444d16c71ea344458a6755feff9/winforms/src/toga_winforms/widgets/base.py#L25-L26

        if screen is None:
            screen = Screen.PrimaryScreen

This seems fairly straightforward - either the branch isn't needed at all, or there's a usage where you're not invoking update_scale() (with no arguments).

In scale_in method: https://github.com/proneon267/toga/blob/407156e1781b4444d16c71ea344458a6755feff9/winforms/src/toga_winforms/widgets/base.py#L46-L47

        if not hasattr(Scalable, "dpi_scale"):
            self.update_scale()

How should I test these branches for complete code coverage?

By setting up conditions where Scalable doesn't have a dpi_scale attribute, then invoking the method. It's not entirely clear to me what these conditions would be - but it does seem like dpi_scale should be a cached property, rather than an attribute that needs to be checked live on every usage, which will significantly simplify the coverage requirements.

Also, is code coverage dependent on tests?

I'm not sure what you mean by "dependent" here, but any code executed by a test will be marked as covered. There is a very limited window right at the start of the test suite that will escape coverage, but that essentially only impacts code that executes as a side-effect of importing the original app class.

The test coverage remains the same, even if I do assert_called_* on the functions called in winforms_DisplaySettingsChanged by mocking window._impl.

The coverage report says that lines 365-375 aren't executing. It's not complaining about 365->exit. This suggests the method is being invoked, but self.interface.windows is empty, so there's nothing to update.

Moreover, it complains about missing test coverage for: https://github.com/proneon267/toga/blob/407156e1781b4444d16c71ea344458a6755feff9/winforms/src/toga_winforms/window.py#L82-L88

The coverage report says lines 82-83 aren't covered. It's not complaining about 82->exit. This means you're not testing the "True" condition of the if - that is, what happens if the app has a toolbar.

I am confused about how the code coverage is calculated.

They're calculated when the line of code is executed. If the line of code isn't executed, it's reported as a coverage miss. It's that simple.

coverage.py is a very robust, well tested, and extensively used tool. If you believe that code is executing but is not being caught by coverage, the burden of proof is very high. It is far more likely that you are mistaken in your assumption that the code is running at all.

[proneon267]

On the latest commit, I got:

=========== 382 passed, 26 skipped, 18 xfailed in 159.25s (0:02:39) ===========
Name                                                                                     Stmts   Miss Branch BrPart  Cover   Missing
------------------------------------------------------------------------------------------------------------------------------------
D:\a\toga\toga\testbed\build\testbed\windows\app\src\app_packages\toga_winforms\app.py     167      2     52      5  96.8%   70-72, 371->375, 375->379, 386->367
------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                                                     1978      2    354      5  99.7%

In winforms/src/app.py I have: https://github.com/proneon267/toga/blob/392487a22a79e4bf157041d6fc3b2877d2b74a7b/winforms/src/toga_winforms/app.py#L364-L390

    def winforms_DisplaySettingsChanged(self, sender, event):
        # Print statements added only for testing, will be removed
        # in final code cleanup.
        for window in self.interface.windows:
            window._impl.update_scale(
                screen=WinForms.Screen.FromControl(window._impl.native)
            )
            if window._impl.toolbar_native is not None:
                print("About to update toolbar font scale...")
                window._impl.update_toolbar_font_scale()
                print("Done...")
            if isinstance(window._impl, MainWindow):
                print("About to update menubar font scale...")
                window._impl.update_menubar_font_scale()
                print("Done...")
            for widget in window.widgets:
                print("About to update menubar font scale...")
                widget.refresh()
                print("Done...")
            print("About to resize window content...")
            window._impl.resize_content()
            print("Done...")
            if hasattr(window._impl, "current_stack_trace_dialog_impl"):
                print("About to resize stack trace dialog...")
                window._impl.current_stack_trace_dialog_impl.resize_content()
                print("Done...")

When I run the test with:

briefcase dev --test -- tests/test_app.py --slow -k "test_system_dpi_change"

I get:

[testbed] Running test suite in dev environment...
===========================================================================
Waiting for app to be ready for testing... ready.
platform win32 -- Python 3.10.6
pytest==7.3.1
pluggy==1.0.0
C:\Users\proneon267\AppData\Local\Programs\Python\Python310\lib\site-packages\pytest_tldr.py:160: PytestRemovedIn8Warning: The (startdir: py.path.local) argument is deprecated, please use (start_path: pathlib.Path)
see https://docs.pytest.org/en/latest/deprecations.html#py-path-local-arguments-for-hooks-replaced-with-pathlib-path
  headers = self.config.hook.pytest_report_header(
asyncio: mode=auto
rootdir: E:\patch-20\testbed
plugins: asyncio-0.21.0, freezegun-0.4.2, pytest_freezer-0.4.8, tldr-0.2.5
cachedir: C:\Users\PRONEO~1\AppData\Local\Temp\.pytest_cache

------------------------------------------------------------------------------
tests/test_app.py::test_system_dpi_change ...
Constructing Window probe

Constructing app probe
Triggering DPI change event for testing property changes
About to update toolbar font scale...
Done...
About to update menubar font scale...
Done...
About to update menubar font scale...
Done...
About to resize window content...
Done...
About to resize stack trace dialog...
Done...
Triggering DPI change event for testing widget refresh calls
About to update toolbar font scale...
Done...
About to update menubar font scale...
Done...
About to update menubar font scale...
Done...
About to resize window content...
Done...
About to resize stack trace dialog...
Done...
Triggering DPI change event for restoring original state
About to update toolbar font scale...
Done...
About to update menubar font scale...
Done...
About to update menubar font scale...
Done...
About to resize window content...
Done...
About to resize stack trace dialog...
Done...
ok

------------------------------------------------------------------------------
Ran 1 tests in 3.55s

OK

[testbed] Test suite passed!

As you can see, the methods are being called, but still coverage complains about missing test coverage. Am I missing something here?

[freakboy3742]

When I run the test with:

briefcase dev --test -- tests/test_app.py --slow -k "test_system_dpi_change"

As an aside - you need to be slightly wary when running the tests --slow. It's a very useful debugging technique, but it can lead to slightly different execution order. If you see failures or different coverage in the CI report than you do with --slow, it's likely because the execution order is different.

As you can see, the methods are being called, but still coverage complains about missing test coverage. Am I missing something here?

Yes :-)

In the coverage report, the entries "70-72" and "371->375" are telling you different things.

"70-72" means "lines 70, 71, and 72 were not executed. This is a report of missing line coverage.

"371->375" means "the code can jump from line 371 to line 375, but that jump was never taken". This is a report of missing branch coverage.

So, yes - lines 371, 372, 373, and 374 are being executed. That's what the report is telling you. But the alternative path through the code - where the toolbar is None - is not being executed. And the report is failing on that basis.

[proneon267]

Done. Thanks!

[proneon267]

I have modified the scaling code so that the window's scaling factor is determined by the screen on which it is present. The widgets' scaling factor uses the scaling factor of the window on which they are present. This will prevent performance hit due to constant querying of DPI scaling factor from the OS.

I have manually tested the latest commit but I have not updated the tests and hence they will fail. I will update the tests as soon as time permits.

This also raises another question - if you have different scale factors set on different monitors, does a window update correctly when it's moved between them? I assume you have multiple monitors to test this, since you created https://github.com/beeware/toga/pull/1930.

I have found a way to trigger DPI change event when the window is moved from one screen to another. I will push it in the next commit.

[proneon267]

I have added support for scaling when moving between screens. I do not have a way to test this functionality in testbed CI. Hence, I have tested this functionality manually on a triple monitor setup.

[proneon267]

Let me know about required changes.

[mhsmith]

Thanks for the update. I'm busy on other things at the moment, but I'll hopefully get a chance to look at this within the next couple of weeks.

Meanwhile, there's no need to keep on merging from main every day, unless GitHub is indicating a conflict.