fix: add custom cursor interface on windows

[Kingtous]

This PR adds an additional interface to set custom cursor on windows, which fixes issues mentioned below.

Currently, Customizing MouseCursor on windows is not working, the customed mouse will restore to arrow once it moves. See https://github.com/flutter/flutter/issues/31952#issuecomment-1056115014 https://github.com/flutter/flutter/issues/99484 for details. Variable current_cursor_ in cursor handler will not update when we set our custom cursors, and we cannot change this field to prevent the cursor from restoring due to lacking of interface.

The reason is that cursor handler on windows has no interface to change current cursor to custom cursor, only support setting windows self-contained icons.

Changes:

• add new function handled by method channel of SystemCursors.mousecursors, which can change current_cursor_, make possible to handle custom cursor on windows correctly.

We have successfully tested this interface in https://github.com/Kingtous/rustdesk_flutter_custom_cursor/blob/8f3be4ac68b4fb20d05b8d30df4c13d8fd77f9c4/lib/flutter_custom_cursor.dart#L113, which works perfectly on windows.

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

• [x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
• [x] I read the Tree Hygiene wiki page, which explains my responsibilities.
• [x] I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
• [x] I listed at least one issue that this PR fixes in the description above.
• [ ] I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
• [x] I updated/added relevant documentation (doc comments with ///).
• [x] I signed the CLA.
• [x] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[flutter-dashboard[bot]]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[google-cla[bot]]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[Kingtous]

Thanks for the review, all comments has resolved. Is there other jobs I need to do? :)

[loic-sharma]

@Kingtous Thanks for following up quickly! Could you add a unit test that sends a setSystemCursor message and uses MockWindowBindingHandler to verify the cursor is set as expected? (If you want to go the extra mile, ideally activateSystemCursor would be tested too, but feel free to skip that!)

Once that's ready, we should get approval from @dkwingsmt as they worked in this space.

[dkwingsmt]

Thank you. I didn't know you can create ICONINFO using a buffer.

However, I'm hesitant on merging the method channel, since we are hoping to design a universal API (method channel) for all platforms, and the current method is likely platform dependent. But I also understand that we need to develop one step at a time. So I think there are two options:

1. Make this PR only includes embedding APIs (such as SetFlutterCursor), just enough for you to develop a plugin. The plugin will listen to the channel, create ICONINFO, and call the new embedding APIs.
2. Merge this PR, but we might deprecate these methods in the future. (When we come up with a universal API, we'll probably adapt the current legacy API onto the new one for a while for compatibility.)

Also, the flow I'm imagining is, instead of setting the cursor using the buffer directly, which you have to do every time you set the same cursor,

1. Create a cursor using the buffer, and assign it with a name (string)
2. Activate the cursor using the string.

[Kingtous]

Thank you. I didn't know you can create ICONINFO using a buffer.

However, I'm hesitant on merging the method channel, since we are hoping to design a universal API (method channel) for all platforms, and the current method is likely platform dependent. But I also understand that we need to develop one step at a time. So I think there are two options:

1. Make this PR only includes embedding APIs (such as SetFlutterCursor), just enough for you to develop a plugin. The plugin will listen to the channel, create ICONINFO, and call the new embedding APIs.
2. Merge this PR, but we might deprecate these methods in the future. (When we come up with a universal API, we'll probably adapt the current legacy API onto the new one for a while for compatibility.)

Also, the flow I'm imagining is, instead of setting the cursor using the buffer directly, which you have to do every time you set the same cursor,

1. Create a cursor using the buffer, and assign it with a name (string)
2. Activate the cursor using the string.

Sounds great for a universal API to change cursor, which is a very important feature, especially for games. Although this PR is providing a new feature, it more like a bugfix. It fills the limitation(or fix bugs?) with custom cursor on windows, currently.(by extending offcial MouseCursor, MouseCursorSession abstract class, linux/macOS works great)

This problem has blocked so many developers as mentioned in https://github.com/flutter/flutter/issues/31952#issuecomment-1056115014 https://github.com/flutter/flutter/issues/99484

I think the two options are all ok, and I prefer Option 2. Currently, this PR can be a temporary and fast workaround to uniform custom cursor behaviors on all platforms. Devs can change custom cursor easily by providing a raw BGRA buffer, without any win32 knowledge.

here are some questions: Option 1: I am not very familiar with embedding API, can it be invoked without a MethodChannel?

I've thought about custom cursor cache, but it may bring a more complex custom cursor logic, which may not suitable for keeping this embedding API simple. Also, a cursor buffer is not so big, it only is invoked when the system cursor hovers on the specific item.

[Kingtous]

@Kingtous Thanks for following up quickly! Could you add a unit test that sends a setSystemCursor message and uses MockWindowBindingHandler to verify the cursor is set as expected? (If you want to go the extra mile, ideally activateSystemCursor would be tested too, but feel free to skip that!)

Once that's ready, we should get approval from @dkwingsmt as they worked in this space.

I noticed that cursor_handler.cc does not have unittest now. btw, can I manually generate a dummy cursor (buffer are all zero) for this test? I think a pure binary cursor is not suitable for the unittest.

[Kingtous]

@Kingtous Thanks for following up quickly! Could you add a unit test that sends a setSystemCursor message and uses MockWindowBindingHandler to verify the cursor is set as expected? (If you want to go the extra mile, ideally activateSystemCursor would be tested too, but feel free to skip that!)

Once that's ready, we should get approval from @dkwingsmt as they worked in this space.

I've extracted the cursor creation logic to a function named GetCursorFromBuffer. And I added a unittest which will check if the HCURSOR is not null. I think this approach may keep this unit tests simple?

[dkwingsmt]

@stuartmorgan What do you think? (Also I'm not familiar enough with the capability of plugins.)

[Kingtous]

Thanks for the careful review. All suggestions have resolved :)

[stuartmorgan]

@stuartmorgan What do you think? (Also I'm not familiar enough with the capability of plugins.)

About what part in particular?

If the goal is to just land something that would allow a plugin to do their own cursor handling, the best approach would be to add a Flutter-view-level version of the top-level window proc delegation (see https://github.com/flutter/flutter/issues/53168#issuecomment-676531731, the only reason I didn't do it is that we didn't have a use case any more). It should be easy—it would look just like this, but for the Flutter view HWND instead of the top-level HWND—and it's a general-purpose API that may well have other uses in the future. If that were added, a plugin could just handle WM_SETCURSOR to do their own cursor management.

[Kingtous]

@stuartmorgan What do you think? (Also I'm not familiar enough with the capability of plugins.)

About what part in particular?

If the goal is to just land something that would allow a plugin to do their own cursor handling, the best approach would be to add a Flutter-view-level version of the top-level window proc delegation (see flutter/flutter#53168 (comment), the only reason I didn't do it is that we didn't have a use case any more). It should be easy—it would look just like this, but for the Flutter view HWND instead of the top-level HWND—and it's a general-purpose API that may well have other uses in the future. If that were added, a plugin could just handle WM_SETCURSOR to do their own cursor management.

Yes, the problems is due to the issue: Currently plugins cannot get WM_SETCURSOR message from flutter because of this. https://github.com/flutter/engine/blob/243019f734a29acf13c87dfbf9dc12ee03a2d54d/shell/platform/windows/window.cc#L400-L408.

Flutter intercepts WM_SETCURSOR message and do OnSetCursor, then return TRUE to prevent message transfer down among plugins. Then the cursor will change to the cursor which created by flutter engine before.

We tried to handle WM_SETCURSOR in the plugins, but the handler never reaches. https://github.com/Kingtous/rustdesk_flutter_custom_cursor/blob/f0c8f4cdbcf50fa9224bc4780c63fd577ba47f15/windows/flutter_custom_cursor_plugin.cpp#L72

Although this PR is providing a new feature, it more like a bugfix. It fills the limitation(or fix bugs?) with custom cursor on windows engine, currently.(by extending offcial MouseCursor, MouseCursorSession abstract class, linux/macOS works great)

Here are some clues why let plugins to handle WM_SETCURSOR is not a good way: Now, OnSetCursor methods cannot be removed here, because it must be here to keep cursor behavior normally. IF WE RETURN FALSE here, although plugins can handle this type of message, the cursor will not consistent. Assuming we return FALSE, when we set a custom cursor in plugins:

• set custom cursor in plugin(to simplify, we call the custom cursor CC, now cursor is CC)
• flutter get WM_SETCURSOR, call OnSetCursor, cursor will change to default Arrow cursor. the WM_SETCURSOR continues to transfer down.(now cursor is Arrow)
• the plugin get WM_SETCURSOR, set custom cursor in plugin again. (Arrow-> CC)

So the cursor will change to arrow between two frames.

The key problem that causes this issue is that Windows need to set cursor twice to successfully get specific cursor work, which is different from macOS/Linux. (So our plugins can handle custom cursors in macOS/Linux greatly)

To solve this problem gracefully, I think the best way is adding a method channel to change cursor_ in flutter engine(which will be used to set cursor on OnSetCursor). So we can reuse current logic to keep custom cursor working, and keep engine simple and reuseable.

[stuartmorgan]

Flutter intercepts WM_SETCURSOR message and do OnSetCursor, then return TRUE to prevent message transfer down among plugins.

Flutter isn't "intercepting" it, it's handling it at the level of the Flutter view, which is the right place for WM_SETCURSOR to be handled.

We tried to handle WM_SETCURSOR in the plugins, but the handler never reaches.

That's expected, because you're setting a handler for the top-level HWND, not the Flutter view HWND. Which is why my comment above is saying that the general fix would be to add plugin API to allow registering Flutter view HWND handlers in addition to top-level HWND handlers. Such a handler would be called before the engine code you linked to.

Here are some clues why let plugins to handle WM_SETCURSOR is not a good way: Now, OnSetCursor methods cannot be removed here, because it must be here to keep cursor behavior normally.

My suggestion does not involve removing OnSetCursor.

IF WE RETURN FALSE here, although plugins can handle this type of message, the cursor will not consistent.

My suggestion does not involve changing the return value of the existing code.

Assuming we return FALSE, when we set a custom cursor in plugins:

• set custom cursor in plugin(to simplify, we call the custom cursor CC, now cursor is CC)
• flutter get WM_SETCURSOR, call OnSetCursor, cursor will change to default Arrow cursor. the WM_SETCURSOR continues to transfer down.(now cursor is Arrow)
• the plugin get WM_SETCURSOR, set custom cursor in plugin again. (Arrow-> CC)

That is not the flow that would happen with my suggested solution. The flow would be:

• The plugin gets WM_SETCURSOR (via the handler it registers with the new RegisterFlutterViewWindowProcDelegate, or whatever we call it), sets the cursor to CC, and returns TRUE because it handled it.
• The engine's WM_SETCURSOR handling is never called because the plugin code, which ran first, returned TRUE.

To solve this problem gracefully, I think the best way is adding a method channel to change cursor_ in flutter engine(which will be used to set cursor on OnSetCursor). So we can reuse current logic to keep custom cursor working, and keep engine simple and reuseable.

I can't speak to whether adding the method channel code proposed here is a good idea; @dkwingsmt raised concerns with it above, and knows far more about the future plans here than I do. But if @dkwingsmt would prefer to enable plugin-level solutions for now instead, it could be done very easily, by adding a general-purpose API that I had always intended to add if there was ever a use case for it—and in fact almost added previously for this exact use case.

[Kingtous]

Thanks for the reply!

Here are some clues why let plugins to handle WM_SETCURSOR is not a good way: Now, OnSetCursor methods cannot be removed here, because it must be here to keep cursor behavior normally.

It's is just my inspiration for adding a method channel to change cursor_ field, not considering to let WM_SETCURSOR return TRUE :).

The plugin gets WM_SETCURSOR (via the handler it registers with the new RegisterFlutterViewWindowProcDelegate, or whatever we call it), sets the cursor to CC, and returns TRUE because it handled it. The engine's WM_SETCURSOR handling is never called because the plugin code, which ran first, returned TRUE.

Sounds like a more generic solution to solve this problem than I proposed. I dont know which is a better solution for it. As u mentioned, there are few use cases to register a FlutterView level window delegate.

However, I'm hesitant on merging the method channel, since we are hoping to design a universal API (method channel) for all platforms, and the current method is likely platform dependent. But I also understand that we need to develop one step at a time. So I think there are two options:

I think @dkwingsmt may want to integrate cursor logic in the engine and develops a universal custom cursor API for all platforms (an engine-level solution for customizing cursors?).

btw, if needed to add this RegisterFlutterViewWindowProcDelegate or revise this pr, I am willing to help :)

[Kingtous]

Any good news here about this issue?

I've split the setCustomCursor channel be 3 ones (createCustomCursor, deleteCustomCursor, setCustomCursor), just like @dkwingsmt mentioned here, which gives a better way to handle customizing cursors.

Also, the flow I'm imagining is, instead of setting the cursor using the buffer directly, which you have to do every time you set > the same cursor,

Create a cursor using the buffer, and assign it with a name (string) Activate the cursor using the string.

• createCustomCursor: used to create a custom cursor, if successful, a unique index(key) is returned with result->Success
• deleteCustomCursor: used to delete a custom cursor from the cache with the provided index.
• setCustomCursor: used to set a custom cursor from the cache.

The reason I didn't choose to use string for keys is: int64_t index comparison is more efficient than string ones, and it can be used to get HCURSOR directly by an offset.

Looking forward to your kind review and response of this bugfix :)

[dkwingsmt]

My apology for being late. The PR looks generally OK. I think the method channel is a good enough implementation for now since we're bound to make the universal API compatible anyway.

However, I'm not sure about using vector indexes as cursor ID (see comment). I suggest letting the framework provide the ID in the method call argument (instead of generating it from the engine and return it) and mapping the ID to the cursors in a map.

[Kingtous]

My apology for being late. The PR looks generally OK. I think the method channel is a good enough implementation for now since we're bound to make the universal API compatible anyway.

However, I'm not sure about using vector indexes as cursor ID (see comment). I suggest letting the framework provide the ID in the method call argument (instead of generating it from the engine and return it) and mapping the ID to the cursors in a map.

Thanks for the kind reply and careful review. I will prepare the refactor for providing key from the framework as soon as possible.

[Kingtous]

Appreciated. To ensure it's worked with the latest master channel of flutter. I'll test the commit with our custom cursor plugin again. Sorry for a little delay :)

[dkwingsmt]

No problem, let me know when it's ready :) Thank you so much for working on this!

[Kingtous]

No problem, let me know when it's ready :) Thank you so much for working on this!

I've tested this commit with the latest flutter/engine, it works perfectly with https://github.com/Kingtous/rustdesk_flutter_custom_cursor/blob/refactor/flutter/master/lib/cursor_manager.dart

The test cursor has successfully shown without a delay or an error.

All comments have been resolved now.

Later I consider to publish this custom plugin to pub.dev to make customizing cursors on flutter easier, which brings convinience for all flutter devs. Thanks. :)

[auto-submit[bot]]

auto label is removed for flutter/engine, pr: 36143, due to - Please get at least one approved review if you are already a member or two member reviews if you are not a member before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

[auto-submit[bot]]

auto label is removed for flutter/engine, pr: 36143, due to Validations Fail.

[dkwingsmt]

@stuartmorgan Would you like to give a second review?

[dkwingsmt]

@Kingtous I've approved this PR, but it is required that each PR needs the participance of 2 members of flutter/flutter-hackers, or in the case of this PR, 2 approvals. I'm looking for a second reviewer.

[SahajRana]

How long will this take?

[chinmaygarde]

@dkwingsmt This looks good to go. Please CQ when ready :)