-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Optimizations/refactoring/tweaks to DispatcherQueueHelper #3498
Optimizations/refactoring/tweaks to DispatcherQueueHelper #3498
Conversation
The previous version caused the C# compiler to always instantiate a display class for the closure even if the execution had thread access
Thanks Sergio0694 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 🙌 |
@@ -0,0 +1,251 @@ | |||
// Licensed to the .NET Foundation under one or more agreements. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you git mv this file as I'm surprised it didn't mark it as a move...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just moved it as usual through Visual Studio, not sure why it wasn't picked up 🤔
Maybe it's because I also renamed the file in the process?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You must move first [commit] and then rename. You can do both in one commit
Microsoft.Toolkit.Uwp.Connectivity/BluetoothLEHelper/BluetoothLEHelper.cs
Show resolved
Hide resolved
This PR has been marked as "needs attention 👋" and awaiting a response from the team. |
This PR has been marked as "needs attention 👋" and awaiting a response from the team. |
Hello @michael-hawker! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
@azchohfi think we're all good here now? |
Follow up for #3206
PR Type
What kind of change does this PR introduce?
no api changes)What is the new behavior?
This PR includes a number of optimizations, tweaks and refactorings to
DispatcherQueueHelper
:null
checks forfunction
, and enabled nullability annotations. Thosenull
checks were unnecessary since there were also other parameters that could also benull
(ie. the actualDispatcherQueue
) 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.DispatcherQueueExtensions
, since these are all extensions and not a helper class.Extensions
namespaceEnqueueAsync
to mirror theTryEnqueue
name of the actual API in theDispatcherQueue
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.TryEnqueue
fails, so if that happens we now return a wrappedInvalidOperationException
in theTask
, 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:
Pull Request has been submitted to the documentation repository instructions. 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 templatesTests for the changes have been added (for bug fixes / features) (if applicable)