Skip to content

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

}

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 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
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 25, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 25, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 26, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 26, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 26, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 26, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 26, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 26, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 26, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 26, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 27, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 27, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 27, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 27, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 27, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 27, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 27, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 27, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 20, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 20, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 21, 2025
romanejaquez pushed a commit to romanejaquez/flutter that referenced this pull request Aug 14, 2025
…lutter#163855)

Partially fixes flutter#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

- [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], including [Features
we expect every widget to implement].
- [x] I signed the [CLA].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [x] All existing and new tests are passing.

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

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
[test-exempt]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes
[Discord]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md
[Data Driven Fixes]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
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 related. 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