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

feat: add custom cursor interface on windows #36143

Merged

Conversation

Kingtous
Copy link
Contributor

@Kingtous Kingtous commented Sep 14, 2022

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 flutter/flutter#31952 (comment)
flutter/flutter#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.

Screenshot_20220914_183105

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

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • 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.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

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

@flutter-dashboard
Copy link

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
Copy link

google-cla bot commented Sep 14, 2022

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 Kingtous force-pushed the feat/windows/custom_hcursor_support branch 4 times, most recently from 66d2190 to 93434f3 Compare September 14, 2022 12:21
@Kingtous Kingtous force-pushed the feat/windows/custom_hcursor_support branch from 0d14ab9 to ad36c9e Compare October 26, 2022 01:27
@Kingtous
Copy link
Contributor Author

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

@Kingtous Kingtous changed the title fix: add custom cursor interface on windows feat: add custom cursor interface on windows Oct 26, 2022
@loic-sharma
Copy link
Member

loic-sharma commented Oct 26, 2022

@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
Copy link
Contributor

dkwingsmt commented Oct 26, 2022

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
Copy link
Contributor Author

Kingtous commented Oct 27, 2022

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 flutter/flutter#31952 (comment) flutter/flutter#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
Copy link
Contributor Author

@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 requested review from loic-sharma and dkwingsmt and removed request for loic-sharma and dkwingsmt October 27, 2022 03:10
@Kingtous
Copy link
Contributor Author

@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?

shell/platform/windows/cursor_handler.cc Outdated Show resolved Hide resolved
shell/platform/windows/cursor_handler.cc Outdated Show resolved Hide resolved
shell/platform/windows/cursor_handler.cc Outdated Show resolved Hide resolved
shell/platform/windows/cursor_handler.cc Outdated Show resolved Hide resolved
shell/platform/windows/cursor_handler.cc Outdated Show resolved Hide resolved
shell/platform/windows/cursor_handler.cc Outdated Show resolved Hide resolved
shell/platform/windows/cursor_handler.cc Outdated Show resolved Hide resolved
shell/platform/windows/cursor_handler.h Outdated Show resolved Hide resolved
shell/platform/windows/cursor_handler.h Outdated Show resolved Hide resolved
shell/platform/windows/cursor_handler_unittests.cc Outdated Show resolved Hide resolved
@dkwingsmt
Copy link
Contributor

dkwingsmt commented Oct 28, 2022

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

@dkwingsmt
Copy link
Contributor

dkwingsmt commented Nov 22, 2022

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

@Kingtous
Copy link
Contributor Author

Kingtous commented Nov 22, 2022

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. :)

@dkwingsmt dkwingsmt added autosubmit Merge PR when tree becomes green via auto submit App and removed needs tests labels Nov 22, 2022
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 22, 2022
@auto-submit
Copy link
Contributor

auto-submit bot commented Nov 22, 2022

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
Copy link
Contributor

auto-submit bot commented Nov 22, 2022

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

@dkwingsmt
Copy link
Contributor

@stuartmorgan Would you like to give a second review?

@Kingtous Kingtous requested review from dkwingsmt and removed request for loic-sharma November 28, 2022 03:41
@dkwingsmt
Copy link
Contributor

@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
Copy link

How long will this take?

@chinmaygarde
Copy link
Member

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

@dkwingsmt dkwingsmt added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 1, 2022
@auto-submit auto-submit bot merged commit fa24f94 into flutter:main Dec 1, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 1, 2022
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Dec 1, 2022
…116387)

* fa24f943a feat: add custom cursor interface on windows (flutter/engine#36143)

* 1603fa1bb Remove glitches when scrolling on old Android TV devices (flutter/engine#37493)
mit-mit pushed a commit to mit-mit/flutter that referenced this pull request Dec 6, 2022
…lutter#116387)

* fa24f943a feat: add custom cursor interface on windows (flutter/engine#36143)

* 1603fa1bb Remove glitches when scrolling on old Android TV devices (flutter/engine#37493)
shogohida pushed a commit to shogohida/flutter that referenced this pull request Dec 7, 2022
…lutter#116387)

* fa24f943a feat: add custom cursor interface on windows (flutter/engine#36143)

* 1603fa1bb Remove glitches when scrolling on old Android TV devices (flutter/engine#37493)
Kingtous added a commit to Kingtous/engine that referenced this pull request Dec 11, 2022
* feat: initial custom cursor and cache support

* opt: add cursor validity check

* opt: format code

* opt: rename more suitable name

* opt: key, method name

* opt: code

* feat: add unittests

* opt: change to ref

* opt: docs

* opt: format code

* opt: code

* opt: correct code and comments

* opt: rgba -> bgra

* opt: name

* opt: create valid/compatible mask of bitmap & mask

* opt: null check

* opt: split cursor functions into configurable ones

* fix: EncodableValue type

* opt: doc for custom cursor key

* update: cursor doc

* opt: docs

* opt: refactor vector cache to unordered_map

* opt: code

* opt: unifrom key value

* opt: uniform key_iter to name_iter

* opt: docs

* fix: get width out of range
gspencergoog pushed a commit to gspencergoog/flutter that referenced this pull request Jan 19, 2023
…lutter#116387)

* fa24f943a feat: add custom cursor interface on windows (flutter/engine#36143)

* 1603fa1bb Remove glitches when scrolling on old Android TV devices (flutter/engine#37493)
Kingtous added a commit to Kingtous/engine that referenced this pull request Jan 26, 2023
* feat: initial custom cursor and cache support

* opt: add cursor validity check

* opt: format code

* opt: rename more suitable name

* opt: key, method name

* opt: code

* feat: add unittests

* opt: change to ref

* opt: docs

* opt: format code

* opt: code

* opt: correct code and comments

* opt: rgba -> bgra

* opt: name

* opt: create valid/compatible mask of bitmap & mask

* opt: null check

* opt: split cursor functions into configurable ones

* fix: EncodableValue type

* opt: doc for custom cursor key

* update: cursor doc

* opt: docs

* opt: refactor vector cache to unordered_map

* opt: code

* opt: unifrom key value

* opt: uniform key_iter to name_iter

* opt: docs

* fix: get width out of range
Kingtous added a commit to Kingtous/flutter_custom_cursor that referenced this pull request Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-windows
Projects
None yet
6 participants