Xaml Islands Fixes. #3206
Xaml Islands Fixes. #3206
Conversation
…n favor of DispatcherHelper.
|
Thanks azchohfi for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌 |
ghost
assigned 
Microsoft.Toolkit.Uwp.Connectivity/BluetoothLEHelper/BluetoothLEHelper.cs
Outdated
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.Connectivity/BluetoothLEHelper/ObservableGattCharacteristics.cs
Outdated
Show resolved
Hide resolved
UnitTests/UnitTests.XamlIslands/UnitTests.XamlIslands.dll.manifest
Outdated
Show resolved
Hide resolved
UnitTests/UnitTests.XamlIslands/XamlIslandsTest_ThemeListener_Threading.cs
Outdated
Show resolved
Hide resolved
| namespace UnitTests.XamlIslands | ||
| { | ||
| [STATestClass] | ||
| public partial class XamlIslandsTest_ThemeListener_Threading |
There was a problem hiding this comment.
XamlIslandsTest_ThemeListener_Threading [](start = 25, length = 39)
you may want to spin up two different UI threads (each on their own STA) to check that you dispatch back to the correct UI thread in both cases, as a test. #Closed
This will not fire for XAML Islands - is that ok for your scenario? If not we can discuss what you can do instead. #Closed Refers to: Microsoft.Toolkit.Uwp.UI/Helpers/ThemeListener.cs:74 in 8464f8e. [](commit_id = 8464f8e, deletion_comment = False) |
This whole type is basically unusable in XAML Islands since it relies on CoreWindow pointer events. Refers to: Microsoft.Toolkit.Uwp.UI/Extensions/ScrollViewer/ScrollViewerExtensions.MiddleClickScrolling.cs:22 in 8464f8e. [](commit_id = 8464f8e, deletion_comment = False) |
… after the creation of their parents.
| /// <summary> | ||
| /// Gets or sets which DispatcherQueue is used to dispatch UI updates. | ||
| /// </summary> | ||
| public DispatcherQueue DispatcherQueue { get; set; } |
There was a problem hiding this comment.
Why does this need to be public?
There was a problem hiding this comment.
We didn't have the requirement that this class needed to be instantiated from a UI thread. Since this might be a scenario for some users, I wanted to make sure that they could set the dispatcher if they already have an instance of it (thru the ctor), or change it later (thru the public property).
| /// <summary> | ||
| /// Gets or sets which DispatcherQueue is used to dispatch UI updates. | ||
| /// </summary> | ||
| public DispatcherQueue DispatcherQueue { get; set; } |
There was a problem hiding this comment.
Why does this need to be public?
There was a problem hiding this comment.
We didn't have the requirement that this class needed to be instantiated from a UI thread. Since this might be a scenario for some users, I wanted to make sure that they could set the dispatcher if they already have an instance of it (thru the ctor), or change it later (thru the public property).
|
Good questions @skendrot!
We didn't have the requirement that this class needed to be instantiated from a UI thread. Since this might be a scenario for some users, I wanted to make sure that they could set the dispatcher if they already have an instance of it (thru the ctor), or change it later (thru the public property).
This changes will make the toolkit works better (literally not crash) when running inside Xaml Islands. CoreDispatcher is UWP only, but DispatcherQueue was designed to works fine on both Win32, and UWP.
UWP's CoreApplicationView and the toolkit's DispatcherHelper are both 1:1. This scenario worked fine when we shipped Windows8, but Islands created this difficult situation of taking these 1:1 APIs (1 thing to 1 thread, in most cases), and making it 1:*. Its a fundamental change that Islands introduced, not only for dispatchers, but also for DPI, GetForCurrentView, MainView. That is why this PR is still in draft. I'm still talking a lot with @Austin-Lamb to make sure we are covering all the scenarios we can to make sure the Toolkit works in Xaml Islands. Again, this will benefit every Toolkit developer, and we are trying to make it as few breaking changes as possible. |
|
We need to target 19H1 to do the other fixes, so I'll send a separate PR after the SDK bump is merged. Lets review this one as-is for now. |
@Austin-Lamb we were going to do that, but we changed our for 7.0 we'll still support RS4. On 8.0 we'll follow whatever WinUI3 does. |
|
Actually, v7.0 should support RS3, not RS4. Those checks can still be remove though. |
There was a problem hiding this comment.
Thanks for fixing Eyedropper @azchohfi The list of controls is performing as expected now with different DPI's and resizing
build.bar executed fine and was successful
unittests.xamlislands.package is also performing as expected.
|
@Austin-Lamb we've been keeping the min target lower for the toolkit to ease developers who still target down-level or LTS OS versions with the Toolkit as if we bump the toolkit version, they can't target anything below that, even with conditional XAML. Since we're close to just moving to WinUI 3 (#3106), we're not going to bump the min version for 7.0 (but upgrade the target SDK). However, we really only test and will address critical issues on the more recent OS builds (as otherwise the test matrix is too large for us to support), but we are always happy for any minor updates that do address issues with lower platform versions submitted by the community, as long as they keep the functionality the same on current platforms. |
Microsoft.Toolkit.Uwp.SampleApp/SamplePages/DispatcherHelper/DispatcherHelperPage.xaml.cs
Show resolved
Hide resolved
Kyaa-dost
added
XamlIslands 🏝️
bug 🐛
## Follow up for #3206 <!-- Add the relevant issue number after the "#" mentioned above (for ex: Fixes #1234) which will automatically close the issue once the PR is merged. --> <!-- Add a brief overview here of the feature/bug & fix. --> ## PR Type What kind of change does this PR introduce? <!-- Please uncomment one or more that apply to this PR. --> - Bugfix - Code style update (formatting) - Refactoring (no functional changes, ~~no api changes~~) - Optimization ## What is the new behavior? <!-- Describe how was this issue resolved or changed? --> This PR includes a number of optimizations, tweaks and refactorings to `DispatcherQueueHelper`: - Removed unnecessary `null` checks for `function`, and enabled nullability annotations. Those `null` checks were unnecessary since there were also other parameters that could also be `null` (ie. the actual `DispatcherQueue`) anyway, plus we'd still get an exception anyway when those instances are accessed. Instead, we now have nullability annotations to make the intent clearer. - Avoided the slowdown and memory allocations in case the thread already had access to the dispatcher queue, which was caused by the C# compiler (sadly) always preemptively instantiating the display class for the closure of the lambda expression used in the other branch (I made a sharplab repro [here](https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA+ABATARgLABQGADAAQY4CsA3IYRgMzlakDChA3oaT+UxUnKCAogDsAjgFcY0gIIBnAJ6iwACgAiAS3kAHAIYAXMAAsYUAIrTppKTJgAacjhZ6wBzRFEBKbry4FeQNJNADNSVVtpADoACT15ABVjWD0AE1kwMBh5eR8AoJ5/AoKDKEVfYr8KyqDXd09VL1p8mqDq1p4MAHYhKNYIAFsdABsYAxhUjCRmjtIAX3bisEMTcOEELJ160VIYPNmi2d5u3oAxKEH1ze3VPZmOhZagx8rFngA3PShSI3lSAF5SKIYAB3IT9IajbYAZQgkigWQAPBBgAArGBuAB8jXuBTeNisMCiCTKYkiMFUjQBmPxh1apXKT0qdKOdQ8ohx+NeTI6vyi0LGACVspJhgZVKJRcMmlzeC8OssjMY1hsYFt2bt9h0WbM+QKDFc1Tc7rKePKCnMZTyePiTnyprj5oRHvR+CwtLoVqYLITONVGKRgBAIMNSHFEskYGkMlkcqQOKQAOZjahOp7+/goUgkxRkwmqCgsYx6USpUZQLWFbmBR5zIA=) that illustrates the issue). Instead now the fallback path is moved to a separate method, so that the IL has the correct codegen. - Renamed the class to `DispatcherQueueExtensions`, since these are all extensions and not a helper class - Similarly, moved the class to the `.Extensions` namespace - Renamed the APIs to `EnqueueAsync` to mirror the `TryEnqueue` name of the actual API in the `DispatcherQueue` class. This better follows the naming convention of the class, and it's also clearer as in case users have multiple windows and different dispatching queues, there isn't really a single "UI thread" - the API is literally just enqueueing an operation on a given dispatcher queue. - Added a check in case `TryEnqueue` fails, so if that happens we now return a wrapped `InvalidOperationException` in the `Task`, whereas the previous behavior would've just caused that task to never be completed, leaving the caller just waiting forever. ## PR Checklist Please check if your PR fulfills the following requirements: - [X] Tested code with current [supported SDKs](../readme.md#supported) - [ ] ~~Pull Request has been submitted to the documentation repository [instructions](..\contributing.md#docs). Link: <!-- docs PR link -->~~ - [ ] ~~Sample in sample app has been added / updated (for bug fixes / features)~~ - [ ] ~~Icon has been created (if new sample) following the [Thumbnail Style Guide and templates](https://github.com/windows-toolkit/WindowsCommunityToolkit-design-assets)~~ - [ ] ~~Tests for the changes have been added (for bug fixes / features) (if applicable)~~ - [X] Header has been added to all new source files (run *build/UpdateHeaders.bat*) - [ ] Contains **NO** breaking changes
Fixes #3205
Fixed dispatcher for Xaml Islands. leveraging DispatcherQueueHelper in favor of DispatcherHelper.
Many other Xaml Islands related fixes, including a test project.
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Many controls don't work inside a Xaml Islands context.
What is the new behavior?
Some fixed for Xaml Islands, now leveraging DispatcherQueue whenever possible.
PR Checklist
Please check if your PR fulfills the following requirements: