Add system font change listener for windows #12276

chunhtai · 2019-09-13T20:55:11Z

This pr add listener in windows embedding and use the new embedding api to notify system font changes
This requires framework flutter/flutter#38930
This also requires example app change google/flutter-desktop-embedding#563

flutter/flutter#38556

googlebot · 2019-09-13T20:55:14Z

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-13T20:55:14Z

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.

chunhtai · 2019-09-16T19:32:42Z

@googlebot I fixed it

googlebot · 2019-09-16T19:33:52Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

googlebot · 2019-09-16T19:33:52Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

chunhtai · 2019-09-16T19:36:35Z

shell/platform/embedder/tests/embedder_unittests.cc

@@ -228,6 +228,16 @@ TEST(EmbedderTestNoFixture, CanGetCurrentTimeInNanoseconds) {
  ASSERT_LT((point2 - point1), fml::TimeDelta::FromMilliseconds(1));
 }

+TEST_F(EmbedderTest, CanReloadSystemFonts) {


This test make sure the hook from embedding -> embedding_engine -> shell works. It does not test for functionality.

chunhtai · 2019-09-16T19:37:02Z

shell/common/shell_unittests.cc

@@ -519,6 +519,30 @@ TEST_F(ShellTest, ReportTimingsIsCalledImmediatelyAfterTheFirstFrame) {
  ASSERT_EQ(timestamps.size(), FrameTiming::kCount);
 }

+TEST_F(ShellTest, ReloadSystemFonts) {


This tests the actual functionality

chunhtai · 2019-09-16T19:45:15Z

shell/platform/windows/win32_flutter_window.cc

@@ -147,6 +151,22 @@ void Win32FlutterWindow::OnClose() {
  messageloop_running_ = false;
 }

+void Win32FlutterWindow::OnFontChange() {


@kpozin Here is the current design for system font api.
When system font changes:

Use FlutterEngineReloadSystemFonts in embedder.h to reload system font and clear caches

send system channel message {"type": "fontsChange"} to "flutter/system" flutter framework to rebuild the widget.

Noted that you will probably need to implement fuchsia version of loading default fonts.
see: third_party/txt/src/txt/platform_windows.cc
FlutterEngineReloadSystemFonts relys on this function to retrieve the latest system fonts. If there is any caching happen under this layer, the FlutterEngineReloadSystemFonts will still load the old value.

Let me know if this looks ok to you.

Yes, this seems fine to me.

Thanks for confirming, I will let you know if anything change.

@kpozin a little update
you will only need to call this embedder api

engine/shell/platform/embedder/embedder.h

Line 1211 in aadd5a3

FlutterEngineResult FlutterEngineReloadSystemFonts(

I make it so it will send the system message as part of the api.
However, you will still need to implement fuchsia version of loading default fonts.
https://github.com/flutter/engine/blob/master/third_party/txt/src/txt/platform_fuchsia.cc

stuartmorgan-g · 2019-09-18T23:04:39Z

shell/common/shell_unittests.cc

+  std::vector<std::string> families(1, "Robotofake");
+  auto font =
+      fontCollection->GetMinikinFontCollectionForFamilies(families, "en");
+  ASSERT_EQ(font->getId(), (unsigned int)0);


C-style casts are forbidden; use C++ casts (static_cast here)

stuartmorgan-g · 2019-09-18T23:06:09Z

shell/platform/windows/client_wrapper/flutter_view_controller.cc

@@ -58,5 +58,8 @@ FlutterDesktopPluginRegistrarRef FlutterViewController::GetRegistrarForPlugin(
  }
  return FlutterDesktopGetPluginRegistrar(controller_, plugin_name.c_str());
 }
+void FlutterViewController::OnFontChange() {


Blank line above this.

stuartmorgan-g · 2019-09-18T23:06:34Z

shell/platform/windows/client_wrapper/include/flutter/flutter_view_controller.h

@@ -61,6 +61,8 @@ class FlutterViewController {
  // loop.
  void ProcessMessages();

+  void OnFontChange();


Needs a declaration comment.

It should also get a TODO to move out of the view controller class once there is an engine class. You can reference #38600

stuartmorgan-g · 2019-09-18T23:09:48Z

shell/platform/windows/public/flutter_windows.h

@@ -69,6 +69,10 @@ FlutterDesktopGetHWND(FlutterDesktopViewControllerRef controller);
 // loop.
 FLUTTER_EXPORT void FlutterDesktopProcessMessages();

+// Invokes OnFontChange callback in flutter windows view.
+FLUTTER_EXPORT void FlutterOnFontChange(


Everything in this API surface should use FlutterDesktop as a prefix.

stuartmorgan-g · 2019-09-18T23:10:06Z

shell/platform/windows/win32_flutter_window.cc

 #include <chrono>
+#include "flutter/shell/platform/common/cpp/client_wrapper/include/flutter/json_message_codec.h"


Blank line above this.

stuartmorgan-g · 2019-09-18T23:14:21Z

shell/platform/windows/win32_flutter_window.cc

 #include <chrono>
+#include "flutter/shell/platform/common/cpp/client_wrapper/include/flutter/json_message_codec.h"
+
+static constexpr char kSystemChannelName[] = "flutter/system";


win32_flutter_window doesn't seem like the right place for the flutter/system channel to live. Please set up a new handler for now, following the model of platform_handler.{h,cc}. Later we may decide to fold it into another class, but that will be easier if it's not embedded into this one to start with.

This is all cruft now.

shell/platform/embedder/embedder.h

shell/platform/windows/win32_window.cc

stuartmorgan-g · 2019-09-18T23:27:02Z

third_party/txt/src/txt/platform_windows.cc

+namespace txt {
+
+std::string GetDefaultFontFamily() {
+  return "Arial";


This change seems unrelated. Is this improving font fallback handling? If so, it would be great to split this out into its own change as part of #39915

Also, I'm surprised to see a file being added to third_party. What is the source for txt, and why isn't this an upstream change?

This is essential for reloading system fonts. The original GetDefaultFontFamily in third_party/txt/src/txt/platform.cc uses SkFontMgr::RefDefault() which will cache the result and there is no way to clear it. When a new system font is installed, it will not load the latest system fonts but return previous cached. We have to implement the windows version of it.

There is no upstream for txt. The txt's source lives in here. One thing that worries me is that this might be migrated to skia library at some point @jason-simmons Do you think it is ok to change it now?

Yes - go ahead and modify this in libtxt. if we later migrate to Skia's text shaper then we will need to port this feature over to the integration with Skia.

googlebot · 2019-09-19T22:46:00Z

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-19T22:46:00Z

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-19T22:58:39Z

shell/common/shell.cc

+  // After system fonts are reloaded, we send a system channel message
+  // to notify flutter framework.
+  rapidjson::Document document;
+  document.Parse("{}");


document.SetObject();

stuartmorgan-g · 2019-09-19T23:00:39Z

shell/common/shell.cc

+  rapidjson::StringBuffer buffer;
+  rapidjson::Writer<rapidjson::StringBuffer> writer(buffer);
+  document.Accept(writer);
+  std::string message(buffer.GetString(), buffer.GetSize());


IIRC you only need GetString(), since we should be building rapidjson with std::string support. Am I misremembering?

stuartmorgan-g · 2019-09-19T23:01:48Z

shell/platform/windows/client_wrapper/flutter_view_controller.cc

@@ -58,5 +58,4 @@ FlutterDesktopPluginRegistrarRef FlutterViewController::GetRegistrarForPlugin(
  }
  return FlutterDesktopGetPluginRegistrar(controller_, plugin_name.c_str());
 }
-


Revert please, since you're no longer touching this file otherwise.

stuartmorgan-g · 2019-09-19T23:01:59Z

shell/platform/windows/client_wrapper/testing/stub_flutter_windows_api.cc

@@ -70,6 +70,8 @@ void FlutterDesktopProcessMessages() {
  }
 }

+void FlutterOnFontChange(FlutterDesktopViewControllerRef controller) {}


Remove these changes.

stuartmorgan-g · 2019-09-19T23:03:10Z

shell/platform/windows/win32_flutter_window.cc

 #include <chrono>
+#include "flutter/shell/platform/common/cpp/client_wrapper/include/flutter/json_message_codec.h"
+
+static constexpr char kSystemChannelName[] = "flutter/system";


This is all cruft now.

googlebot · 2019-09-19T23:57:36Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

googlebot · 2019-09-19T23:57:36Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

chunhtai · 2019-09-23T17:57:30Z

@stuartmorgan this is ready for re-review

stuartmorgan-g

LGTM!

…2276)

[email protected]:flutter/engine.git/compare/e7fd442410f4...953ac71 git log e7fd442..953ac71 --no-merges --oneline 2019-09-24 [email protected] Roll src/third_party/dart acac9ab11b..d53d355c6c (12 commits) 2019-09-24 [email protected] Roll src/third_party/skia b4b1005d485f..c096654fa7c6 (1 commits) (flutter/engine#12415) 2019-09-24 [email protected] Roll src/third_party/skia b3542d95da65..b4b1005d485f (6 commits) (flutter/engine#12414) 2019-09-24 [email protected] Write MINIMAL_SDK to exception message (flutter/engine#11345) 2019-09-23 [email protected] Track "mouse leave" event (flutter/engine#12363) 2019-09-23 [email protected] Roll src/third_party/dart 77ff89b223..acac9ab11b (6 commits) 2019-09-23 [email protected] Don't send pointer events when the framework isn't ready yet (flutter/engine#12403) 2019-09-23 [email protected] Update test to verify that secondary isolate gets shutdown before root isolate exits. (flutter/engine#12342) 2019-09-23 [email protected] Roll src/third_party/skia 57ef68077574..b3542d95da65 (12 commits) (flutter/engine#12409) 2019-09-23 [email protected] Update --dart-vm-flags whitelist to include --write-service-info and --sample-buffer-duration (flutter/engine#12395) 2019-09-23 [email protected] Add system font change listener for windows (flutter/engine#12276) 2019-09-23 [email protected] Roll fuchsia/sdk/core/mac-amd64 from xPX7p... to kgFwg... (flutter/engine#12405) 2019-09-23 [email protected] Check for index bounds in RTL handling for trailing whitespace runs. (flutter/engine#12336) 2019-09-23 [email protected] Roll fuchsia/sdk/core/linux-amd64 from F-g18... to NY2A5... (flutter/engine#12407) 2019-09-23 [email protected] Roll src/third_party/dart f546362691..77ff89b223 (6 commits) 2019-09-23 [email protected] Updating text field location in IOS as a pre-work for spellcheck (flutter/engine#12192) 2019-09-23 [email protected] Roll buildroot and remove toolchain prefix. (flutter/engine#12324) 2019-09-23 [email protected] Roll src/third_party/dart 94dd49cdb6..f546362691 (1 commits) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

googlebot added the cla: no label Sep 13, 2019

chunhtai force-pushed the issues/38556 branch from 19ca3e6 to 65f9874 Compare September 16, 2019 19:31

chunhtai closed this Sep 16, 2019

chunhtai reopened this Sep 16, 2019

chunhtai force-pushed the issues/38556 branch from 65f9874 to a347504 Compare September 16, 2019 19:33

googlebot added cla: yes and removed cla: no labels Sep 16, 2019

chunhtai requested a review from stuartmorgan-g September 16, 2019 19:34

chunhtai commented Sep 16, 2019

View reviewed changes

chunhtai requested a review from GaryQian September 16, 2019 19:37

chunhtai commented Sep 16, 2019

View reviewed changes

chunhtai mentioned this pull request Sep 16, 2019

Implement system fonts system channel listener flutter/flutter#38930

Merged

9 tasks

stuartmorgan-g reviewed Sep 18, 2019

View reviewed changes

chunhtai force-pushed the issues/38556 branch from 615844a to 55b7fb5 Compare September 19, 2019 22:45

googlebot added cla: no and removed cla: yes labels Sep 19, 2019

stuartmorgan-g suggested changes Sep 19, 2019

View reviewed changes

chunhtai force-pushed the issues/38556 branch from 55b7fb5 to 7c47a64 Compare September 19, 2019 23:57

googlebot removed the cla: no label Sep 19, 2019

googlebot added the cla: yes label Sep 19, 2019

chunhtai force-pushed the issues/38556 branch 2 times, most recently from a401745 to d42ec1c Compare September 23, 2019 16:43

stuartmorgan-g approved these changes Sep 23, 2019

View reviewed changes

chunhtai added 3 commits September 23, 2019 11:26

Add windows font change logic

ece29f4

update

2cb339e

fix comment

514ed64

chunhtai force-pushed the issues/38556 branch from cd579e2 to 514ed64 Compare September 23, 2019 18:26

chunhtai merged commit aadd5a3 into flutter:master Sep 23, 2019

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 24, 2019

aadd5a3 Add system font change listener for windows (flutter/engine#1…

d1f6df9

…2276)

engine-flutter-autoroll mentioned this pull request Sep 24, 2019

Roll engine e7fd442410f4..953ac71749ee (18 commits) flutter/flutter#41163

Merged

		#include <chrono>
		#include "flutter/shell/platform/common/cpp/client_wrapper/include/flutter/json_message_codec.h"

Add system font change listener for windows #12276

Add system font change listener for windows #12276

Uh oh!

Conversation

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

Uh oh!

Uh oh!

googlebot commented Sep 13, 2019

Uh oh!

googlebot commented Sep 13, 2019

Uh oh!

chunhtai commented Sep 16, 2019

Uh oh!

googlebot commented Sep 16, 2019

Uh oh!

googlebot commented Sep 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai Sep 16, 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!

chunhtai Sep 16, 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!

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!

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!

googlebot commented Sep 19, 2019

Uh oh!

googlebot commented Sep 19, 2019

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!

googlebot commented Sep 19, 2019

Uh oh!

googlebot commented Sep 19, 2019

Uh oh!

chunhtai commented Sep 23, 2019

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chunhtai commented Sep 13, 2019 •

edited

Loading

chunhtai Sep 16, 2019 •

edited

Loading

chunhtai Sep 16, 2019 •

edited

Loading