Adds Support for system dpi awareness for older versions of Windows #12735
Conversation
| AssignProcAddress(user32_module_, "SetProcessDpiAware", | ||
| set_process_dpi_aware_); | ||
|
|
||
| SetProcessDPIAware(); |
There was a problem hiding this comment.
Whilst it’s tempting to do this I do not recommend it. The host app should be in charge of it’s DPI mode, not flutter. For add to app scenarios you’ll have to support all DPI modes in the fullness of time. Also, it’s not recommended to set DPI programatically, that should be done in the calling app’s manifest. Recommend updating this to sniff the current DPI mode and act accordingly. You’ll have to put this logic in testbed / example tho
There was a problem hiding this comment.
From what I understand, setting the awareness in the application manifest will override this calls anyway. I added the calls to ensure that even if the host didn't provide it in the manifest, we would. I tested with Testbed, by setting a different awareness in the manifest and it took that value over the api call. What do you think?
| WNDCLASS window_class = ResgisterWindowClass(converted_title); | ||
| // Set DPI awareness for all Windows versions. This call has to be made before | ||
| // the HWND is created. | ||
| dpi_helper_->SetDpiAwerenessAllVersions(); |
There was a problem hiding this comment.
As mentioned above, don’t set the DPI awareness mode here but in the calling app.
| BOOL result = that->dpi_helper_->EnableNonClientDpiScaling(window); | ||
| if (result != TRUE) { | ||
| OutputDebugString(L"Failed to enable non-client area autoscaling"); | ||
| if (that->dpi_helper_->IsPerMonitorV2Available()) { |
There was a problem hiding this comment.
Assuming you move the DPI mode setting logic into the client app, this would become a getter for the current DPI mode
| if (permonitorv2_supported_) { | ||
| SetProcessDpiAwarenessContext(DPI_AWARENESS_CONTEXT_PER_MONITOR_AWARE_V2); | ||
| } else { | ||
| std::cerr << "Per Monitor V2 DPI awareness is not supported on this " |
There was a problem hiding this comment.
Why is this logged? It's not an error case, unless I'm not understanding the PR.
There was a problem hiding this comment.
I saw it as more of a fringe case, but it's true it's not an error. Makes sense to remove it.
| // | ||
| // This call is overriden if DPI awareness is stated in the application | ||
| // manifest. | ||
| void SetDpiAwerenessAllVersions(); |
| // Wrapper for OS functionality to return the DPI for the System. Only used if | ||
| // Per Monitor V2 is not supported by the current Windows version. | ||
| UINT GetDpiForSystem(); | ||
|
|
||
| // Sets the current process to a specified DPI awareness context. | ||
| BOOL SetProcessDpiAwarenessContext(DPI_AWARENESS_CONTEXT); |
There was a problem hiding this comment.
Do we actually need three public functions here? Would it make sense to just expose a top-level SetProcessDpiAware (which would actually be what's currently called SetDpiAwarenessAllVersions, and the current function with that name being called something with System in it, maybe)? It's not clear to me if there are cases where a client would need to know the details of which mode is being set.
There was a problem hiding this comment.
Yeah I agree, we just need to expose the one that would set it for all versions.
| @@ -28,17 +28,35 @@ class Win32DpiHelper { | |||
| // Wrapper for OS functionality to return the DPI for |HWND| | |||
| UINT GetDpiForWindow(HWND); | |||
|
|
|||
| // Wrapper for OS functionality to return the DPI for the System. Only used if | |||
| // Per Monitor V2 is not supported by the current Windows version. | |||
| UINT GetDpiForSystem(); | |||
There was a problem hiding this comment.
Similar to the comment below: is there a reason to expose this separately rather than making it an internal detail of GetDpiForWindow?
There was a problem hiding this comment.
GetDpiForWindow is also the name of the win32 api function, which is specific for Per Monitor V2. I didn't want to mix the names and create confusion on what it does. I could just change it to GetDpi, and under the hood, do either ..DpiForWindow/System accordingly. What do you think?
|
Holding this PR for little bit while working on other priorities at the moment. |
|
I've marked the PR as Draft so we skip it in the triage meeting. |
|
Closing for now since I'm not actively working on this but I will pick it up soon |
Not supporting Per Monitor V1 since I wasn't able to get dpi_changed events from the Windows procedure. Will investigate this further, but this solution still covers most of our use cases.
This change also sets dpi awareness programmatically, removing the need to state it in the application manifest.
google/flutter-desktop-embedding#572