microsoft / WindowsAppSDK

The Windows App SDK empowers all Windows desktop apps with modern Windows UI, APIs, and platform features, including back-compat support, shipped via NuGet.
https://docs.microsoft.com/windows/apps/windows-app-sdk/
MIT License
3.78k stars 319 forks source link

Proposal: Add WinRT APIs to create Windows.System.DispatcherQueueController on current thread easily #2466

Open roxk opened 2 years ago

roxk commented 2 years ago

I know there is a feature portal for proposal but this is very code-oriented/developer-experience related, so I wrote it here instead of the portal.

Summary

Now that WinUI 3 mica support is here in 1.1 preview 3, each app that implement mica need to have their own dispatcher queue helper as in the xaml control gallery, or in this sample for C++.

The ergonomic is not good in C# (dllimport), nor in C++ (using ABI namespace and C API directly for a release-defining feature).

This proposal suggests WASDK provide the following WinRT API:

namespace Microsoft.Windows.System
{
  runtimeclass DispatcherQueueController
  {
    // Note the return type is WS instead of Microsoft.UI.Dispatching
    static Windows.System.DispatcherQueueController CreateOnCurrentThread();
  }
}

So C# and C++ can all use the same API. In the future this would benefit other supported language projection as well.

On top of that, I'm guessing the existing Windows::System::DispatcherQueueController::CreateOnDedicatedThread is just wrapping the ABI interface and C API anyways. To make the API in WinRT complete, this proposal suggests that a complete WinRT API that wraps IDispatcherQueueController, DispatcherQueueOptions, and CreateDispatcherQueueController, as follows:

namespace Microsoft::Windows::System
{
  enum DispatcherQueueOptions
  {
    /*Map to existing DispatcherQueueOptions*/
  }
  runtimeclass DispatcherQueueController
  {
    static Windows.System.DispatcherQueueController CreateDispatcherQueueController(DispatcherQueueOptions options);
  }
}

This has the following benefits:

Examples Of Proposed API In The Mica Sample

C++

winrt::WS::DispatcherQueueController CreateSystemDispatcherQueueController()
    {
-        DispatcherQueueOptions options
-        {
-            sizeof(DispatcherQueueOptions),
-            DQTYPE_THREAD_CURRENT,
-            DQTAT_COM_NONE
-        };
-
-        ::ABI::Windows::System::IDispatcherQueueController* ptr{ nullptr };
-        winrt::check_hresult(CreateDispatcherQueueController(options, &ptr));
-        return { ptr, take_ownership_from_abi };
+        return Microsoft::Windows::System::DispatcherQueueController::CreateOnCurrentThread();
    }

C

class WindowsSystemDispatcherQueueHelper
{
-    [StructLayout(LayoutKind.Sequential)]
-    struct DispatcherQueueOptions
-    {
-        internal int dwSize;
-        internal int threadType;
-        internal int apartmentType;
-    }
-    
-    [DllImport("CoreMessaging.dll")]
-    private static extern int CreateDispatcherQueueController([In] DispatcherQueueOptions options, [In, Out, MarshalAs(UnmanagedType.IUnknown)] ref object dispatcherQueueController);
-
    object m_dispatcherQueueController = null;
    public void EnsureWindowsSystemDispatcherQueueController()
    {
        if (Windows.System.DispatcherQueue.GetForCurrentThread() != null)
        {
            // one already exists, so we'll just use it.
            return;
        }

-        if (m_dispatcherQueueController == null)
-        {
-            DispatcherQueueOptions options;
-            options.dwSize = Marshal.SizeOf(typeof(DispatcherQueueOptions));
-            options.threadType = 2;    // DQTYPE_THREAD_CURRENT
-            options.apartmentType = 2; // DQTAT_COM_STA
-
-            CreateDispatcherQueueController(options, ref m_dispatcherQueueController);
-        }
+        m_dispatcherQueueController = Microsoft.Windows.System.DispatcherQueueController.CreateOnCurrentThread();
    }
}

Scope

Capability Priority
This proposal will allow supported languages to use (the same) WinRT API to create Windows::System::DispatcherQueueController on current thread in 1 line Must
This proposal will allow supported languages to use WinRT API to create Windows::System::DispatcherQueueController with options Must

Notes

  1. IMO this should be implemented by 1.1. So when this feature is officially rolled out no one has to write dllimport/::ABI:: just to use mica.
  2. Note that the "existing" one in WinUI 3 doesn't work as it creates MUD instead of WS dispatcher queue controller.
  3. I'd love to submit a PR for this but I need to make sure the proposal is accepted before investing my time on it.

Potential Confusion

Developers might be confused about the difference between Windows::System::DispatcherQueueController, Microsoft::Windows::System::DispatcherQueueController (proposed, only contain static method to create WS::DispatcherQueueController), and Microsoft::UI::Dispatching::DispatcherQueueController, and which to use. We might need to provide a doc that explains them briefly. The doc might need to explain

Open question

Since the proposed class only contains static methods, is it more appropriate to name it DispatcherQueueControllerHelper?

sylveon commented 2 years ago

This already exists: https://docs.microsoft.com/en-us/windows/winui/api/microsoft.ui.dispatching.dispatcherqueuecontroller.createoncurrentthread

roxk commented 2 years ago
  1. The samples need to use it then
  2. Will update the proposal to wrap the API completely
sylveon commented 2 years ago

The options that DispatcherQueueOptions offers are:

I'm not sure why we need to expose it raw, when all that's missing from the native struct is the apartment type for dedicated threads, which could be instead simply added as an overload for CreateOnDedicatedThread.

roxk commented 2 years ago

Exposing it raw has the benefit that we won't miss any API that is available in C. A WinRT API that maps exactly to C API is also often how WASDK API is structured. Put it the other way, had Windows::System::DispatcherQueueController exposed everything, we wouldn't need WASDK to provide CreateOnDedicatedThread, and then perhaps need an overload for other apartment type in the future. It's more stable (require no future change unless needed) and complete.

Existing CreateOn*Thread API can still be provided for convenience, and the CreateDispatcherQueueController is the API for developers who want more controls/create based on other flows (reading from config, etc). Your suggestion of adding an overload doesn't conflict with/obsolete this proposal.

Bottomline: Make this API complete. Provide everything that the C API supports. Overload or exposing it raw, doesn't matter. Though I'd argue exposing it raw is more idiomatic/future-proof in the first place.

roxk commented 2 years ago

So it turns out the one in Microsoft.UI.Dispatching is returning Microsoft.UI.Dispatching.DispatcherQueueController, i.e. the lifted one. Mica needs the system one. Update the proposal back, and added more wordings to make it clear we want to work with the system one instead of the lifted one.

sylveon commented 2 years ago

It wouldnt be very future proof either way since any addition to the C API will require updating the WinRT APIs/types.

sylveon commented 2 years ago

Microsoft.Windows.System will most likely lead to confusion (e.g. "why do we have 3 different DispatcherQueueController classes", "why can't I create a M.W.S.DispatcherQueueController"). We already have a mess with the many different dispatchers in WinRT: CoreDispatcher, W.S.DispatcherQueue, M.UI.D.DispatcherQueue, the non-working Dispatcher property on WinUI 3 XAML controls, etc. Let's not contribute more to this mess.

This should probably be added directly to CsWinRT, so that it can be in Windows.System (e.g. under the class DispatcherQueueControllerInterop), and C can continue using CreateDispatcherQueueController, as the system DispatcherQueueController is effectively superseded by the one in Microsoft.UI.Dispatching, so unlikely to receive any improvements (not to mention those would be bound to system updates).

If you really need a Windows.System.DispatcherQueueController in a WASDK world, dropping to system APIs is not unexpected. The WinUI Mica example is demonstrating using a low-level API, not a high-level one, so interop is not unexpected. They're planning to add a simplier API like a brush or property in the future. So this is a temporary workaround for a (also temporary) minor inconvenience that will add technical debt in the future by confusing developers.

roxk commented 2 years ago

dropping to system APIs is not unexpected

Maybe to you, but not to me, and I'd bet to most other developers. It's confusing to developers and not a good developer experience. Does the API work? Yes. Is it pleasant? Not really. The platform can do better, and I'd like to contribute.

This should probably be added directly to CsWinRT

I'd like to see an elegant and simple API in C++ as well. So any solution that only works in CsWinRT but not C++ doesn't work. Unless we also implement similar interop in, say, WIL.

Not to mention this kind of short term solution might necessitate more effort in the future. The proposed API automatically work in e.g. rust if rust ever becomes supported. With a manual per-language solution we'd then lose feature parity and require more work to bring rust up-to-date.

effectively superseded by the one in Microsoft.UI.Dispatching, so unlikely to receive any improvements

I'm not sure why the fact that an API is "superseded" (and required for a release-defining feature in the brand new UI framework...And, is it even true?) has to do with the fact that using the API is unpleasant. If we were to meet developers where they are, we need to improve the developer experience. "It's a superseded API" is an excuse IMO. Now, assuming it's indeed superseded API and if the team think thus they cannot afford/do not want to invest time on this proposal. That at least is a valid reason, although I'd be very disappointed as 1.1 mica's API is awful and that hurts WASDK's reputation and usability as a whole.

I'd agree the naming can be confusing though. And my suggested solution is

It works for other fields, e.g. Android and AndroidX, where AndroidX is a similar decoupling/lifted API effort. As an example, Context's AndroidX helper couterpart is ContextCompat. Maybe the class name in this proposal can be Microsoft::Windows::System::DispatcherQueueControllerHelper (as asked in the open question section).

riverar commented 2 years ago

Can you clean up your proposal by deleting all the irrelevant bits/edits? I'm very confused as to what you're proposing. (The API already exists.)

sylveon commented 2 years ago

The proposed API automatically work in e.g. rust if rust ever becomes supported. With a manual per-language solution we'd then lose feature parity and require more work to bring rust up-to-date.

Rust already has several easy ways of calling CreateDispatcherQueueController, and I doubt the language will be supported before we get a brush/property for acrylic/mica.

and required for a release-defining feature in the brand new UI framework...

This is the bug here. That feature shouldn't require it. Or it should create one if required.

has to do with the fact that using the API is unpleasant

Before today, that API was only used for very niche scenarios. In 99% of usage scenarios, a DispatcherQueue already existed for the thread you are on. Whatever niche scenario required this is already far past not wanting to do an interop 4 liner.

roxk commented 2 years ago

Can you clean up your proposal by deleting all the irrelevant bits/edits?

Temporarily removed edits. Hope no one ask "what changed in the proposal"?😅

(The API already exists.)

That one is for creating the controller in Microsoft.UI.Dispatching(doc), whereas this proposal suggest creating similar APIs for the one in Windows.System (doc), which is needed for mica in WinUI 3 1.1 (at least preview 3).

riverar commented 2 years ago

@roxk History is maintained for edits and can be accessed by clicking the "edited" text.

image

riverar commented 2 years ago

I must reject the proposal of adding a factory function to Microsoft.Windows.System.DispatcherQueueController for an unrelated type in the Windows.System namespace. This could impose undue namespace requirements on folks consuming this class and can be extremely confusing to developers.

Can you find another home for it? Perhaps the proposal could be modified to provide, say, an optional Microsoft.Windows.Platform.Helpers class that hosts a CreateWindowsSystemDispatcherQueueControllerForCurrentThread?

jonwis commented 2 years ago

@jeffstall or @rkarman can you add more Dispatcher people here?

This seems like it would be a candidate for a Really Great Sample of using the Windows APIs, linked to from the docs.microsoft.com content about dispatcher queues.

roxk commented 2 years ago

edits and can be accessed by clicking the "edited" text.

TIL. Thanks!

Can you find another home for it? Perhaps the proposal could be modified to provide, say, an optional Microsoft.Windows.Platform.Helpers class

An alternative name, Microsoft.Windows.System.DispatcherQueueControllerHelper is suggested in the open question section. I prefer to be more specific in the namespace and name so that future helpers classes can be found in their respective namespace, with a distinct name. Bottomline, we should avoid a God helper class that does everything.

I'm open to other naming suggestions though!

jamespack commented 1 year ago

A simpler API for this would be great. You guys have a much better chance at getting the options right then we do. As an example, the documentation for the options don't mention that not setting the right options could cause a crash on close that is relatively difficult to diagnose.