-
Notifications
You must be signed in to change notification settings - Fork 28.2k
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
Update the mouse cursor handler to work with multi-view on Windows #163855
Update the mouse cursor handler to work with multi-view on Windows #163855
Conversation
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.
This seems reasonable 👍 Do you have an example app that demonstrates this working.
Also, I would let others review this in addition to myself, just to be safe.
Thanks. For an example, merge with |
@@ -983,6 +983,29 @@ std::optional<LRESULT> FlutterWindowsEngine::ProcessExternalWindowMessage( | |||
return std::nullopt; | |||
} | |||
|
|||
bool FlutterWindowsEngine::UpdateFlutterCursor( | |||
const std::string& cursor_name) const { |
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.
If I update the cursor, then add a new view, will that new view use the correct cursor? Could you add a test for this scenario?
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.
Since the cursor is a shared global state on Windows, the cursor isn't set when a view is added. Instead, it's updated later when the framework calls "activateSystemCursor" or "setCustomCursor/windows", as setting the cursor only affects the window that contains the cursor in its client area or the window capturing the mouse. Now that the engine sets the cursor for all existing views, the view with the cursor in it will display the correct cursor type.
I could add a test to verify that the engine properly sets the cursor for all existing views and that the global cursor state matches the specified one. Would that help/be enough?
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 apologize for the very late response. Feel free to ping me on Discord if I forget to follow-up on a pull request! :)
Since the cursor is a shared global state on Windows
Ah right! Reading win32's SetCursor
docs: https://learn.microsoft.com/windows/win32/api/winuser/nf-winuser-setcursor
The cursor is a shared resource. A window should set the cursor shape only when the cursor is in its client area or when the window is capturing mouse input. In systems without a mouse, the window should restore the previous cursor before the cursor leaves the client area or before it relinquishes control to another window.
Given the cursor is shared, it seems we should move cursor update/set logic up from the view and to the engine. Ideally we'd remove FlutterWindowsView::UpdateFlutterCursor
, FlutterWindowsView::SetFlutterCursor
, WindowBindingHandler::UpdateFlutterCursor
, WindowBindingHandler::SetFlutterCursor
and we'd add the LoadCursor
and SetCursor
win32 APIs to the WindowsProcTable
to allow tests to mock these APIs. If we do this, we don't need a test for the "new view uses the correct cursor" scenario. What do you think?
cc @dkwingsmt who worked on the desktop cursor implementation.
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.
Makes sense to me! I'll refactor as suggested -- no worries about the delay, this PR isn't blocking anything.
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.
Makes sense, but then does it mean we need to hook up the callbacks when Flutter windows gain focus and lose focus to set/reset the cursors? Is it currently supported?
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.
This already seems to be properly set up. The framework is notified of pointer enter/leave events, which occur when focus changes.
@loic-sharma There is an open question for you. Can you clarify or approve? |
I created a sample application and recorded a video to demonstrate cursor changes and custom cursors across multiple windows after the refactor. It uses the Video: Code: // Add to pubspec.yaml:
// dependencies:
// custom_mouse_cursor: ^1.1.2
import 'dart:ui';
import 'package:flutter/material.dart';
import 'package:custom_mouse_cursor/custom_mouse_cursor.dart';
late CustomMouseCursor iconCursor;
Future<void> initializeCursor() async {
const List<Shadow> shadows = [
BoxShadow(
color: Color.fromRGBO(0, 0, 0, 0.8),
offset: Offset(1, 1),
blurRadius: 3,
spreadRadius: 2,
),
];
iconCursor = await CustomMouseCursor.icon(
Icons.celebration,
size: 48.0,
hotX: 24,
hotY: 24,
color: Colors.blueAccent,
shadows: shadows,
);
}
void main() async {
const Size size = Size(400, 300);
final controller1 = RegularWindowController(size: size);
final controller2 = RegularWindowController(size: size);
await initializeCursor();
runWidget(ViewCollection(views: [
RegularWindow(
controller: controller1,
child: MyApp(otherController: controller2),
),
RegularWindow(
controller: controller2,
child: MyApp(otherController: controller1),
),
]));
}
class MyApp extends StatefulWidget {
MyApp({super.key, required this.otherController});
final RegularWindowController otherController;
final CustomMouseCursor cursor = iconCursor;
@override
MyAppState createState() => MyAppState();
}
class MyAppState extends State<MyApp> with TickerProviderStateMixin {
late AnimationController _controller;
@override
void initState() {
super.initState();
_controller = AnimationController(
vsync: this,
duration: const Duration(seconds: 4),
)..repeat(reverse: true);
}
@override
void dispose() {
_controller.dispose();
super.dispose();
}
void _focusOtherWindow() {
WidgetsBinding.instance.platformDispatcher.requestViewFocusChange(
viewId: widget.otherController.rootView.viewId,
direction: ViewFocusDirection.forward,
state: ViewFocusState.focused,
);
}
@override
Widget build(BuildContext context) {
return MaterialApp(
home: Scaffold(
appBar: AppBar(title: Text('View ${View.of(context).viewId}')),
body: Center(
child: Column(
mainAxisAlignment: MainAxisAlignment.center,
children: [
SizedBox(
height: 40,
child: LayoutBuilder(
builder: (context, constraints) {
return AnimatedBuilder(
animation: _controller,
builder: (context, child) {
return Stack(
children: [
Positioned(
left: _controller.value *
(constraints.maxWidth - 150),
child: SizedBox(
width: 150,
child: OutlinedButton(
onPressed: _focusOtherWindow,
child: Text(
'Focus View ${widget.otherController.isReady ? widget.otherController.rootView.viewId : 0}',
),
),
),
),
],
);
},
);
},
),
),
const SizedBox(height: 30),
MouseRegion(
cursor: widget.cursor, // Use widget's cursor property
child: GestureDetector(
child: Container(
decoration: const BoxDecoration(
color: Colors.white,
border: Border(
top: BorderSide(color: Colors.grey, width: 1),
bottom: BorderSide(color: Colors.grey, width: 1),
),
),
child: const Column(
mainAxisAlignment: MainAxisAlignment.spaceAround,
children: [
SizedBox(height: 8),
Row(
mainAxisAlignment: MainAxisAlignment.center,
children: [
Text("Hover me!", style: TextStyle(fontSize: 18)),
]),
SizedBox(height: 8),
],
),
),
),
),
],
),
),
),
);
}
} The animated button shows that the cursor changes even when the pointer remains still. It also shows that switching focus correctly updates the cursor in the new view. Previously, setting the cursor when no views were available was an error. Now that the engine calls |
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.
Very nice clean up and great test app! Excellent work 🥳
Partially fixes #142845.
This PR removes implicit view assumptions from the
'flutter/mousecursor'
channel handler on Windows.The cursor is now set for all existing views, ensuring that the current
FlutterWindow
with the cursor in its client area uses the correct cursor. The method arguments remain unchanged.Cursor handler tests have also been updated to avoid assuming an implicit view.
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.