Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@matanlurey
Copy link
Contributor

(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:

  1. Is the web implementation good enough or would we want to "ship" with an optimized impl?
  2. Can folks imagine other edge cases to test beyond correctness and deep/immutable copies?

@github-actions github-actions bot added the platform-web Code specifically for the web engine label Mar 1, 2024
Copy link
Contributor

@dnfield dnfield left a 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

@jonahwilliams
Copy link
Contributor

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.

@matanlurey
Copy link
Contributor Author

Oh shoot I had a VM implementation and I think I just didn't add it to the PR. Next week 😄

@dnfield
Copy link
Contributor

dnfield commented Mar 1, 2024

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.

@dnfield
Copy link
Contributor

dnfield commented Mar 1, 2024

We do, OTOH, have Path.from, and that is documented as doing some CoW behavior.

@jonahwilliams
Copy link
Contributor

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

@matanlurey
Copy link
Contributor Author

matanlurey commented Mar 4, 2024

Updated with the missing code for lib/ui/painting.dart and tests.

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:

  1. A user that wants to copy is going to have to implement it themselves
  2. We can do better than the user, because we know the internal data structure
  3. Every field, AFAICT, is deeply immutable, so the operation is very low-complexity:
Paint.from(Paint other) {
  _data.buffer.asUint32List().setAll(0, other._data.buffer.asUint32List());
  _objects = other._objects?.toList();
}

Rect and Offset are simple immutable classes, where-as Paint is more complicated.

@matanlurey matanlurey requested a review from dnfield March 4, 2024 17:24
@matanlurey
Copy link
Contributor Author

@eyebrowsoffire Jackson, the web failure (that I can't reproduce locally) only happens on chrome-dart2wasm-skwasm-ui - is there anything about this configuration that would cause hangs?

@flutter-dashboard
Copy link

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.

@matanlurey matanlurey requested review from eyebrowsoffire and removed request for dnfield March 25, 2024 18:23
@matanlurey
Copy link
Contributor Author

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

@matanlurey
Copy link
Contributor Author

@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 storkeMiterLimit not being hooked up as expected, which recently got fixed. I am re-running rebased on CI to see if true :)

@matanlurey matanlurey added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 29, 2024
@matanlurey
Copy link
Contributor Author

Yup that did it, all tests passing!

Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Mar 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App platform-web Code specifically for the web engine

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add copyWith feature to Paint

3 participants