Use DPI utils provided by embedder #658

franciscojma86 · 2020-02-01T01:04:40Z

The embedder now provides an API to get the right DPI dynamically, solving crashes caused by unavailable libraries on older versions.

Also, listens to the WM_DPICHANGED message and resizes the window properly.

Fixes flutter/flutter#40855

Depends on flutter/engine#16313

stuartmorgan-g

Just small things.

If you want to also fix up window_size to use the new functions instead of ShellScalingApi directly for window and monitor DPI in this PR, since it's very closely related, that would be great, but if not I can do it in a follow-up.

example/windows/win32_window.cc

stuartmorgan-g · 2020-02-04T18:17:18Z

example/windows/win32_window.cc

                   newWidth, newHeight, SWP_NOZORDER | SWP_NOACTIVATE);

-      return 0;
+      break;


Why don't we want to indicate that we've handled the message?

The examples that I've seen still call the DefWindowProc. Is there a reason to stop the propagation?

The docs say "If an application processes this message, it should return zero." It may be that the default handler just doesn't do anything, but absent a specific reason (like, we figure out that the docs are not accurate in this regard) I'd rather we follow what the documentation says the expected contract is.

stuartmorgan-g · 2020-02-04T18:18:04Z

example/windows/win32_window.cc

      return 0;

+    case WM_DPICHANGED: {
+      // Resize the window only for toplevel windows which have a suggested


I'm not sure what this comment means. Isn't this class always a top-level window?

Yeah, bad copy paste from the engine code.

stuartmorgan-g · 2020-02-04T18:18:52Z

example/windows/main.cpp

 // limitations under the License.

 #include <flutter/flutter_view_controller.h>
+#include <flutter_windows.h>


Is this cruft from an earlier iteration, or fixing a missing include?

stuartmorgan-g · 2020-02-04T18:20:38Z

example/windows/win32_window.cc

+    case WM_DPICHANGED: {
+      // Resize the window only for toplevel windows which have a suggested
+      // size.
+      auto lprcNewScale = reinterpret_cast<RECT *>(lparam);


Use style-compliant variable names, not Windows-style naming, per style guide.

clarkezone

LGTM

clarkezone · 2020-02-05T11:42:53Z

example/windows/win32_window.cc

+// Dynamically loads the |EnableNonClientDpiScaling| from the User32 module.
+// Appended with FDE to differentiate from the win32 API.
+bool FDEEnableNonClientDpiScaling(HWND hwnd) {
+  HMODULE user32_module_ = LoadLibraryA("User32.dll");


do you clean this up somewhere aka release the HMODULE?

I wasn't. Added a FreeLibrary call after using the method.

stuartmorgan-g · 2020-02-06T00:47:11Z

example/windows/win32_window.cc

+// Appended with FDE to differentiate from the win32 API.
+void EnableFullDpiSupportIfAvailable(HWND hwnd) {
+  HMODULE user32_module_ = LoadLibraryA("User32.dll");
+  if (user32_module_) {


This should be if (!user32_module_), no?

stuartmorgan-g · 2020-02-06T00:47:26Z

example/windows/win32_window.cc

+// Dynamically loads the |EnableNonClientDpiScaling| from the User32 module.
+// Appended with FDE to differentiate from the win32 API.
+void EnableFullDpiSupportIfAvailable(HWND hwnd) {
+  HMODULE user32_module_ = LoadLibraryA("User32.dll");


Remove trailing _; it's not an ivar.

stuartmorgan-g · 2020-02-06T00:48:02Z

example/windows/win32_window.cc

+      reinterpret_cast<EnableNonClientDpiScaling_ *>(
+          GetProcAddress(user32_module_, "EnableNonClientDpiScaling"));
+
+  FreeLibrary(user32_module_);


Shouldn't this be after calling the function you looked up from the library?

stuartmorgan-g · 2020-02-06T00:49:41Z

plugins/window_size/windows/window_size_plugin.cpp

 #include <flutter/plugin_registrar_windows.h>
 #include <flutter/standard_method_codec.h>
+#include <flutter_windows.h>
+#include <windows.h>


IIRC having windows.h after VersionHelpers.h broke compilation, which is why they are a separate group. I think maybe putting a comment on the group will prevent the autoformatter from reordering it.

stuartmorgan-g · 2020-02-06T00:51:04Z

plugins/window_size/windows/window_size_plugin.cpp

-  GetDpiForMonitor(monitor, MDT_EFFECTIVE_DPI, &dpi_x, &dpi_y);
-  double scale_factor = dpi_x / kBaseDpi;
+  // Send a nullptr since the top-level window hasn't been created. This will
+  // get the neares monitor's DPI.


primary

But this isn't the right thing to call; we want the DPI for monitor, which is why I had you expose the other function too.

stuartmorgan-g · 2020-02-06T00:51:18Z

plugins/window_size/windows/window_size_plugin.cpp

  RECT frame;
  GetWindowRect(window, &frame);
  HMONITOR window_monitor = MonitorFromWindow(window, MONITOR_DEFAULTTOPRIMARY);
  // TODO: Support fallback for systems older than Windows 10(1607).


You can remove the TODO now.

stuartmorgan-g · 2020-02-06T00:51:43Z

testbed/windows/win32_window.cc

+// Appended with FDE to differentiate from the win32 API.
+void EnableFullDpiSupportIfAvailable(HWND hwnd) {
+  HMODULE user32_module_ = LoadLibraryA("User32.dll");
+  if (user32_module_) {


Remember to mirror all the same fixes here.

stuartmorgan-g · 2020-02-06T00:52:41Z

example/windows/win32_window.cc

+#include <flutter_windows.h>
+
 #include "resource.h"
-#include "shellscalingapi.h"


Presumably you also need to make a project change to stop linking directly to Shcore.lib?

franciscojma86 · 2020-02-06T02:07:38Z

Sorry for the sloppy patch, PTAL!

stuartmorgan-g

LGTM with nits. I think you'll get some collisions with my recent warnings cleanup changes, but I expect they'll all be easily resolvable. If anything seems non-obvious, let me know and I can re-review.

stuartmorgan-g · 2020-02-11T17:14:30Z

plugins/window_size/windows/window_size_plugin.cpp

-  GetDpiForMonitor(monitor, MDT_EFFECTIVE_DPI, &dpi_x, &dpi_y);
-  double scale_factor = dpi_x / kBaseDpi;
+  INT dpi = FlutterDesktopGetDpiForMonitor(monitor);
+  double scale_factor = static_cast<double>(dpi) / kBaseDpi;


You shouldn't need the cast here; kBaseDpi is a double in this file. (We could do the same in the other file so we don't need to explicitly cast, but it's not a big deal either way.)

stuartmorgan-g · 2020-02-11T17:17:03Z

plugins/window_size/windows/window_size_plugin.cpp

-  UINT dpi_x, dpi_y;
-  GetDpiForMonitor(monitor, MDT_EFFECTIVE_DPI, &dpi_x, &dpi_y);
-  double scale_factor = dpi_x / kBaseDpi;
+  INT dpi = FlutterDesktopGetDpiForMonitor(monitor);


The API returns a UINT, not INT.

stuartmorgan-g · 2020-02-12T03:13:15Z

See my comment here: 5b2af71#r37243493

There's nothing wrong with the CI.

franciscojma86 · 2020-02-13T00:35:05Z

Landing on red since the AppVeyor config has been removed and the build step seems to be an old artifact from the CI changes

franciscojma86 requested review from clarkezone and stuartmorgan-g February 1, 2020 01:04

stuartmorgan-g reviewed Feb 4, 2020

View reviewed changes

clarkezone approved these changes Feb 5, 2020

View reviewed changes

franciscojma86 force-pushed the dpi branch from 228769c to 20642b6 Compare February 6, 2020 00:08

franciscojma86 requested a review from stuartmorgan-g February 6, 2020 00:38

stuartmorgan-g suggested changes Feb 6, 2020

View reviewed changes

franciscojma86 requested a review from stuartmorgan-g February 6, 2020 02:07

franciscojma86 added 11 commits February 7, 2020 08:57

mostly works

c5ca35f

Clean

f05ec50

Format

dbdacd9

Enable dpi scaling

596c90c

Comments

c567480

Add to testbed

4045cd9

Fix window size and format

95b1c8b

Return 0

71c445b

Reveiew

b0245b4

Remove direct linking

c05e593

Clean

d5f5dbc

franciscojma86 force-pushed the dpi branch from 0af0e75 to d5f5dbc Compare February 7, 2020 17:29

Use nearest monitor properly

77cf246

stuartmorgan-g approved these changes Feb 11, 2020

View reviewed changes

franciscojma86 added 2 commits February 11, 2020 14:31

Merge branch 'master' into dpi

1ddc8af

Remove cast

a49e351

franciscojma86 merged commit d25b181 into google:master Feb 13, 2020

franciscojma86 mentioned this pull request Feb 19, 2020

Embedding doesn't run on Windows 7 and crashes with an unclear missing DLL message. flutter/flutter#40855

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 5, 2021

Use DPI utils provided by embedder #658

Use DPI utils provided by embedder #658

Uh oh!

Conversation

franciscojma86 commented Feb 1, 2020

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clarkezone left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

franciscojma86 commented Feb 6, 2020

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g commented Feb 12, 2020

Uh oh!

franciscojma86 commented Feb 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants