WinUI 3 foundational implementation - #4600
Conversation
|
Additional notes:
Testbed notes:
|
WinForms situation is documented on the description on #4331 and is also not being able to accept focus and not being able to test dialogs (which aren't implemented in this PR). So not saying this should be crammed into this PR, but if ad hoc skips are acceptable then we might be able to run all tests on Windows ARM64 as well. |
|
@johnzhou721 Thanks for adding the link with the extra context. I’ve used the skips to try and get as much coverage as possible, mainly because this is a new backend and I think we need all the coverage we can get. To apply these skips more broadly (e.g. for WinForms), I think some more structured approach is needed - one that fits better with the existing structure. On the other hand, it may not be worth spending the time on something which could be resolved soon. On a side note, has anyone tried the new windows-11-vs2026-arm runner? It might be worth trialing that runner before proceeding further. |
There was a problem hiding this comment.
For en ren implementasjonen :)
I took a morning to go through this code and flag some small things inline. A lot of this just comes down to personal preference, but I did catch some things that can be fixed in regards to DPI scaling.
There's a lot of complexity in terms of how staging areas are handled, but I think it's very well-documented and understandable.
There's some minor grammatical issues in the comments, but I'm not flagging them as they're mostly still understandable, and to avoid coming off as overly nitpicky.
Overall nice work though! I'm reviewing solely because I'm really excited about WinUI3 backend coming alive, not because I'm in any authority to definitively suggest changes to anything. Feel free to discuss anything I flagged directly inline.
P.S. Haven't tested yet, this is all from a static analysis of the code.
|
|
||
| async def test_as_image(app): | ||
| """A screen can be captured as an image""" | ||
| skip_on_backends("toga_winui3", reason="Screen.get_image_data is no implemented.") |
There was a problem hiding this comment.
I think we can use a pytest.skip in the probe method ScreenProbe.get_screenshot instead of doing this skip in the test (as we don't like to put platform specfics in testbed; exceptions exist such as skip_on_backends in each widget file.)
There was a problem hiding this comment.
I've done this now. The other benefit is that ScreenProbe gets constructed now too.
| on_alt_release_handler, | ||
| on_alt_drag_handler, | ||
| ): | ||
| skip_on_backends("toga_winui3") |
There was a problem hiding this comment.
Each module under this canvas folder have a skip_on_backends line (in test_deprecated_code.py and test_canvas.py) with toga_textual listed, so I think it'd be more uniform if we can put this platform skip on these lines as well.
| skip_on_backends( | ||
| "toga_textual", | ||
| reason="Canvas is not implemented on Textual.", | ||
| allow_module_level=True, | ||
| ) |
There was a problem hiding this comment.
This is the line I'm pointing to in https://github.com/beeware/toga/pull/4600/changes#r3697342126
|
|
||
| @pytest.fixture | ||
| async def widget(): | ||
| skip_on_backends("toga_winui3") |
There was a problem hiding this comment.
I think this can be put on line 15 with the toga_textual.
Some widgets use convention for skip_on_backends in the widget fixture, in that case I think it's fine to add to that list but I think it helps to consolidate all the skip_on_backend calls in one place (either module level or widget fixture) when possible. Whether to do this now and where to move into is up to the core team.
There was a problem hiding this comment.
I see you point, but I'm not sure which is the preferred solution. I think something to take into consideration is whether the widget/feature is expected to be implemented at all. Perhaps @freakboy3742 can weigh in on this?
There was a problem hiding this comment.
I see you point, but I'm not sure which is the preferred solution. I think something to take into consideration is whether the widget/feature is expected to be implemented at all. Perhaps @freakboy3742 can weigh in on this?
If it’s expected to be implemented a skip is used, if it’s not then it’s an xfail (xfail_on_platforms). IiRC that’s how we differentiate expectation of implementation, using the type of the skip rather than where it’s placed in the code. I think there’s a page in the contribution guide about this, but I can’t find where right now.
There was a problem hiding this comment.
I agree the two should be consolidated - there should be a single consistent location for declaring when a the testbed won't test a widget. The module level skip will be faster to run (as it won't construct and fail every test in the module), and it's also convenient to have the first actual code in a module be "... and here's where it doesn't apply". It also avoids the need to update the cleanup test as well.
The description about the use of xfail vs skip is in the testbed topic guide.
There was a problem hiding this comment.
Great we got this sorted. I'll update all the missing WinUI 3 widgets to module level skips.
| ) | ||
| async def test_secondary_window_toolbar(app, second_window, second_window_probe): | ||
| """A toolbar can be added to a secondary window""" | ||
| skip_on_backends("toga_winui3") |
There was a problem hiding this comment.
I think we can make has_toolbar use pytest.skip in WinUI3 tests here instead of a platform skip.
There was a problem hiding this comment.
Changed as suggested.
| @property | ||
| def min_size(self): | ||
| """The minimum size of the window in physical pixels (device pixels).""" | ||
| css_to_physical = self.get_current_screen().css_to_physical |
There was a problem hiding this comment.
Hmm... on some systems there's a slight timing difference between moving to a screen and using that screen's DPI.
I think we should directly use GetDpiForWindow here instead of querying the screen's properties for all DPI scale cases.
| # Window and client sizes are in physical pixels. | ||
| window_size = self.native.AppWindow.Size | ||
| client_size = self.native.AppWindow.ClientSize |
There was a problem hiding this comment.
This approach proved to be sort of unreliable when doing DPI scaling on WinForms. I suggest AdjustWindowRectExForDpi which is also more direct.
| def get_position(self) -> Position: | ||
| position = self.native.AppWindow.Position | ||
| physical_to_css = App.app._impl.get_primary_screen().physical_to_css | ||
|
|
||
| return Position(physical_to_css(position.X), physical_to_css(position.Y)) | ||
|
|
||
| def set_position(self, position: PositionT): | ||
| css_to_physical = App.app._impl.get_primary_screen().css_to_physical | ||
|
|
||
| self.native.AppWindow.Move( | ||
| PointInt32(css_to_physical(position.x), css_to_physical(position.y)) | ||
| ) |
There was a problem hiding this comment.
Same here with obtaining DPI directly from GetDpiFromWindow.
| if not self.interface.toolbar: | ||
| return | ||
|
|
||
| self.interface.factory.not_implemented("Window.create_toolbars") |
There was a problem hiding this comment.
| self.interface.factory.not_implemented("Window.create_toolbars") | |
| self.interface.factory.not_implemented("window toolbars") |
| parent_1.Name = "Parent Name" | ||
| assert parent_1.Name == parent_2.Name == "Parent Name" | ||
|
|
||
| parent_2.Name = "New Parent Name" | ||
| assert parent_1.Name == parent_2.Name == "New Parent Name" |
There was a problem hiding this comment.
I suggest perhaps using a uuid here, because this is called on many different widgets, all of which may have different parent containers.
Agreed about this.
No I have not. I think we can put it in this PR to see if the ARM64 coverage increases (since the handling inline is try-catch not direct skip in arm64 ci) and if so remove all the try-catches. |
|
Thanks for the review @johnzhou721! The DPI suggestions were especially helpful.
Unfortunately it didn't work. |
No worries! I don't have much time right now, but if you want to test multiple monitors setup, you can use this project: https://github.com/VirtualDrivers/Virtual-Display-Driver -- note that it may require test signing, though. The console app in that repo isn't really reliable, so installing the drivers through Device Manager may be a better approach there. (Hopefully my line of Norwegian at the start of my review wasn't too awkward...) |
This PR adds the foundation of a WinUI 3 backend for the Windows platform. The backend is based on the win32more Microsoft App SDK projection. The currently implemented entry points are:
Note: Documentation is missing
Refs #2574
PR Checklist:
Assisted-by: