Add remaining dart fixes for Color deprecations when importing painting.dart #162609
Conversation
|
FYI @bwilkerson for this somewhat workaround we found. :) |
| - kind: 'rename' | ||
| newName: 'a' | ||
|
|
||
| - title: "Rename from 'red' to 'r'" |
There was a problem hiding this comment.
beware of #161598, I introduced subtle bugs in a downstream library due to this
There was a problem hiding this comment.
Yea, i don't think these should be rename fixes. Instead it should do something like this to avoid the introduction of bugs.
foo.red;
(foo.r * 255.0).round() & 0xff;| - kind: 'rename' | ||
| newName: 'a' | ||
|
|
||
| - title: "Rename from 'red' to 'r'" |
There was a problem hiding this comment.
Yea, i don't think these should be rename fixes. Instead it should do something like this to avoid the introduction of bugs.
foo.red;
(foo.r * 255.0).round() & 0xff;|
So I think this is as close as I can get it.. There will be a missing parenthesis, but the analyzer will help with that. myColor.blue; // before
myColor.b * 255.0).round() & 0xff; // afterWe have several 'partial' fixes like this, since it gets folks most of the way there. Might double check with @bwilkerson though. I don't think any other data driven fix rules will get me that extra opening parenthesis. |
There was a problem hiding this comment.
This is unfortunate, I'm interested in what @bwilkerson has to say. Maybe there is a better alternative.
@Piinks can you link to prior art here so I can see what we are doing elsewhere. As this PR is written it generates broken syntax. This is going to freak out formatters and leave people counting parenthesis.
I wonder if we should have proper dart syntax, but broken semantics. Something like:
color.blue;
// becomes
color._((color.b * 255.0).round() & 0xff);Both options are bad but at least this is valid syntax. People just have to delete the method call (color._). I honestly think no dart fix is better than a broken dart fix.
If we can't get it to fix the code correctly, maybe we can just get a custom message in the linter failure that tells people exactly what they need to write?
|
Sorry for the long delay. I'm just catching up after a 3 day team training session. @gaaclarke is right that this is not how Also, for the change from But the real problem here is that data-driven fixes don't yet support the ability to change the return type. We should add that capability, but I don't know what your timeframe for this is so I don't know whether we can get it done in time. I added an issue to track the work and discuss the requirements: dart-lang/sdk#60079. Also, I'm interested to know what other cases people have run into where |
|
Reviving this PR that I clearly let fall through a crack... From @gaaclarke
I know we have done this several times, but we have several hundred fixes now. I have not been able to dig through them to find an example for you.
This would be nice, but dart fix can't do this. I can't grab the
I still think the partial fix is more helpful to the user trying to navigate this change. That being said, I've removed the partial fixes, and instead updated the deprecation lint to provide the correct code. I think this was the main friction point developers were sharing with us anyways, red != r, so folks trying to migrate based on the lint message were still broken and confused. From @bwilkerson
Done. Thanks for the feedback all. PTAL |
| - title: "Rename from 'alpha' to 'a'" | ||
| date: 2024-09-10 | ||
| element: | ||
| uris: [ 'painting.dart' ] | ||
| method: 'alpha' | ||
| inClass: 'Color' | ||
| changes: | ||
| - kind: 'rename' | ||
| newName: 'a' |
There was a problem hiding this comment.
I don't know if we want this change since it can lead to subtle bugs.
double foo = color.alpha / 255.0 becoming double foo = color.a / 255.0 will give very different results.
There was a problem hiding this comment.
Similar to r, g, b, I think the subtleties like this are what folks are really hurting over with this migration.
Should the alpha deprecation instead also have a better lint message?
There was a problem hiding this comment.
With a != alpha, what is the conversion then? The migration guide doesn't mention it.
There was a problem hiding this comment.
The math is the same as the one for the [red, green, blue] color components. We mention it in this section of the migration guide: https://docs.flutter.dev/release/breaking-changes/wide-gamut-framework#access-color-components
| @@ -270,19 +270,19 @@ class Color { | |||
| /// | |||
| /// A value of 0.0 means this color is fully transparent. A value of 1.0 means | |||
| /// this color is fully opaque. | |||
| @Deprecated('Use .a.') | |||
| @Deprecated('Use (*.a * 255.0).round() & 0xff') | |||
There was a problem hiding this comment.
You accidentally added this to opacity not alpha =)
There was a problem hiding this comment.
Whoops, must be Friday. 😅
Towards #160617
Noticed the
opacityandwithOpacitydeprecations were supported by dart fix, but not the others. Adding them here.But strange too given
dart:uidoes not support dart fix (dart-lang/sdk#59764)..We think since
painting.dartexportsdart:ui, this is somewhat of a workaround.If a user has this atop their file:
If instead
Will continue to follow up on dart fix support for
dart:Xlibraries, for now hopefully this will help!Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.