[core] port raster resampling property #12176
Conversation
mollymerp
added
Core
mollymerp
force-pushed
the
raster-resampling
branch
from
f7949be to
ec109c7
Compare
ignore feature-state tests and other to-be-ported tests
mollymerp
force-pushed
the
raster-resampling
branch
from
ec109c7 to
2cf3012
Compare
| Values of this type are used in the `MGLRasterStyleLayer.rasterResampling` | ||
| property. | ||
| */ | ||
| typedef NS_ENUM(NSUInteger, MGLRasterResampling) { |
There was a problem hiding this comment.
This enumeration and the corresponding property are named with a gerund, which makes them look like a protocol at the point of use. Consider overriding the property’s name to raster-resampling-mode or raster-resampling-method in platform/darwin/scripts/style-spec-cocoa-conventions-v8.json.
There was a problem hiding this comment.
If we're going to change it as a platform override, I'd consider we change it in the style-spec and on JS too to keep it consistent. It hasn't gone out in a release yet so not too late to change.
Personally I'm not fussed either way, though raster-resampling is shorter and I don't think is ambiguous compared to raster-resampling-method.
There was a problem hiding this comment.
This is very much a platform-specific nuance. In Objective-C and Swift, MGLRasterResampling on its own is very clearly a protocol name, as opposed to an enumeration name. That’s a product of how we’re automatically generating enumeration names based on style specification properties. We could tweak just the enumeration name without tweaking the property name, but that would be even more confusing than changing it wholesale on iOS/macOS.
As you can see from style-spec-cocoa-conventions-v8.json, there’s quite a lot of precedent for overriding property names on iOS/macOS, but we’re trying to keep a light touch, mostly tweaking parts of speech or avoiding conflicts with Cocoa terminology.
I think raster-resampling is OK in the style specification, where properties often resemble CSS naming convention and don’t have the same language constraints.
| @@ -6,6 +6,27 @@ | |||
|
|
|||
| NS_ASSUME_NONNULL_BEGIN | |||
|
|
|||
| /** | |||
| The resampling/interpolation method to use for overscaling, also known as | |||
| texture magnification filter | |||
There was a problem hiding this comment.
Nit: This would be fixed upstream in the style specification, but there’s a missing period at the end of this documentation comment and the ones for the individual values below. Not critical though.
|
Great work on this @mollymerp 👍 , sorry I created more work for the team by pushing this into GL JS. |
friedbunny
added
iOS
friedbunny
removed
the
needs changelog
port mapbox/mapbox-gl-js#6411
cc @andrewharvey