Let Flutter SDK use cupertino_icons 1.0.0 #65087

xster · 2020-09-02T10:24:01Z

Description

#65083 made all the new CupertinoIcons IconData available. This one micro-tunes all the various demos and aesthetics. For package/flutter changes, they're backward compatible with cupertino_icon 0.1.3. For demo apps, just update to cupertino_icon 1.0.0.

Related Issues

#33916

Tests

Gallery golden

Checklist

Before you create this PR, confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

No, no existing tests failed, so this is not a breaking change.

flutter-dashboard · 2020-09-02T10:24:04Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

xster · 2020-09-03T09:30:25Z

Unfortunately I had to run update-packages, so it dumped quite a few more files in this PR

skia-gold · 2020-09-03T17:43:26Z

Gold has detected about 3 untriaged digest(s) on patchset 8.
View them at https://flutter-gold.skia.org/cl/github/65087

xster · 2020-09-03T19:59:28Z

Pausing this for now. I can't update any packages in flutter/flutter until I update flutter/gallery first.

flutter/gallery#288

xster · 2020-09-03T21:35:02Z

Sorry, had to do a big detour to update the packages.

Update the packages to use it in the gallery app to golden against the real package font in screenshot.

This PR also does some micro-tuning of some spacing/color for iOS 14 with the new icons.

skia-gold · 2020-09-03T21:46:26Z

Gold has detected about 3 untriaged digest(s) on patchset 10.
View them at https://flutter-gold.skia.org/cl/github/65087

xster

The gold diff look ok to me

xster · 2020-09-03T21:36:00Z

packages/flutter/lib/src/cupertino/nav_bar.dart

This is "the main thing" this PR does. Our spacing, sizes were a bit off.

xster · 2020-09-03T21:36:21Z

packages/flutter_tools/templates/app/pubspec.yaml.tmpl

This is "the second thing" this PR does. Ups our template package.

xster · 2020-09-03T21:36:38Z

dev/integration_tests/flutter_gallery/test/demo/cupertino/cupertino_navigation_demo_test.dart

This is the "third thing". Asserts the new icon looks in goldens.

xster · 2020-09-03T21:37:04Z

packages/flutter/lib/src/cupertino/text_field.dart

This is "the fourth thing". Our colors are a bit off now.

nit: on iOS 14 this seems to be pure black with 20% alpha (0x33000000) and pure white with 20% alpha (0x33FFFFF) for light and dark.

LongCatIsLooong

Thank you for going over the laborious process! With the previous pull request, CupertinoIcons now has 100% coverage of all the SF symbol codepoints?

LongCatIsLooong · 2020-09-03T21:48:28Z

dev/integration_tests/flutter_gallery/test/demo/cupertino/cupertino_navigation_demo_test.dart

I wonder why the golden tests weren't failing prior to this change?

There aren't any golden tests prior. I just added it :)

LongCatIsLooong · 2020-09-03T22:40:54Z

packages/flutter/lib/src/cupertino/text_field.dart

nit: on iOS 14 this seems to be pure black with 20% alpha (0x33000000) and pure white with 20% alpha (0x33FFFFF) for light and dark.

xster · 2020-09-03T22:51:40Z

CupertinoIcons now has 100% coverage of all the SF symbol codepoints?

Correct. It didn't initially but I just ended up backfilling all the glyphs we had from before. flutter/website#4570 for more details.

xster · 2020-09-03T22:52:34Z

Umm I can't reply to #65087 (comment) for some reason. Ah cool, thanks for checking. Was this from sketch?

LongCatIsLooong · 2020-09-03T22:56:41Z

no those are from Xcode 12

xster · 2020-09-10T19:00:39Z

dev/benchmarks/macrobenchmarks/pubspec.yaml

@pennzht @yjbanov I'm unfortunately removing the flutter/flutter <-> flutter/gallery linkage here.

The reason being that flutter/flutter can no longer proactively control the pub dependencies it uses (since they're effectively controlled by flutter/gallery with the one version rule) and increases maintenance cost.

I've been trying to cycle package updates through flutter/gallery first as well but have trouble rolling to gallery head and haven't been able to resolve it this week flutter/gallery#282 (comment) to unblock Flutter's 1.22 branch cut.

Some context: flutter/gallery#254 (comment), flutter/gallery#254 (comment).

Could web benchmarks be driven off of flutter/flutter's gallery https://github.com/flutter/flutter/tree/master/dev/integration_tests/flutter_gallery instead of the flutter/gallery gallery?

Is this acceptable?

Is this acceptable?

That would be very unfortunate. We were going to expand usage of the new gallery to benchmark the CanvasKit renderer as well. This will halt that effort too.

We have similar linkage between flutter/flutter and flutter/plugins. What are we doing wrong with flutter/gallery that it leads to so much pain?

Could you point me to it? I didn't find it in the codebase.

The macrobenchmarks app depends on several plugins, such as shared_preferences and url_launcher.

Ah I see what you mean. Right it does. flutter/flutter has pub dependencies against released pub packages which is fine. Pub can dependency resolve the asks in pubspec.yaml for all pubspecs in the repo and produce a pubspec.lock (which seems to be fed back into pubspec.yaml in flutter/flutter's case). It doesn't add a git dependency on the flutter/plugin repo.

The difference here is that it's no longer a 1-tiered resolution. The pubspec.lock resolution in the gallery repo is already resolved. Those become the specific pubspec.yaml asks feeding into flutter/flutter.

If I change a shared_preferences dependency in flutter/flutter today, I can use flutter update-packages --force-upgrade. It'll find all instances and pull a valid resolved version. It can't account for flutter/gallery.

This is of course solvable, but it implies building more tooling into flutter/flutter to account for flutter/gallery and push synchronized PRs back into flutter/gallery.

@pennzht has a plan to fix this (https://docs.google.com/document/d/1eI9OqjGbyKSAGQVupN6Wjz3NmrOjhLG2JSPWkvJMyfs). In the meantime, is it possible to make it work with the current setup, so we have time to fix it in the next couple of weeks?

Thanks for taking this on. Yes, I can wait. I've done a reduced-scope workaround for the time being. My timing concerns are mainly around getting our visuals correct now that iOS 14 is launched so I'd like to get this through at some point this month but it's not urgent.

Ok, so let's remove the gallery benchmarks for now. After a discussion with @pennzht @Hixie @jmagman @jonahwilliams and @christopherfujino we decided to move the benchmarks into the customer shard. We'll start working on that.

yjbanov

LGTM for gallery benchmark removal.

renew package update

xster · 2020-10-16T18:58:58Z

Looks like all the failures are infra flakes failing to git clone. Restarting

xster · 2020-10-16T20:22:01Z

a few of the retries flaked again on clone :S

running again

xster · 2020-10-17T05:35:00Z

Looks like the flake reruns finished but isn't updating back the github checks for some reason. Submitting.

googlebot added the cla: yes label Sep 2, 2020

xster force-pushed the cupertino-icon-4 branch from 961d756 to 697f1df Compare September 3, 2020 07:03

xster requested a review from LongCatIsLooong September 3, 2020 09:06

xster requested a review from Piinks as a code owner September 3, 2020 09:29

xster marked this pull request as draft September 3, 2020 17:30

xster force-pushed the cupertino-icon-4 branch from 58cb609 to 7943ee2 Compare September 3, 2020 17:31

xster marked this pull request as ready for review September 3, 2020 21:33

xster commented Sep 3, 2020

View reviewed changes

LongCatIsLooong approved these changes Sep 3, 2020

View reviewed changes

This was referenced Sep 4, 2020

Update create templates to 1.0.0 #65234

Merged

LUCI seems to not use pub in the latest state #65397

Closed

xster force-pushed the cupertino-icon-4 branch 3 times, most recently from eff54a3 to 93242d8 Compare September 10, 2020 17:21

xster commented Sep 10, 2020

View reviewed changes

xster force-pushed the cupertino-icon-4 branch from 93242d8 to d1a0ee1 Compare September 11, 2020 17:46

xster requested a review from pennzht September 11, 2020 17:47

xster requested a review from yjbanov September 11, 2020 17:47

yjbanov approved these changes Sep 29, 2020

View reviewed changes

xster force-pushed the cupertino-icon-4 branch from d1a0ee1 to b209c63 Compare September 30, 2020 06:20

jonahwilliams requested a review from yjbanov October 15, 2020 20:45

xster force-pushed the cupertino-icon-4 branch from b209c63 to ba291e0 Compare October 16, 2020 17:04

xster added 2 commits October 16, 2020 10:05

make Flutter use the new cupertino_icons

66b6035

renew package update

run update packages again

5d1b596

xster force-pushed the cupertino-icon-4 branch from ba291e0 to 5d1b596 Compare October 16, 2020 17:07

xster merged commit 3302a12 into flutter:master Oct 17, 2020

xster deleted the cupertino-icon-4 branch October 17, 2020 05:36

Let Flutter SDK use cupertino_icons 1.0.0 #65087

Let Flutter SDK use cupertino_icons 1.0.0 #65087

Uh oh!

Conversation

xster commented Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

flutter-dashboard bot commented Sep 2, 2020

Uh oh!

xster commented Sep 3, 2020

Uh oh!

skia-gold commented Sep 3, 2020

Uh oh!

xster commented Sep 3, 2020

Uh oh!

xster commented Sep 3, 2020

Uh oh!

skia-gold commented Sep 3, 2020

Uh oh!

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

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

xster commented Sep 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xster commented Sep 3, 2020

Uh oh!

LongCatIsLooong commented Sep 3, 2020

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!

yjbanov left a comment

Choose a reason for hiding this comment

Uh oh!

xster commented Oct 16, 2020

Uh oh!

xster commented Oct 16, 2020

Uh oh!

xster commented Oct 17, 2020

Uh oh!

Reviewers

Assignees

xster commented Sep 2, 2020 •

edited

Loading

xster commented Sep 3, 2020 •

edited

Loading