Add high contrast accessibility support for TV #237

zhouleonlei · 2022-02-09T07:36:47Z

Add accessibility high contrast in TV menu settings

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

swift-kim

Looks much better than before. Thanks!

shell/platform/tizen/accessibility_settings.cc

swift-kim · 2022-02-10T02:46:58Z

shell/platform/tizen/accessibility_settings.cc

+static void OnAccessibilityHighContrastChangedCallback(
+    system_settings_key_e key,
+    void* user_data) {
+  auto accessibilitySettings =


This violates the variable naming rule. As we already know that this file is an implementation file of AccessibilitySettings, you could simply rename the pointer to

Suggested change

auto accessibilitySettings =

auto* self =

swift-kim · 2022-02-10T02:50:03Z

shell/platform/tizen/accessibility_settings.cc

+AccessibilitySettings::AccessibilitySettings(FlutterTizenEngine* engine)
+    : engine_(engine) {
+  Init();
+}
+
+AccessibilitySettings::~AccessibilitySettings() {
+  Dispose();
+}


The Init() and Dispose() methods look redundant here. You may simply move the implementations to the constructor and destructor respectively.

shell/platform/tizen/accessibility_settings.cc

swift-kim · 2022-02-10T03:17:20Z

shell/platform/tizen/accessibility_settings.cc

+        << ret;
+  }
+#endif
+  return enabled;


You might simply change the return type to bool.

Suggested change

return enabled;

return enabled != 0;

By the way cannot system_settings_set_value_bool be used instead of system_settings_get_value_int?

You might simply change the return type to bool.

By the way cannot system_settings_set_value_bool be used instead of system_settings_get_value_int?

I had tried system_settings_set_value_bool, but met the parameter error

Looks much better than before. Thanks!

Thanks for your review. I have modified all of them.
Please check.

shell/platform/tizen/accessibility_settings.h

swift-kim · 2022-02-10T03:24:08Z

shell/platform/tizen/flutter_tizen_engine.cc


+// Set bold font and application color when accessibility high contrast state is
+// changed.
+void FlutterTizenEngine::SetAccessibilityHighContrastEnabled(int enabled) {


FlutterTizenEngine is a wrapper class of the engine's embedder API so it is advised to follow the engine's terminology if possible. How about

Suggested change

void FlutterTizenEngine::SetAccessibilityHighContrastEnabled(int enabled) {

void FlutterTizenEngine::EnableAccessibilityFeature(bool bold_text) {

shell/platform/tizen/flutter_tizen_engine.cc

shell/platform/tizen/flutter_tizen_engine.h

swift-kim

Thanks for the quick fixes. We're almost there.

shell/platform/tizen/flutter_tizen_engine.h

shell/platform/tizen/accessibility_settings.cc

swift-kim · 2022-02-10T11:16:45Z

shell/platform/tizen/accessibility_settings.cc

+  system_settings_set_changed_cb(
+      system_settings_key_e(
+          SYSTEM_SETTINGS_KEY_MENU_SYSTEM_ACCESSIBILITY_HIGHCONTRAST),
+      OnAccessibilityHighContrastChangedCallback, this);


You may simply inline the OnAccessibilityHighContrastChangedCallback function to reduce the use of ifdef:

Suggested change

OnAccessibilityHighContrastChangedCallback, this);

[](system_settings_key_e key, void* user_data) -> void {

auto* self = reinterpret_cast<AccessibilitySettings*>(user_data);

self->OnAccessibilityHighContrastStateChanged();

},

this);

shell/platform/tizen/accessibility_settings.cc

shell/platform/tizen/accessibility_settings.h

swift-kim · 2022-02-10T13:12:47Z

The code is good, but the "high contrast" option from the TV settings and Flutter's "bold text" accessibility feature do not seem to match by their meaning. In the framework, they are two different concepts:

Have you considered other ways to enable the feature? The most desirable way I can think of right now is to add the missing kFlutterAccessibilityFeatureHighContrast flag to the embedder API and send a PR to the upstream.

swift-kim · 2022-03-04T03:26:20Z

Another question: Will this change work out of the box without having to change any app code (Dart)?

zhouleonlei · 2022-03-04T03:41:44Z

Another question: Will this change work out of the box without having to change any app code (Dart)?

No need to modify codes in App side.

swift-kim · 2022-03-04T05:17:51Z

Let's merge this for now.

I heard from @zhouleonlei that turning on the high contrast mode on TV enables both bold text and dark background for native apps. Flutter apps should be affected in the same way, but this PR enables bold text only. Let's see if we can add the highContrast (dark background) option to the embedder API in the future.

swift-kim · 2022-03-04T05:40:47Z

Cherry-picked into the flutter-2.10.1-tizen branch: dba8fff.

swift-kim reviewed Feb 10, 2022

View reviewed changes

swift-kim changed the title ~~Add high contrast~~ Add high contrast accessibility support for TV Feb 10, 2022

zhouleonlei force-pushed the lei_281 branch 2 times, most recently from 162345c to 17dcac8 Compare February 10, 2022 09:01

swift-kim reviewed Feb 10, 2022

View reviewed changes

zhouleonlei force-pushed the lei_281 branch from 17dcac8 to 45c4924 Compare February 10, 2022 12:28

Add high contrast

45c4924

swift-kim approved these changes Feb 10, 2022

View reviewed changes

swift-kim merged commit d1741f2 into flutter-tizen:flutter-2.8.1-tizen Mar 4, 2022

swift-kim pushed a commit that referenced this pull request Mar 4, 2022

Add high contrast accessibility support for TV (#237)

9b052ef

swift-kim pushed a commit that referenced this pull request Mar 4, 2022

Add high contrast accessibility support for TV (#237)

dba8fff

This was referenced Mar 11, 2022

Accessibility support flutter-tizen/flutter-tizen#151

Closed

Update engine hash to c03dc26 flutter-tizen/flutter-tizen#344

Merged

swift-kim pushed a commit that referenced this pull request May 12, 2022

Add high contrast accessibility support for TV (#237)

30f8db7

swift-kim pushed a commit that referenced this pull request Aug 5, 2022

Add high contrast accessibility support for TV (#237)

56b1648

swift-kim pushed a commit that referenced this pull request Sep 1, 2022

Add high contrast accessibility support for TV (#237)

09f2ae2

	void FlutterTizenEngine::SetAccessibilityHighContrastEnabled(int enabled) {
	void FlutterTizenEngine::EnableAccessibilityFeature(bool bold_text) {

-      OnAccessibilityHighContrastChangedCallback, this);
+      [](system_settings_key_e key, void* user_data) -> void {
+        auto* self = reinterpret_cast<AccessibilitySettings*>(user_data);
+        self->OnAccessibilityHighContrastStateChanged();
+      },
+      this);

Add high contrast accessibility support for TV #237

Add high contrast accessibility support for TV #237

Uh oh!

Conversation

zhouleonlei commented Feb 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

swift-kim 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

swift-kim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

swift-kim commented Feb 10, 2022

Uh oh!

swift-kim commented Mar 4, 2022

Uh oh!

zhouleonlei commented Mar 4, 2022

Uh oh!

swift-kim commented Mar 4, 2022

Uh oh!

swift-kim commented Mar 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zhouleonlei commented Feb 9, 2022 •

edited

Loading