Skip to content

WinUI 3 foundational implementation - #4600

Open
Oliver-Leigh wants to merge 120 commits into
beeware:mainfrom
Oliver-Leigh:winui-3
Open

WinUI 3 foundational implementation#4600
Oliver-Leigh wants to merge 120 commits into
beeware:mainfrom
Oliver-Leigh:winui-3

Conversation

@Oliver-Leigh

Copy link
Copy Markdown
Contributor

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:

  • App
  • Command
  • Font
  • Icon
  • Paths
  • resources
  • MenuStatusIcon
  • SimpleStatusIcon
  • StatusIconSet
  • MainWindow
  • Window
  • Box
  • Button
  • Label
button_example_light button_example_dark

Note: Documentation is missing

Refs #2574

PR Checklist:

  • I will abide by the BeeWare Code of Conduct
  • I have read and have followed the CONTRIBUTING.md file
  • This PR was generated or assisted using an AI tool

Assisted-by:

@Oliver-Leigh

Copy link
Copy Markdown
Contributor Author

Thanks for the review @johnzhou721! The DPI suggestions were especially helpful.

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.

Unfortunately it didn't work.

@johnzhou721

Copy link
Copy Markdown
Contributor

Thanks for the review @johnzhou721! The DPI suggestions were especially helpful.

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 freakboy3742 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Comment thread changes/2574.feature.md

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fair enough! Updated.

Comment thread core/src/toga/app.py Outdated
)

def _startup(self) -> None:
print("app._startup() - start")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm assuming these are here for debugging purposes - they'll need to be removed before this lands.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Comment thread testbed/tests/app/test_desktop.py Outdated
]

# The size properties for WinUI 3 are not immediately available
await asyncio.sleep(0.1)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Comment thread testbed/tests/app/test_desktop.py Outdated
screen_window_dict[window_information["paired_screen"]] = window_information[
"window"
]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there any particular significance to the move of this test?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If I'm reading this right... this fix will also fix the "winforms testing on ARM" issue - is that your read as well?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Comment thread winui3/src/toga_winui3/fonts.py Outdated
Comment on lines +68 to +70
- 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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So I understand the implications here - it looks like Italic is still being honored; where is it being disabled as a default?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Comments have now been updated.

Comment on lines +52 to +56
def _creator(self, property):
def property_creator(property=property):
return property

return property_creator

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's to do with the staging. I've added a note at the top of the class.

Comment thread winui3/src/toga_winui3/icons.py Outdated

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a missing "on" here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes. I've rewritten this for clarity.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This module may not be necessary any more; the recent changes in core mean we can fall back on platformdirs as a default here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@freakboy3742

Copy link
Copy Markdown
Member

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.

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.

3 participants