Skip to content

Report a correct display ID in the window metrics event on win32 #174156

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Aug 21, 2025
Merged

Report a correct display ID in the window metrics event on win32 #174156

merged 8 commits into from
Aug 21, 2025
+111 −35

Conversation

mattkae
Copy link
Contributor

@mattkae mattkae commented Aug 20, 2025

What's new?

Now that we're sending down "true" display information to the framework, we also need to send down the correct display id in the WindowMetricsEvent for the view so that the display resolves properly. This pull request adds that :)

How to test

Here's a silly app that makes sure that our Display is legit:

// Copyright 2014 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'package:flutter/widgets.dart';

void main() => runApp(W());

class W extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    final x = View.of(context);
    print(x.display.id);
    return const Center(
      child: Text('Hello, world!', key: Key('title'), textDirection: TextDirection.ltr),
    );
  }
}

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

@mattkae mattkae requested a review from loic-sharma August 20, 2025 20:16
@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 Aug 20, 2025
gemini-code-assist[bot]
gemini-code-assist bot reviewed Aug 20, 2025
View reviewed changes
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request correctly adds the display ID to the WindowMetricsEvent on Win32. The refactoring in DisplayMonitor to extract FromMonitor is a good improvement for code clarity and reuse. The changes are applied consistently across the codebase, including updates to constructors, tests, and mocks. I have one suggestion to improve the fallback logic in FlutterWindow::GetDisplay for better logging and clarity in an edge case.

loic-sharma
loic-sharma reviewed Aug 20, 2025
View reviewed changes
engine/src/flutter/shell/platform/windows/flutter_window.cc
@@ -76,9 +77,11 @@ static uint64_t ConvertWinButtonToFlutterButton(UINT button) {
FlutterWindow::FlutterWindow(
int width,
int height,
std::shared_ptr<DisplayMonitor> const& display_monitor,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm maybe we should rename this to DisplayManager. "Monitor" is a bit on an overloaded term here, and we're using it for more than just listening to changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we do that in another PR? Just want to limit the scope, I can add that to my list as a follow up

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup that sounds good to me 👍

@mattkae mattkae requested a review from loic-sharma August 20, 2025 20:26
loic-sharma
loic-sharma reviewed Aug 20, 2025
View reviewed changes
@mattkae mattkae requested a review from loic-sharma August 20, 2025 21:19
loic-sharma
loic-sharma reviewed Aug 20, 2025
View reviewed changes
loic-sharma
loic-sharma reviewed Aug 20, 2025
View reviewed changes
loic-sharma
loic-sharma approved these changes Aug 20, 2025
View reviewed changes
Copy link
Member

@loic-sharma loic-sharma left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this! LGTM but please make sure there's a test somewhere that the display ID is plumbed to the window metric: https://github.com/flutter/flutter/pull/174156/files#r2289414502

@mattkae mattkae enabled auto-merge August 21, 2025 13:21
@mattkae mattkae added this pull request to the merge queue Aug 21, 2025
auto-merge was automatically disabled August 21, 2025 21:18

Pull Request is not mergeable

Merged via the queue into flutter:master with commit 4d4a69f Aug 21, 2025
184 checks passed
@mattkae mattkae deleted the display-id-in-win32-metrics branch August 21, 2025 23:30
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 22, 2025
@engine-flutter-autoroll
4d4a69f14 Report a correct display ID in the window metrics event on …
c0cb7fd 
…win32 (flutter/flutter#174156)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 22, 2025
Closed
SydneyBao pushed a commit to SydneyBao/flutter that referenced this pull request Aug 22, 2025
@mattkae @SydneyBao
Report a correct display ID in the window metrics event on win32 (flu…
4d9d821 
…tter#174156)

## What's new?
Now that we're sending down "true" display information to the framework,
we also need to send down the correct display id in the
WindowMetricsEvent for the view so that the display resolves properly.
This pull request adds that :)

## How to test
Here's a silly app that makes sure that our `Display` is legit:

```dart
// Copyright 2014 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'package:flutter/widgets.dart';

void main() => runApp(W());

class W extends StatelessWidget {
  @OverRide
  Widget build(BuildContext context) {
    final x = View.of(context);
    print(x.display.id);
    return const Center(
      child: Text('Hello, world!', key: Key('title'), textDirection: TextDirection.ltr),
    );
  }
}
```

## 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.
SydneyBao pushed a commit to SydneyBao/flutter that referenced this pull request Aug 22, 2025
@mattkae @SydneyBao
Report a correct display ID in the window metrics event on win32 (flu…
d714645 
…tter#174156)

## What's new?
Now that we're sending down "true" display information to the framework,
we also need to send down the correct display id in the
WindowMetricsEvent for the view so that the display resolves properly.
This pull request adds that :)

## How to test
Here's a silly app that makes sure that our `Display` is legit:

```dart
// Copyright 2014 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'package:flutter/widgets.dart';

void main() => runApp(W());

class W extends StatelessWidget {
  @OverRide
  Widget build(BuildContext context) {
    final x = View.of(context);
    print(x.display.id);
    return const Center(
      child: Text('Hello, world!', key: Key('title'), textDirection: TextDirection.ltr),
    );
  }
}
```

## 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.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 22, 2025
@engine-flutter-autoroll
4d4a69f14 Report a correct display ID in the window metrics event on …
22ed324 
…win32 (flutter/flutter#174156)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 22, 2025
Merged
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Aug 22, 2025
@engine-flutter-autoroll
Roll Flutter from d2ac0210ee05 to 26bb33b9572b (20 revisions) (#9872)
bbb89c9 
flutter/flutter@d2ac021...26bb33b

2025-08-22 34871572+gmackall@users.noreply.github.com [HCPP] Clean up overlay layer when last frame had overlay content and current doesn't (flutter/flutter#173881)
2025-08-22 32538273+ValentinVignal@users.noreply.github.com Migrate more files to use WidgetStateProperty (flutter/flutter#174176)
2025-08-22 engine-flutter-autoroll@skia.org Roll Skia from 75fef9fb3ed7 to cb15e1452399 (1 revision) (flutter/flutter#174255)
2025-08-22 engine-flutter-autoroll@skia.org Roll Skia from 006241a7fbe1 to 75fef9fb3ed7 (1 revision) (flutter/flutter#174254)
2025-08-22 biggs0125@gmail.com Skip wasm build when dry run is disabled and --wasm is not specified. (flutter/flutter#174184)
2025-08-22 engine-flutter-autoroll@skia.org Roll Dart SDK from c153c5259e62 to 4f9623f024ab (2 revisions) (flutter/flutter#174250)
2025-08-22 engine-flutter-autoroll@skia.org Roll Skia from d70087007490 to 006241a7fbe1 (2 revisions) (flutter/flutter#174252)
2025-08-22 engine-flutter-autoroll@skia.org Roll Skia from c09589f7ca69 to d70087007490 (22 revisions) (flutter/flutter#174245)
2025-08-21 bkonyi@google.com [ Widget Preview ] Add regression test for issue 173895 (flutter/flutter#174037)
2025-08-21 okorohelijah@google.com Improve xcresult comment and naming (flutter/flutter#173129)
2025-08-21 matanlurey@users.noreply.github.com Update `.gemini/styleguide.md` to encourage `master`-only (flutter/flutter#174065)
2025-08-21 bkonyi@google.com [ Widget Preview ] Fix crash when attempting to provide non-const params to a `Preview` (flutter/flutter#174242)
2025-08-21 engine-flutter-autoroll@skia.org Roll Skia from 721e68fe652a to c09589f7ca69 (12 revisions) (flutter/flutter#174162)
2025-08-21 engine-flutter-autoroll@skia.org Roll Dart SDK from 0d0a0c394381 to c153c5259e62 (7 revisions) (flutter/flutter#174235)
2025-08-21 bkonyi@google.com [ Tool ] Throw `ToolExit` when asset entries use absolute paths (flutter/flutter#174230)
2025-08-21 engine-flutter-autoroll@skia.org Roll Dart SDK from 0d0a0c394381 to c153c5259e62 (7 revisions) (flutter/flutter#174227)
2025-08-21 bkonyi@google.com [ Tool ] Cleanup widget preview and frontend server shutdown (flutter/flutter#173863)
2025-08-21 matanlurey@users.noreply.github.com Use an alternative to `git describe` for `master` version resolution (flutter/flutter#174088)
2025-08-21 matt.kosarek@canonical.com Report a correct display ID in the window metrics event on win32 (flutter/flutter#174156)
2025-08-21 47866232+chunhtai@users.noreply.github.com Revert "Update the AccessibilityPlugin::Announce method to account fo… (flutter/flutter#174223)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC bmparr@google.com,stuartmorgan@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@loic-sharma loic-sharma loic-sharma approved these changes

+1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
a: desktop Running on desktop engine flutter/engine repository. See also e: labels. platform-windows Building on or for Windows specifically
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mattkae @loic-sharma