Introduce the DisplayManager so that we send real display info down to the client #173301
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a DisplayManager to provide display information to the framework on Windows, which is a great addition for features like fullscreen windowing. The implementation is solid, creating a message-only window to listen for display changes.
My review includes a few suggestions to improve code quality and correctness:
- Adhering to the C++ style guide regarding virtual destructors.
- Improving code hygiene by removing an unused include.
- Using the project's standard logging mechanism for consistency and better error reporting.
- A high-severity comment on the use of an enum value for display updates, which seems semantically incorrect based on the current embedder API and could lead to future issues.
| std::vector<FlutterEngineDisplay> const& displays) const { | ||
| embedder_api_.NotifyDisplayUpdate(engine_, | ||
| kFlutterEngineDisplaysUpdateTypeStartup, | ||
| displays.data(), displays.size()); |
There was a problem hiding this comment.
The OnDisplaysChanged method uses kFlutterEngineDisplaysUpdateTypeStartup when notifying the engine of display changes. According to its name and documentation in embedder.h, this enum value is intended for the initial display setup at startup.
Using it for dynamic updates is semantically incorrect and could lead to unexpected behavior if the framework adds special handling for startup display information in the future.
It seems the embedder API is missing a value for dynamic updates (e.g., kFlutterEngineDisplaysUpdateTypeChanged). Please consider adding a new value to FlutterEngineDisplaysUpdateType in embedder.h and using it here. If changing embedder.h is out of scope for this PR, adding a code comment to explain this limitation would be helpful.
| auto error = GetLastError(); | ||
| LPWSTR message = nullptr; | ||
| size_t size = FormatMessageW( | ||
| FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM | | ||
| FORMAT_MESSAGE_IGNORE_INSERTS, | ||
| NULL, error, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), | ||
| reinterpret_cast<LPWSTR>(&message), 0, NULL); | ||
| OutputDebugString(message); | ||
| LocalFree(message); |
There was a problem hiding this comment.
For consistency with the rest of the Flutter engine code, it's better to use FML_LOG(ERROR) instead of OutputDebugString. This ensures that error messages are routed through the standard logging channels.
The current implementation also doesn't check if FormatMessageW succeeds, which could lead to issues if message is nullptr. The suggested change below addresses these points.
Please also add #include "flutter/fml/platform/win/wstring_conversion.h" at the top of the file to use fml::WideToUtf8.
const auto error = GetLastError();
LPWSTR message_buffer = nullptr;
size_t size = FormatMessageW(
FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM |
FORMAT_MESSAGE_IGNORE_INSERTS,
nullptr, error, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
reinterpret_cast<LPWSTR>(&message_buffer), 0, nullptr);
if (size > 0 && message_buffer) {
FML_LOG(ERROR) << "Failed to create display manager window: "
<< fml::WideToUtf8(message_buffer);
LocalFree(message_buffer);
} else {
FML_LOG(ERROR) << "Failed to create display manager window. Error code: "
<< error;
}| } | ||
| UnregisterClass(window_class_name_.c_str(), nullptr); | ||
| } | ||
| #include <iostream> |
There was a problem hiding this comment.
This #include <iostream> appears to be unused. Please remove it to maintain code hygiene.
| class DisplayManager { | ||
| public: | ||
| DisplayManager(FlutterWindowsEngine* engine); | ||
| virtual ~DisplayManager(); |
There was a problem hiding this comment.
The destructor is marked virtual, but DisplayManager doesn't seem to be designed as a base class. According to the Google C++ Style Guide, destructors should only be virtual in classes intended for inheritance.1
Please consider making the destructor non-virtual.
~DisplayManager();
What's new?
DisplayManagerwhich is responsible for retrieving display info on the win32 platformWindowsProcTablefor test mocking purposesWhy?
The win32 platform is about to be interested in the displays that it has available, specifically when setting the fullscreen status of a window for windowing. Hence, we need to provide it with an accurate list of such things
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.