Extend SystemUiOverlayStyle with additonal values for each platform by jonahwilliams · Pull Request #17171 · flutter/flutter

jonahwilliams · 2018-05-01T22:21:29Z

Requires flutter-team-archive/engine#5112. See this PR for more context

Work towards full support for #16837

HansMuller · 2018-05-02T15:39:32Z

  /// applications with a dark background.
-  light,
+  static const SystemUiOverlayStyle light = const SystemUiOverlayStyle(
+    navigationBarColor: 0xFF000000,


Black seems at odds with overlays should be drawn with a light color

Let me do another pass on these values... I keep getting my wires crossed whether we are referring to the color of the UI or the color to match the UI.

HansMuller · 2018-05-02T15:41:22Z

  /// applications with a light background.
-  dark,
+  static const SystemUiOverlayStyle dark = const SystemUiOverlayStyle(
+    navigationBarColor: 0xFFFFFFFF,


White seems at odds with overlays should be drawn with a dark color.

Shouldn't this version specify a navigationDividerColor (because a non-white divider looks best on white).

Fixed colors.

I can add a divider color, but we can't actually invoke that API until we update our target SDK version to P though. We can address this later if we decide to draw the bar ourselves using a transparent navigation bar

HansMuller · 2018-05-02T15:42:40Z

+  /// The color of the divider between the bottom navigation and content.
+  /// 
+  /// Only honored in Android versions P and greater.
+  final int navigationDividerColor;


For the sake of consistency (but not brevity): navigationBarDividerColor

HansMuller · 2018-05-02T15:46:29Z

+  /// Only honored in Android versions O and greater.
+  final int navigationBarColor;
+
+  /// The color of the divider between the bottom navigation and content.


between the systems bottom navigation bar and the app's content.

HansMuller · 2018-05-02T15:49:31Z

 }

+/// Specifies a dark vs light preference for certain system chrome options.
+enum SystemChromeTheme {


We've been using FooTheme to name inherited widgets (typically configured with FooData) that configure visuals for descendant Foo widgets.

Instead of defining a new enum here, could we use Brightness

Sounds good. brightness was defined in material so I pulled it into services and re-exported from theme_data. Let me know if there is a better approach here, I didn't think that services should import material since it is supposed to be at a lower level

Having services provide Brightness sounds sensible to me.

HansMuller · 2018-05-02T15:51:01Z

+    this.navigationIconTheme,
+  });
+
+  /// The color of the bottom navigation bar.


Even though the doc's context informs otherwise, think it's worth qualifying this as the "system" navigation bar to avoid confusion with BottomNavigationBar.

HansMuller · 2018-05-02T15:53:04Z

+  /// The theme of the navigation bar buttons.
+  /// 
+  /// Only honored in Android versions O and greater.
+  final SystemChromeTheme navigationIconTheme;


Folding in earlier naming suggestions: Brightness navigationBarIconBrightness

HansMuller · 2018-05-02T15:54:03Z

+  /// The color of top status bar.
+  /// 
+  /// Only honored in Android version O and greater.
+  final int statusBarColor;


Why not use a Color here (and pass along it's int value)?

HansMuller · 2018-05-02T15:55:03Z

+  /// The theme of top status bar.
+  /// 
+  /// Only honored in iOS and Android version O and greater.
+  final SystemChromeTheme statusBarTheme;


Folding in earlier naming suggestions: Brightness statusBarBrightness

Done - split into statusBarBrightness and statusBarIconBrightness

…ave platform themes (no breaking change)

jonahwilliams · 2018-05-02T17:29:31Z

To keep this from being a breaking change, I have added checks to prevent the light theme from being set for Android. These checks can be removed when I add platform themes and clean up the app bars

jonahwilliams · 2018-05-03T03:11:50Z

Actually it looks like some of these properties can be used before O. I will double check everything and update

…in M and above

jonahwilliams · 2018-05-08T02:22:39Z

flutter-team-archive/engine#5112 is merged into flutter/flutter, @HansMuller PTAL when you have a minute

HansMuller

Just some small-time feedback about the details.

HansMuller · 2018-05-08T23:07:42Z

-  light,
+  static const SystemUiOverlayStyle light = const SystemUiOverlayStyle(
+    systemNavigationBarColor: const Color(0xFFFFFFFF),
+    systemNavigationBarDividerColor: null,


Don't we want a shade of gray here?

Since I haven't gotten a chance to play with the P preview I don't really know what the behavior is. null makes sure the API isn't invoked yet.

Good point. I imagine that we'll have to render this divider on the framework side anyway.

HansMuller · 2018-05-08T23:10:20Z

+  /// Only honored in Android version M and greater.
+  final Brightness statusBarIconBrightness;
+
+  /// The brightness of the system navigation bar icons.


It would be a little tidier if this definition was next to the other system nav bar defs

HansMuller · 2018-05-08T23:16:02Z

+  }
+
+  @override
+  int get hashCode => 


We usually reserve => for one-liners that return a value

HansMuller · 2018-05-08T23:18:45Z

+  final Brightness systemNavigationBarIconBrightness;
+
+  /// Convert this event to a map for serialization.
+  Map<String, dynamic> toMap() {


I believe this is just for communicating with the native code, so _toMap()

HansMuller · 2018-05-08T23:26:32Z

+  }
 }

 List<String> _stringify(List<dynamic> list) {


This function could be written or just inlined as

list.map((item) => item.toString()).toList()

The toList() might not be needed either, depending the implementation of invokeMethod.

I didn't write this code so I'm going to leave it as is

…lutter#17171)

Hixie · 2018-05-14T23:34:41Z

-  SystemChrome.setSystemUIOverlayStyle(
-    darkBackground ? SystemUiOverlayStyle.light : SystemUiOverlayStyle.dark
-  );
+  // TODO(jonahwilliams): remove once we have platform themes.


in general i wouldn't bother with this kind of TODO. Either we eventually add platform themes and remove it, or we don't and this TODO isn't necessary.

I recommend reserving TODOs for more fundamental things where we know there's a limitation to the current implementation but we are intentionally cutting corners for some reason (e.g. because we are waiting an upstream fix).

Hixie · 2018-05-14T23:35:57Z

+      return false;
+    final SystemUiOverlayStyle typedOther = other;
+    return typedOther.systemNavigationBarColor == systemNavigationBarColor
+      && typedOther.systemNavigationBarDividerColor == systemNavigationBarDividerColor


these should be indented so that the expressions line up

Jonah Williams added 4 commits April 27, 2018 13:20

set status bar to same color as app for new gallery

e823b9a

Merge branch 'master' of github.com:flutter/flutter into system_chrome

82293a8

update SystemChrome api

a6ed815

swap colors for themes and remove gallery change

bb6510a

googlebot added the cla: yes label May 1, 2018

HansMuller reviewed May 2, 2018

View reviewed changes

Jonah Williams added 3 commits May 2, 2018 10:20

correct theme settings and disable light theme for android until we h…

989f1c4

…ave platform themes (no breaking change)

fix doc comment

518ff04

rename systemNavigationBarIconBrightness

897b538

jonahwilliams mentioned this pull request May 3, 2018

Light status bar with dark icons #17231

Closed

Jonah Williams added 4 commits May 3, 2018 12:54

update docs to specify that system chrome status bar can be adjusted …

e7c45ec

…in M and above

Merge branch 'master' of github.com:flutter/flutter into system_chrome

c2f6bd4

Merge branch 'master' of github.com:flutter/flutter into system_chrome

761b7e2

remove todo

8bbaaaf

HansMuller approved these changes May 8, 2018

View reviewed changes

jonahwilliams and others added 3 commits May 8, 2018 17:46

address some comments

06a44de

Merge branch 'master' of github.com:flutter/flutter into system_chrome

5aa13cf

Merge branch 'master' of github.com:flutter/flutter into system_chrome

59fbb8f

jonahwilliams merged commit 5732219 into flutter:master May 10, 2018

DaveShuckerow pushed a commit to DaveShuckerow/flutter that referenced this pull request May 14, 2018

Extend SystemUiOverlayStyle with additonal values for each platform (f…

2c42937

…lutter#17171)

Hixie reviewed May 14, 2018

View reviewed changes

jonahwilliams mentioned this pull request Jul 14, 2018

Need support for configuring Android's SystemNavigationBar color #16837

Closed

jonahwilliams deleted the system_chrome branch July 20, 2018 19:26

github-actions Bot locked as resolved and limited conversation to collaborators Aug 10, 2021

Conversation

jonahwilliams commented May 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams May 2, 2018 • 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!

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!

jonahwilliams commented May 2, 2018

Uh oh!

jonahwilliams commented May 3, 2018

Uh oh!

jonahwilliams commented May 8, 2018

Uh oh!

HansMuller 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!

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!

Reviewers

jonahwilliams commented May 1, 2018 •

edited

Loading

jonahwilliams May 2, 2018 •

edited

Loading