System mouse cursor: Android #18569
System mouse cursor: Android #18569
Conversation
| * This method guarantees to return a non-null object. | ||
| */ | ||
| private PointerIcon resolveSystemCursor(@NonNull String kind) { | ||
| if (MouseCursorPlugin.systemCursorConstants == null) { |
There was a problem hiding this comment.
Since resolveSystemCursor is an instance method and cannot be called unless the constructor is called first, we could initialize systemCursorConstants in the constructor and avoid the if null check on every call.
There was a problem hiding this comment.
My concern is that this map will be expanded to ~70 cursors in the future, which will not be used by most mobile devices (since they don't set cursors at all). I want to avoid this overhead when unnecessary but this might indeed be a premature optimization. Do you still think it's fine to initialize it in the constructor unconditionally?
There was a problem hiding this comment.
Ahh, in that case, a comment would be plenty to justify this to future readers :)
shell/platform/android/io/flutter/plugin/mouse/MouseCursorPlugin.java
Outdated
Show resolved
Hide resolved
shell/platform/android/io/flutter/embedding/engine/systemchannels/MouseCursorChannel.java
Outdated
Show resolved
Hide resolved
shell/platform/android/io/flutter/embedding/engine/systemchannels/MouseCursorChannel.java
Outdated
Show resolved
Hide resolved
|
Implementation roughly looks good, this change is missing tests. |
shell/platform/android/io/flutter/embedding/android/FlutterView.java
Outdated
Show resolved
Hide resolved
shell/platform/android/io/flutter/plugin/mouse/MouseCursorPlugin.java
Outdated
Show resolved
Hide resolved
| } | ||
| }), | ||
| methodResult); | ||
| verify(testView, times(1)).getSystemPointerIcon(PointerIcon.TYPE_TEXT); |
There was a problem hiding this comment.
Can we also test the other cases here as well? Should be simple to replicate.
There was a problem hiding this comment.
That’ll be 70 cases in the future though, and they’re basically the conversion map written a second time. I think the conversion map itself should suffice.
|
Minor test nit, otherwise LGTM! |
3 participants
Description
This PR adds system mouse cursor to the Android engine.
This PR adds only a limited pool of system cursors. More will be added when the system is set up.
Related Issues