-
Notifications
You must be signed in to change notification settings - Fork 58
Update PlatformDrawable API #673
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
Conversation
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.
Review completed. 4 suggestions posted. Main themes are protobuf decoding robustness and ensuring the new SPI PlatformDrawable surface is constructible/usable by external adopters.
Comment augment review to trigger a new review at any time.
| case 2: options.rbColorMode = Int32(try decoder.intField(field)) | ||
| case 3: options.flags = Flags(rawValue: UInt32(try decoder.intField(field))) | ||
| case 4: options.maxDrawableCount = Int8(try decoder.intField(field)) |
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.
The Int32/UInt32/Int8(...) conversions in protobuf decoding will trap on out-of-range (or negative) values, so malformed/forward-version data could crash decoding. Consider validating/clamping before converting so decode failures are handled as errors instead of traps.
🤖 Was this useful? React with 👍 or 👎
|
|
||
| package var _renderer: DisplayList.GraphicsRenderer? | ||
|
|
||
| package init() { |
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.
PlatformDrawableContent.State is public but its initializers are package, and PlatformDrawableContent.draw(...) requires an inout State; SPI clients outside this package may have no way to create/retain a State. If draw is intended for external PlatformDrawable implementers, it likely needs a constructible state type.
🤖 Was this useful? React with 👍 or 👎
|
|
||
| @_spi(DisplayList_ViewSystem) | ||
| @available(OpenSwiftUI_v6_0, *) | ||
| public struct PlatformDrawableOptions: Equatable, Sendable { |
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.
PlatformDrawableOptions is public SPI and PlatformDrawable.options is get set, but there’s no public/SPI initializer (and base is internal), which can make it impossible for external adopters to initialize their stored options. If the intent is framework-owned mutation only, consider making that contract explicit.
🤖 Was this useful? React with 👍 or 👎
|
|
||
| class RBDisplayListInterpolator {} | ||
|
|
||
| public enum RBDisplayList {} |
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.
As a case-less enum, RBDisplayList is uninhabited, so any API that takes it can’t be called in shim builds. Is a constructible placeholder type intended here?
🤖 Was this useful? React with 👍 or 👎
25d72f9 to
3cd6e73
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #673 +/- ##
==========================================
- Coverage 13.86% 13.82% -0.05%
==========================================
Files 558 559 +1
Lines 32492 32555 +63
==========================================
- Hits 4505 4500 -5
- Misses 27987 28055 +68 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
No description provided.