[Widget Inspector] Fix missing cupertino icon in on-device inspector #168847
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
github-actions
bot
added
the
framework
elliette
changed the title
| import 'dart:collection' show HashMap; | ||
|
|
||
| // ignore: unused_import, required for the Widget Inspector's on-device button. | ||
| import 'package:flutter/cupertino.dart' show CupertinoIcons; |
There was a problem hiding this comment.
Importing cupertino into the widgets library is layer violation and I don't think the analyzer script is going to like this.
There was a problem hiding this comment.
Uhm I don't see the symbols imported from CupertinoIcons being used anywhere. Does it work without this import?
There was a problem hiding this comment.
Unfortunately no, that's why I added this note to the PR:
For some reason, adding the import to packages/flutter/lib/src/material/app.dart and packages/flutter/lib/src/cupertino/app.dart did not resolve the issue. That's why I've added it to lib/src/widgets/app.dart, even though it's not actually used in that file.
I first tried adding the import to where we are actually using the icon (in the two files mentioned above) however despite that change I was still seeing the [?] instead of the icon. I do not understand why though 😕
There was a problem hiding this comment.
@LongCatIsLooong is right, this is not something we can do.
There was a problem hiding this comment.
You tested in debug mode (so no icon tree shaking) right? In that case I don't know off the top of my head. I'll try to debug this.
Piinks
left a comment
Piinks
left a comment
There was a problem hiding this comment.
- adds an explicit dependency on package:cupertino_icons to the framework
- imports CupertinoIcons into lib/src/widgets/app.dart
These are not things we can do. Is there another solution we can explore?
The two other options I can think of are:
Would option #1 be possible? |
We need to avoid checking in binary assets in this repository.
If we need to import it into the widgets layer, we run into the same issue. The widgets layer and below cannot establish a dependency on a higher design level. What about using unicode characters for a generic icon? |
github-actions
bot
added
the
f: cupertino
|
Importing
Fortunately, we wouldn't be importing this directly into the widgets layer, the Material and Cupertino styled on-device buttons are defined in flutter/packages/flutter/lib/src/widgets/app.dart Lines 1044 to 1065 in 606bb06 |
|
I am wary of adding cupertino_icons as a dependency to the framework. The framework needs to separate itself from design, not integrate more with it. I understand the cupertino icons are aesthetically pleasing, but I don't think that justifies adding them as a dependency for this one feature. |
github-actions
bot
added
the
f: material design
Got it! I've updated the PR to use a Material icon and remove the dependency of Here is what it looks like now: Material-themed app Cupertino-themed app |
Ah, I see from the tests this is not allowed! Marking PR is draft for now while I figure out what icons we want to use here. |
|
This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
Switched to a generic unicode symbol for both Material and Cupertino per @Piinks original suggestion 😅 Thanks for your patience with all the back-and-forth! This should be ready for review now. Material app: Cupertino app: |
Piinks
left a comment
Piinks
left a comment
There was a problem hiding this comment.
Ah thank you! So glad unicode had some good options.
LGTM
elliette
added
the
cp: beta
|
Failed to create CP due to merge conflicts. |
…lutter#168847) Resolves flutter#168846 This is a follow-up to flutter#167677. For apps that do not import `package:cupertino_icons`, the new on-device button's icon shows up as a `[?]`. This fix: * adds an explicit dependency on `package:cupertino_icons` to the framework * imports `CupertinoIcons` into `lib/src/widgets/app.dart` *Note: For some reason, adding the import to `packages/flutter/lib/src/material/app.dart` and `packages/flutter/lib/src/cupertino/app.dart` did not resolve the issue. That's why I've added it to `lib/src/widgets/app.dart`, even though it's not actually used in that file.* **Let me know if this is acceptable!** (cc @Piinks) I'm guessing we might not want to add a dependency on `cupertino_icons` to the Framework (this might even be breaking change?) so if not, it might make sense to use a different icon for the on-device inspector. Thanks!
…lutter#168847) Resolves flutter#168846 This is a follow-up to flutter#167677. For apps that do not import `package:cupertino_icons`, the new on-device button's icon shows up as a `[?]`. This fix: * adds an explicit dependency on `package:cupertino_icons` to the framework * imports `CupertinoIcons` into `lib/src/widgets/app.dart` *Note: For some reason, adding the import to `packages/flutter/lib/src/material/app.dart` and `packages/flutter/lib/src/cupertino/app.dart` did not resolve the issue. That's why I've added it to `lib/src/widgets/app.dart`, even though it's not actually used in that file.* **Let me know if this is acceptable!** (cc @Piinks) I'm guessing we might not want to add a dependency on `cupertino_icons` to the Framework (this might even be breaking change?) so if not, it might make sense to use a different icon for the on-device inspector. Thanks!
5 participants
Resolves #168846
This is a follow-up to #167677. For apps that do not import
package:cupertino_icons, the new on-device button's icon shows up as a[?].This fix:
package:cupertino_iconsto the frameworkCupertinoIconsintolib/src/widgets/app.dartNote: For some reason, adding the import to
packages/flutter/lib/src/material/app.dartandpackages/flutter/lib/src/cupertino/app.dartdid not resolve the issue. That's why I've added it tolib/src/widgets/app.dart, even though it's not actually used in that file.Let me know if this is acceptable! (cc @Piinks) I'm guessing we might not want to add a dependency on
cupertino_iconsto the Framework (this might even be breaking change?) so if not, it might make sense to use a different icon for the on-device inspector. Thanks!