-
Notifications
You must be signed in to change notification settings - Fork 6k
Implement Paint.from(other) for dart:ui.
#51110
Conversation
dnfield
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would need a VM implementation as well to really evaluate.
I don't think we should do this. dart:ui avois implementing convenience APIs in general. Paint is kind of on the edge, but at the very least, we should see what this would look like on the VM implementation. In that case, Paint.from is going to have to copy some ByteData (not so bad) and copy a list of Objects (which probably will all need their own .from methods).
I'll add a comment about this to the bug as well.
/cc @goderbauer
|
Paint is unique in that we don't have a higher level API that wraps it. Pretty much all custom painter/painting code in Flutter will directly create and manipulate paint objects. For this case in particular I don't think its unreasonable to add convienence API to it. |
|
Oh shoot I had a VM implementation and I think I just didn't add it to the PR. Next week 😄 |
|
We don't have higher level APIs wrapping Rect or Offset either, and those don't have these kind of convenience constructors. My bigger concern is what to do about some of the mutable fields on paint if we adopt this. I think that however we try to document it, it will be counterintuitive ... and I'm really not sure why we'd support this kind of API when it's so easy for a user who wants the simple case to work to do the right thing themselves. |
|
We do, OTOH, have |
|
We clearly don't need a Rect.from or Offset.from, those are very simple classes. But Paint is a big old bag of properties. IIRC the filters objects stored on Paint are actually immutable so we don't need to deeply copy |
|
Updated with the missing code for I'll note I'm still unable to run or debug the web tests, I've reached out for help. A convenience method makes a lot of sense here:
Paint.from(Paint other) {
_data.buffer.asUint32List().setAll(0, other._data.buffer.asUint32List());
_objects = other._objects?.toList();
}
|
|
@eyebrowsoffire Jackson, the web failure (that I can't reproduce locally) only happens on |
|
This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold. |
|
@eyebrowsoffire I'll need some help debugging why the WASM implementation hits an infinite loop. I setup a few minutes this Friday to pair if that's more helpful, thanks! |
Might be easy - @eyebrowsoffire let me know there was an issue with |
|
Yup that did it, all tests passing! |
jonahwilliams
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…146014) flutter/engine@4b6836f...8ec35b6 2024-03-29 [email protected] [Impeller] removed old blur detritus (flutter/engine#51779) 2024-03-29 [email protected] Implement `Paint.from(other)` for `dart:ui`. (flutter/engine#51110) 2024-03-29 [email protected] Roll Dart SDK from bb65648e20e2 to 52b05146758e (3 revisions) (flutter/engine#51783) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
(Tenatively) Closes flutter/flutter#142871.
I personally think this is a reasonable request, and we (framework authors) can make some users happy in a fairly simple way (flutter/flutter#142871, flutter/flutter#40497).
Some questions: