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

Conversation

@flar
Copy link
Contributor

@flar flar commented Sep 21, 2021

Addressing hopefully all of the concerns raised in #28656 and flutter/flutter#90232

Changes included in this update:

  • Most method/field/parameter names should follow the style guide
  • Dispatcher methods are still mixedCamel to match the Skia methods they are replacing
  • Stroke Cap/Join nomenclature now matches Skia
  • some abbreviations eliminated (notably, "aa" is left in parameter names as seen in SkPaint::setAntiAlias(bool aa))
  • some new parameters were added to make attributes optional for some calls (primarily drawImage variants) similar to the Skia convention of some methods taking a pointer to an SkPaint which is allowed to be null.
  • I was going to change transparent Occluder, but that naming convention is used from the Flutter public API down through to Skia, so DisplayList will go with that flow, but the capitalization is modified for style guidelines

I may have made a bad call in a few places on what would be considered a getter/setter/accessor vs a method, so some feedback on those decisions would be welcome.

@flar
Copy link
Contributor Author

flar commented Sep 21, 2021

I did a quick pass through #28656 to see if there were any style suggestions that I missed and it looks like I've dealt with all of them here other than the suggestion to switch bit mask constants to an enum class, which I am not yet planning to address.

@flar flar requested a review from chinmaygarde September 21, 2021 18:44
@flar flar force-pushed the display-list-name-style-guidelines branch from 6108f7b to 626122c Compare September 21, 2021 23:28
@flar
Copy link
Contributor Author

flar commented Sep 23, 2021

With respect to the bit flag constants, I have a couple of competing issues that it would intersect:

  • We currently have 2 different ways of tracking "which attributes affect which draw operations", one is in use in the DL->Canvas adapter that governs which attributes of SkPaint to sync to the stream, and the other is in the bounds calculator. Those need to be consolidated.
  • The logic for which attributes contribute to bounds is a bit complicated and so I'm thinking that mere bit flags aren't a solid enough mechanism for its implementation and may switch to a class that encapsulates the logic (as well as potentially serving the canvas SkPaint synchronization needs as well).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants