[Update][Proposal] Improve Canvas Documentation by MasahideMori-SimpleAppli · Pull Request #31153 · flutter/engine

MasahideMori-SimpleAppli · 2022-01-29T12:33:07Z

This is an additional patch to the previously submitted patch.

Thanks to @flar for suggestions for making it better.

This patch improves the documentation for the Vertices.raw constructor.

Also, this time, when I Fetch upstream from the browser (flutter / engine), I got an Actions error saying "GitHub token env var is not set.", But I haven't solved it.
I'm not sure what this is because I don't use Actions,
Please let me know if you have any problems with this patch.
If the patch is okay, ignore my problem with Actions.
Thank you.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flar

There is another problem with the docs here in that the colors list says it is RGBA, but the correct format is ARGB. (which is the same format as Color.value)

One more issue which happens with the Canvas.drawVertices call that you also updated with the previous PR - it does not describe how the BlendMode parameter is used. We could say (in the documentation for drawVertices):

The [mode] parameter is used to control how the colors in the [vertices] are combined with the colors in the [paint]. If there are no colors specified in [vertices] then the [mode] has no effect. If there are colors in the [vertices], then the color taken from the [Shader] or [Color] in the [paint] is blended with the colors specified in the [vertices] using the [mode] parameter. For purposes of this blending, the colors from the [paint] are considered the source and the colors from the [vertices] are considered the destination.

flar · 2022-01-31T01:24:57Z

lib/ui/painting.dart


  /// Creates a set of vertex data for use with [Canvas.drawVertices], directly
  /// using the encoding methods of [new Vertices].
+  /// Note that this constructor uses a raw typed data list,


uses raw typed data lists,

flar · 2022-01-31T01:28:26Z

lib/ui/painting.dart

  /// Creates a set of vertex data for use with [Canvas.drawVertices], directly
  /// using the encoding methods of [new Vertices].
+  /// Note that this constructor uses a raw typed data list,
+  /// so it runs faster than Dart list version 


faster than the [Vertices()] constructor

(Make sure it links correctly to the regular constructor)

flar · 2022-01-31T01:28:55Z

lib/ui/painting.dart

  /// using the encoding methods of [new Vertices].
+  /// Note that this constructor uses a raw typed data list,
+  /// so it runs faster than Dart list version 
+  /// because it doesn't require any conversion from Dart list.


from Dart lists.

flar · 2022-01-31T01:31:50Z

lib/ui/painting.dart

  ///
  /// If the [indices] list is provided, all values in the list must be
  /// valid index values for [positions].
+  ///


No extra line here, you are continuing to describe the same parameter as the previous paragrph.

flar · 2022-01-31T01:35:09Z

lib/ui/painting.dart

  /// The [textureCoordinates] list is interpreted as a list of repeated pairs
  /// of x,y coordinates, and must be the same length of [positions] if it
  /// is not null.
+  /// 


No extra line here, both of these paragraphs describe the same parameter.

flar · 2022-01-31T01:35:34Z

lib/ui/painting.dart

  ///
  /// The [positions] list is interpreted as a list of repeated pairs of x,y
  /// coordinates. It must not be null.
+  /// The [positions] parameter is a list of triangular mesh vertices(xy).


These 2 sentences should be combined, such as

The [positions] parameter is a list of triangular mesh vertices and is interpreted as ...

(And no need to specify xy twice as the rest of the first sentence also mentions that they are x,y coordinates.)

MasahideMori-SimpleAppli · 2022-01-31T12:31:49Z

Hi @flar,
Thank you for your comment.
I fixed those issues.
Is the position of the description of drawVertices [blendMode] appropriate here?

Your comments were detailed and very helpful to me.
If there are still corrections left, I would appreciate it if you could point them out.
Thank you for your all help in advance.

flar

Looks Great! Thanks for doing this!

MasahideMori-SimpleAppli · 2022-02-01T09:49:02Z

Thank you, too!
I learned a lot from you.

chinmaygarde · 2022-02-03T21:06:40Z

The linter failure is real. There are couple of trailing whitespaces in this patch:

ERROR: Whitespace check failed. The following files have trailing spaces:
  /b/s/w/ir/cache/builder/src/flutter/lib/ui/painting.dart:3976:  /// so it runs faster than the [Vertices()] constructor 
  /b/s/w/ir/cache/builder/src/flutter/lib/ui/painting.dart:3981:  /// The [positions] parameter is a list of triangular mesh vertices and

Let's address these nits and land ASAP. Thanks.

MasahideMori-SimpleAppli · 2022-02-04T11:31:51Z

Hi @chinmaygarde

Thank you for always helping me out!
I fixed the problem.

flar · 2022-02-04T19:37:49Z

I think I've seen that failure in the Linux Smoke Tests before. You might have to rebase this PR to the Top of Tree to fix it.

MasahideMori-SimpleAppli · 2022-02-05T08:40:44Z

Hi @flar,
Thank you for your advice!
I tried rebase. Thank you again.

fluttergithubbot · 2022-02-05T19:35:10Z

This pull request is not suitable for automatic merging in its current state.

Please get at least one approved review if you are already a member or two member reviews if you are not a member before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

zanderso

Lgtm

…ne#31153)

flar suggested changes Jan 31, 2022

View reviewed changes

flar approved these changes Jan 31, 2022

View reviewed changes

MasahideMori-SimpleAppli added 3 commits February 5, 2022 16:35

[Update][Proposal] Improve Canvas Documentation

af1599a

Text corrections and additional corrections.

a64fef7

Removed unnecessary space.

9a845c4

MasahideMori-SimpleAppli force-pushed the proposal-93190-2 branch from eb7b676 to 9a845c4 Compare February 5, 2022 07:36

flar added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Feb 5, 2022

fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Feb 5, 2022

flar requested a review from zanderso February 5, 2022 19:38

zanderso approved these changes Feb 5, 2022

View reviewed changes

flar added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Feb 5, 2022

fluttergithubbot merged commit ba9d7d9 into flutter:main Feb 5, 2022

engine-flutter-autoroll mentioned this pull request Feb 5, 2022

Roll Engine from ea8386542aef to ba9d7d956916 (2 revisions) flutter/flutter#97885

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 5, 2022

ba9d7d9 [Update][Proposal] Improve Canvas Documentation (flutter/engi…

04b6487

…ne#31153)

engine-flutter-autoroll mentioned this pull request Feb 6, 2022

Roll Engine from ea8386542aef to 2a4709ae9fc4 (3 revisions) flutter/flutter#97890

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 6, 2022

ba9d7d9 [Update][Proposal] Improve Canvas Documentation (flutter/engi…

c5e9825

…ne#31153)

bleroux mentioned this pull request Jun 14, 2022

[Web][Regression] - canvas.drawVertices are missing color on web on 2.10.1 flutter/flutter#98531

Closed

Conversation

MasahideMori-SimpleAppli commented Jan 29, 2022

Pre-launch Checklist

Uh oh!

flar left a comment

Choose a reason for hiding this comment

Uh oh!

flar Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

flar Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

flar Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

flar Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

flar Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

flar Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

flar Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

MasahideMori-SimpleAppli commented Jan 31, 2022

Uh oh!

flar left a comment

Choose a reason for hiding this comment

Uh oh!

MasahideMori-SimpleAppli commented Feb 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chinmaygarde commented Feb 3, 2022

Uh oh!

MasahideMori-SimpleAppli commented Feb 4, 2022

Uh oh!

flar commented Feb 4, 2022

Uh oh!

MasahideMori-SimpleAppli commented Feb 5, 2022

Uh oh!

fluttergithubbot commented Feb 5, 2022

Uh oh!

zanderso left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

MasahideMori-SimpleAppli commented Feb 1, 2022 •

edited

Loading