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

Update the mouse cursor handler to work with multi-view on Windows #163855

Merged
merged 8 commits into from
Mar 13, 2025

Conversation

hbatagelo
Copy link
Contributor

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.

@github-actions github-actions bot added engine flutter/engine repository. See also e: labels. platform-windows Building on or for Windows specifically a: desktop Running on desktop labels Feb 21, 2025
Copy link
Contributor

@mattkae mattkae left a 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.

@hbatagelo
Copy link
Contributor Author

Thanks. For an example, merge with canonical:foundation and run the reference app. Hover over the "Regular" push button. The system cursor should change from the default arrow (IDC_ARROW) to a hand (IDC_HAND). Now, press the button to open a regular window with a new view. In the new window, the mouse cursor should also change to a hand when you hover over the push button (which it didn't before).

@@ -983,6 +983,29 @@ std::optional<LRESULT> FlutterWindowsEngine::ProcessExternalWindowMessage(
return std::nullopt;
}

bool FlutterWindowsEngine::UpdateFlutterCursor(
const std::string& cursor_name) const {
Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Member

@loic-sharma loic-sharma Mar 11, 2025

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

@chinmaygarde
Copy link
Member

@loic-sharma There is an open question for you. Can you clarify or approve?

@loic-sharma loic-sharma requested a review from dkwingsmt March 11, 2025 16:04
@hbatagelo
Copy link
Contributor Author

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 custom_mouse_cursor package along with the windowing API from canonical:foundation-plus-framework.

Video:
cursor_multi_view.webm

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 SetCursor directly, SetFlutterCursor and UpdateFlutterCursor no longer trigger errors when no views are present.

@hbatagelo hbatagelo requested a review from loic-sharma March 13, 2025 17:17
Copy link
Member

@loic-sharma loic-sharma left a 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 🥳

@github-actions github-actions bot added the framework flutter/packages/flutter repository. See also f: labels. label Mar 13, 2025
@hbatagelo hbatagelo enabled auto-merge March 13, 2025 21:04
@hbatagelo hbatagelo added this pull request to the merge queue Mar 13, 2025
Merged via the queue into flutter:master with commit 02f3ddf Mar 13, 2025
169 of 170 checks passed
@hbatagelo hbatagelo deleted the cursor-handler-multi-view branch March 13, 2025 22:39
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 16, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 16, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 16, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 17, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: desktop Running on desktop engine flutter/engine repository. See also e: labels. framework flutter/packages/flutter repository. See also f: labels. platform-windows Building on or for Windows specifically
Projects
None yet
Development

Successfully merging this pull request may close these issues.

☂️ Multi View for Windows/MacOS
5 participants