WinUI 3 foundational implementation - #4600
Conversation
|
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...) |
freakboy3742
left a comment
There was a problem hiding this comment.
I'm still working my way through this, but I wanted to give you the comments I have so far. On the whole, this works great in my testing; I'm still working out how some of the finer details (like the cascading properties and staged properties) work.
There was a problem hiding this comment.
We only need a single feature here - the notable entry in the next release will be that there is a WinUI3 backend at all, rather than multiple specific feature call outs.
There was a problem hiding this comment.
Fair enough! Updated.
| ) | ||
|
|
||
| def _startup(self) -> None: | ||
| print("app._startup() - start") |
There was a problem hiding this comment.
I'm assuming these are here for debugging purposes - they'll need to be removed before this lands.
There was a problem hiding this comment.
Yes, occasionally the testbed reports that the window never became visible. I'm trying to find where the issue happens with this code, but it hasn't popped up again yet.
| ] | ||
|
|
||
| # The size properties for WinUI 3 are not immediately available | ||
| await asyncio.sleep(0.1) |
There was a problem hiding this comment.
Flagging because this is a mild concern as I work through the PR in alphabetical order - unclear if this is purely a "testbed testing" concern, or a "real" concern. If it's purely a Testbed thing, that's OK; it's more of a problem if it's going to have a broader "real" impact.
| screen_window_dict[window_information["paired_screen"]] = window_information[ | ||
| "window" | ||
| ] | ||
|
|
There was a problem hiding this comment.
Is there any particular significance to the move of this test?
There was a problem hiding this comment.
Not really. I was trying to sort out the parts related to the sleep. I've changed this now to use the window_probe.wait_for_window() method instead. I think it's much cleaner.
| # GitHub Windows ARM64 runners don't seem to be able to accept input focus. | ||
| # See https://github.com/actions/partner-runner-images/issues/174 | ||
| if platform.machine() != "ARM64" or os.environ["RUNNING_IN_CI"] != "true": | ||
| raise AssertionError from e |
There was a problem hiding this comment.
If I'm reading this right... this fix will also fix the "winforms testing on ARM" issue - is that your read as well?
There was a problem hiding this comment.
Maybe "fix" is too strong but it would help us get more test coverage on ARM64. I'm honestly not sure what the best structure for this type of skip is. I think the current approach needs some work (for example, here there should probably be an "is Windows" type condition as well).
One possibility is to apply the existing Winforms approach here as well. Then open a separate issue to try and get as much ARM64 coverage on both Windows backends.
| - Italics goes against the Windows design prinicpals, so it is safe to set the | ||
| Normal font style by default. See: | ||
| learn.microsoft.com/windows/apps/design/signature-experiences/typography |
There was a problem hiding this comment.
So I understand the implications here - it looks like Italic is still being honored; where is it being disabled as a default?
There was a problem hiding this comment.
Yes, but I'll reword this comment because it's not clear what I'm trying to say.
The point is that Toga's default style is NORMAL not SYSTEM. Conceivably some parts of the UI would default to a different style. However, the Windows design philosophy means that we have SYSTEM == NORMAL anyway.
There was a problem hiding this comment.
Comments have now been updated.
| def _creator(self, property): | ||
| def property_creator(property=property): | ||
| return property | ||
|
|
||
| return property_creator |
There was a problem hiding this comment.
I'm sure there's a technical reason for this... what is it? As written, it seems like a really complicated way to implement self.FontFamily = family...
There was a problem hiding this comment.
It's to do with the staging. I've added a note at the top of the class.
|
|
||
| The WinUI 3 backend needs two type of icon: | ||
| - Native WinUI 3 - to be used with most native WinUI 3 classes such as button. | ||
| - Win32 - to be used with StatusIcons the title bar. |
There was a problem hiding this comment.
Is there a missing "on" here?
There was a problem hiding this comment.
Yes. I've rewritten this for clarity.
There was a problem hiding this comment.
It's annoying that this is (AFAICT) near identical to the Winforms version; not sure if I have any ideas for how to refactor this, though.
There was a problem hiding this comment.
I think we can factor out ReadyDeque and TwoThreadIocpProactor. I'm not sure where to put them though. In fact there are a few Win32 things which are duplicated as well.
The WinUI 3 loop is a bit simpler than the Winforms loop because of the way scheduling works.
There was a problem hiding this comment.
This module may not be necessary any more; the recent changes in core mean we can fall back on platformdirs as a default here.
There was a problem hiding this comment.
I've rewritten the Paths module to use platformdirs via the PlatformDirsPaths class. This way we can leverage the advantages of platformdirs and still have distinct Config and Data folders.
|
Thanks for those updates; to set expectations - I'm about to head to PyCon AU, so my bandwidth to review code will be reduced for about a week. I'll take a look if I get a chance, but it might take a little longer than normal. |
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: