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

fix(Skia): [WPF] Issue #7829 SynchronizationContext #7840

Conversation

workgroupengineering
Copy link
Contributor

GitHub Issue (If applicable): closes #7829

PR Type

What kind of change does this PR introduce?

  • Bugfix

What is the current behavior?

What is the new behavior?

PR Checklist

Please check if your PR fulfills the following requirements:

Other information

Internal Issue (If applicable):

@workgroupengineering workgroupengineering requested a review from a team January 17, 2022 11:47
@gitpod-io
Copy link

gitpod-io bot commented Jan 17, 2022

@workgroupengineering
Copy link
Contributor Author

Can anyone review this PR?

@workgroupengineering workgroupengineering force-pushed the fixes/Runtimes/Skia/WPF/SynchronizationContext branch from a745464 to a64b743 Compare January 21, 2022 10:21
Copy link
Member

@jeromelaban jeromelaban left a comment

Choose a reason for hiding this comment

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

I've added this test, which matches what you've mentioned:
image

But it does not seem to reproduce fix the issue, can you validate on your end?

src/Uno.UI.Runtime.Skia.Wpf/WpfHost.cs Outdated Show resolved Hide resolved
@workgroupengineering workgroupengineering force-pushed the fixes/Runtimes/Skia/WPF/SynchronizationContext branch from be8b1e4 to 7af534a Compare January 24, 2022 15:38
@workgroupengineering
Copy link
Contributor Author

I've added this test, which matches what you've mentioned: image

But it does not seem to reproduce fix the issue, can you validate on your end?

If run in test it works as expected but if run in Real Test APP the issue is present. I have tryed with 4.1.0-dev.461.

@jeromelaban
Copy link
Member

It would be important to understand what's different in the sample app, it's supposed to behavior the same way. Can you add the test in the runtime tests and see what results you're getting ?

@workgroupengineering
Copy link
Contributor Author

It would be important to understand what's different in the sample app, it's supposed to behavior the same way. Can you add the test in the runtime tests and see what results you're getting ?

I try.

How can I do to indicate that the test must be performed only in wpf?

I need to create a real WPF window and call event a via the window API.

@jeromelaban
Copy link
Member

The test should theoretically work on all platforms, but if we need to limit it to Skia, we'll be able to. Can you add it and launch a build? We'll see how far it goes :)

@workgroupengineering workgroupengineering force-pushed the fixes/Runtimes/Skia/WPF/SynchronizationContext branch 2 times, most recently from c08f893 to 0b7557b Compare January 26, 2022 12:01
@jeromelaban jeromelaban marked this pull request as draft January 26, 2022 13:21
@workgroupengineering workgroupengineering force-pushed the fixes/Runtimes/Skia/WPF/SynchronizationContext branch from 0b7557b to 68937be Compare January 26, 2022 15:30
@workgroupengineering workgroupengineering force-pushed the fixes/Runtimes/Skia/WPF/SynchronizationContext branch from 68937be to 096eaf5 Compare February 5, 2022 18:02
@workgroupengineering workgroupengineering marked this pull request as ready for review February 23, 2022 16:34
@MartinZikmund
Copy link
Member

@jeromelaban Is there some way to the test could avoid requiring separate runtime test project for WPF specifically 🤔 ? (it does not seem so based on the fact the test needs to access WpfHost, but it would be nicer to be able to avoid the additional project.

@jeromelaban
Copy link
Member

@jeromelaban Is there some way to the test could avoid requiring separate runtime test project for WPF specifically 🤔 ? (it does not seem so based on the fact the test needs to access WpfHost, but it would be nicer to be able to avoid the additional project.

I don't think it's necessary at this point. The test that requires a window to validate, this feels like a too low-level replacement for a UI test engine that would do this in a generic way.

The solution to override the synchronization context for all event going out seems a bit too wide as well, particularly if it's about events going out of Uno's APIs, for which the entry point is known. Let's change those entry points for now to set the proper synchronization context. We can create a test that uses the new pointer injection to validate that we get the proper synchronization context out inside of uno pointer or generic input handlers.

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.

[Skia][WPF] RenderSize is [0,0] when generating the control in the event handler
3 participants