Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor to an Expando instead of extending Paint #52

Merged
merged 1 commit into from Jul 28, 2022

Conversation

blaugold
Copy link
Contributor

Extending Paint makes it impossible to compile for Web.

It's not documented, but extending classes from dart:ui should be
avoided because implementations are often platform-dependent.

Extending `Paint` makes it impossible to compile for Web.

It's not documented, but extending classes from `dart:ui` should be
avoided because implementations are often platform-dependent.
@greensopinion
Copy link
Owner

Thanks for the PR!

Expando is pretty cool.

Did you measure the performance impact of using Expando? Not sure if it's still problematic, but there was this: dart-lang/sdk#44333

An alternative would be to do something like this:

class PaintModel {
  final Paint paint;
  final List<double>? strokeDashPattern;
}

If it's not measurable, happy to merge as-is.

@blaugold
Copy link
Contributor Author

I didn't benchmark it, but when testing manually I didn't notice any difference.

The last comment in the issue you linked shows benchmarks before and after the fix, from which I gather that the performance issue is resolved.

For what it's worth, I've used Expandos elsewhere and had no problems with performance.

@greensopinion
Copy link
Owner

web screenshot on #51

@greensopinion greensopinion merged commit 0d383eb into greensopinion:main Jul 28, 2022
@greensopinion
Copy link
Owner

Thanks for the awesome contributions @blaugold !

@blaugold blaugold deleted the fix/extend-paint branch July 28, 2022 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants