added windows system fonts listener #563

chunhtai · 2019-09-13T22:08:06Z

This will requires https://github.com/flutter/engine/pull/12276/files for the new embedding api

googlebot · 2019-09-13T22:08:08Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-09-16T20:19:47Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

chunhtai · 2019-09-16T20:21:19Z

@stuartmorgan This is ready for review. For some reason, I can't add a reviewer

stuartmorgan-g · 2019-09-17T04:34:50Z

Will try to review tomorrow; currently swamped.

stuartmorgan-g · 2019-09-18T23:31:29Z

example/windows/win32_window.h

  // true if the window was created successfully.
  bool CreateAndShow(const std::wstring &title, const Point &origin,
-                     const Size &size);
+                     const Size &size, flutter::FlutterViewController *controller);


I would like to see a limited-purpose interface (e.g., FlutterWindowDelegate or similar) passed rather than the whole controller, with only the specific methods the window needs access to.

stuartmorgan-g · 2019-09-18T23:35:19Z

example/windows/win32_window.cc

      }
      return 0;
+    case WM_FONTCHANGE:
+      controller_->OnFontChange();


I don't know enough about Windows to know if this is a reasonable suggestion or not, but I'm curious if we could forward WM_FONTCHANGE to child_content_ and avoid the need for the special-purpose APIs at the embedding layer. Then we could just handle WM_FONTCHANGE in the embedding layer (in fact, you already do, where I commented wondering whether that code was doing anything).

@clarkezone Is that doable? A good/bad idea?

Seems reasonable. I assume WM_FONTCHANGE isn’t automatically propogated to child windows hence the approach here, but manually posting it would avoid the need for the API as you say.

Is there a standard way to forward windows message? It does not seem like windows provide an api to reroute message to child process. If we want to use sendMessage to forward the message, the embedding will have to start a message loop as well

reroute message to child process

It's not a child process, it's just a different HWND in the same process.

If we want to use sendMessage to forward the message, the embedding will have to start a message loop as well

Why would that be the case? The child window already receives messages (e.g., mouse events), so clearly the existing message loop is capable of sending messages to that HWND.

Also, the docs for SendMessage say: "If the specified window was created by the calling thread, the window procedure is called immediately as a subroutine." I would expect this is the same thread.

i made it too complicated, fixed

googlebot · 2019-09-19T20:12:46Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

stuartmorgan-g · 2019-09-19T21:19:12Z

example/windows/win32_window.cc

      }
      return 0;
+
+    // Messages directly forward to embedding.


Nit: s/forward/forwarded/

stuartmorgan-g · 2019-09-19T21:43:27Z

You'll need to fix the commit email address and force-push to address the CLA issue, then it'll be good to land.

While you're doing that, could you replicate this change in testbed/? I'd like them to stay in sync as much as possible.

chunhtai · 2019-09-19T21:52:19Z

You'll need to fix the commit email address and force-push to address the CLA issue, then it'll be good to land.

While you're doing that, could you replicate this change in testbed/? I'd like them to stay in sync as much as possible.

will do, for some reason my windows desktop does not play well with github, will fixes it once i finalize both engine embedding and here.

googlebot · 2019-09-19T22:54:34Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

chunhtai mentioned this pull request Sep 13, 2019

Add system font change listener for windows flutter/engine#12276

Merged

chunhtai force-pushed the issues/38556 branch from 57927ab to 2b87a4f Compare September 16, 2019 20:19

stuartmorgan-g self-requested a review September 17, 2019 04:34

stuartmorgan-g reviewed Sep 18, 2019

View reviewed changes

chunhtai force-pushed the issues/38556 branch from 2b87a4f to 34a3935 Compare September 19, 2019 20:12

stuartmorgan-g approved these changes Sep 19, 2019

View reviewed changes

added windows system fonts listener

a499580

chunhtai force-pushed the issues/38556 branch from 5d2d08d to a499580 Compare September 19, 2019 22:54

stuartmorgan-g approved these changes Sep 19, 2019

View reviewed changes

stuartmorgan-g merged commit e7676d3 into google:master Sep 19, 2019

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

added windows system fonts listener #563

added windows system fonts listener #563

Uh oh!

Conversation

chunhtai commented Sep 13, 2019

Uh oh!

googlebot commented Sep 13, 2019

Uh oh!

googlebot commented Sep 16, 2019

Uh oh!

chunhtai commented Sep 16, 2019

Uh oh!

stuartmorgan-g commented Sep 17, 2019

Uh oh!

stuartmorgan-g Sep 18, 2019

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Sep 18, 2019

Choose a reason for hiding this comment

Uh oh!

clarkezone Sep 19, 2019

Choose a reason for hiding this comment

Uh oh!

chunhtai Sep 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Sep 19, 2019

Choose a reason for hiding this comment

Uh oh!

chunhtai Sep 19, 2019

Choose a reason for hiding this comment

Uh oh!

googlebot commented Sep 19, 2019

Uh oh!

stuartmorgan-g Sep 19, 2019

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g commented Sep 19, 2019

Uh oh!

chunhtai commented Sep 19, 2019

Uh oh!

googlebot commented Sep 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chunhtai Sep 19, 2019 •

edited

Loading