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

MLNStyleProperty override generated names #16

Closed
hactar opened this issue Jan 22, 2024 · 4 comments
Closed

MLNStyleProperty override generated names #16

hactar opened this issue Jan 22, 2024 · 4 comments

Comments

@hactar
Copy link
Collaborator

hactar commented Jan 22, 2024

While implementing properties for the CircleStyleLayer I noticed something I think is "unswifty". MapLibre likes adding the parent name to properties. So the radius of a MLNCircleStyleLayer is called "circleRadius". When using the property wrapper, this name is then passed through to the DSL:

@MLNStyleProperty<Double>("circleRadius", supportsInterpolation: true)
@MLNStyleProperty<UIColor>("circleColor", supportsInterpolation: false)
@MLNStyleProperty<Double>("circleStrokeWidth", supportsInterpolation: true)
@MLNStyleProperty<UIColor>("circleStrokeColor", supportsInterpolation: true)

This then leads to a call site like this:

            CircleStyleLayer(identifier: "simple-circles", source: pointSource)
                .circleRadius(constant: 16)
                .circleColor(constant: .systemRed)
                .circleStrokeWidth(constant: 2)
                .circleStrokeColor(constant: .white)

I think it would be more "swifty" if the call site was this:

            CircleStyleLayer(identifier: "simple-circles", source: pointSource)
                .radius(constant: 16)
                .color(constant: .systemRed)
                .strokeWidth(constant: 2)
                .strokeColor(constant: .white)

The same goes for the already existing PolylineStyleLayer, where every modifier starts with .line.

Would be great if the macro could "auto" do this, but if this is not possible, then I propose a second call syntax similar to this:

@MLNStyleProperty<Double>(mapLibreName: "circleRadius", dslName: "radius", supportsInterpolation: true)

This then generates modifiers with the dslName, calling the mapLibreName in the background.

I haven't dug into macros yet, so can't submit a PR without a lot of learning first 😔 - hope you could take care of this if you agree.

PS: I'd also remove the "constant" property labels here to more closely match SwiftUI: .strokeColor(.systemRed)

@ianthetechie
Copy link
Collaborator

Both excellent points; thanks for pointing this out! I'll either knock this out over the weekend or write back with an update :)

@ianthetechie
Copy link
Collaborator

Sorry it took me a while to get come up with a more complete response far this @hactar!

Re: the question of un-idiomatic names, this is really just an issue of zero docs on my part. The way this macro works is to just create the property declarations and modifiers on the intermediate (more swift-y) type.

Part of the reason I haven't added dozens of properties no any layer yet is because I haven't yet found a great macro solution for generating functions like makeMLNStyleLayer yet. These basically map internal props 1:1 for now but all the code is copypasta (see Line.swift).

TL;DR, feel free name properties in the swiftiest manner possible; we can deal with any mismatch fallout when we have more code generation.

Tangent: I am not optimistic about our ability to always have the macro select the right identifier to be honest. See backgroundColor as an example property which exists on multiple layer types and whose name cannot be obviously simplified without losing context. This will require a human touch I think.

Finally, regarding the question of removing constant property labels, I just had a look over this and remembered why I wrote it this way to begin with. I was to be an intentional contrast to other sorts of properties (ex: interpolated). I thought this made things clearer at the time, but this is not a strong opinion. Thoughts on whether we should leave this or change given that context @hactar @Archdoog?

@hactar
Copy link
Collaborator Author

hactar commented Jan 28, 2024

Ah thanks - I was thinking that the macros are doing something magical in the background to "hook in" - I see now that I can name them what I want and just have to map accordingly in makeMLNStyleLayer. I've updated my circle layer PR accordingly.

Regarding "constant" - thanks for the context. Imho the intent can be inferred by the easy case not having a label, while proving "interpolated" and potentially other argument labels for the advanced cases - in-line with how SwiftUI has Image("assetName") but also Image(systemName: "mappin") and Image(uiImage: ...). The fewest devs will ever be using anything other than the "constant" case.

@hactar
Copy link
Collaborator Author

hactar commented May 14, 2024

Closing this. Improved via #23.

@hactar hactar closed this as completed May 14, 2024
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

No branches or pull requests

2 participants