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

Add mapViewModifier + Improve ShapeDataBuilder #14

Merged
merged 5 commits into from
Jan 23, 2024

Conversation

hactar
Copy link
Collaborator

@hactar hactar commented Jan 15, 2024

Currently with the lack of view modifiers, it is difficult to set up the mapView the way one may need it. For example, I see no way of setting showUserLocation, or changing the position of the compass. So I've created a mapViewModifier which allows you to set up the mapView with any of its existing properties, kind of like Introspect does for SwiftUI Views.

I do guess that this project does want to add specific modifiers for all properties at some time - .compassPosition(.topLeft) would be more SwiftUI than setting it up via this solution. But until all these modifiers exist this helper modifier would make this spm way more usable.

For more info see the in-code documentation.

@hactar hactar changed the title Add mapViewModifier Add mapViewModifier + Improve ShapeDataBuilder Jan 18, 2024
Copy link
Collaborator

@ianthetechie ianthetechie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR! Looks great in principle; I love this approach as it provides a completely generic "escape hatch" for now, unblocking devs until we decide the best conveniences to add later. (User location tracking is definitely one of them haha; just haven't figured out the best approach to adding these yet.)

Just one thing to discuss re: the way the mapViewModifier is set.

Sources/MapLibreSwiftUI/MapView.swift Outdated Show resolved Hide resolved
@ianthetechie
Copy link
Collaborator

Oh, and it would also be great if there were a preview showing this in action (basically copy your code to show the user location). These act as informal tests (and ensure we know where we're breaking APIs as it's still unstable) + showcases the functionality.

Copy link
Collaborator

@Archdoog Archdoog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I absolute see the necessity for something like this given how much of the maplibre library we would need to support to truly capture the full scope of the MapView.

However, I also see this making the process of building the right solutions for a specific problem more challenging. Let's take the example where someone's directly manipulating some camera parameter on the MLNMapView then moments later the MapViewCamera is being processed off of state. Basically, we're now allowing untracked state manipulation potentially leading to a host of issues like diverging from the MapViewCamera state or the view redrawing over and over.

I'd almost propose we have a FreeformMapView or an UnsafeMapView with this. It would be a useful tool to understand a feature we need a properly scoped modifier for, etc.

@ianthetechie
Copy link
Collaborator

That's a good point @Archdoog. I think you're right; there need to be some bounds set on this.

@hactar, what are your thoughts on having the callback be invoked one time at some defined point after map initialization (probably post style load?). I think this would address the issue and is probably better practice as it would be much more predictable. It's way too easy to turn this into a footgun as-is (and is probably inefficient to run every update).

@hactar
Copy link
Collaborator Author

hactar commented Jan 22, 2024

I'm im full agreement here that opening this "backdoor" is a bypass, not a solution - but one I'd deem necessary with the current state of things - I'll modify the PR based on your feedback and try to give it a better name so that if anyone uses it they are aware that they may brake state management if used incorrectly.

Regarding the one time callback, while that might be useful for fire and forget set up modifiers (turn off logo for example), it won't work for many basic use cases, for example:

Showing a map with .showsUserLocation = true triggers a location permission request. In my use case I don't want that pop up appearing over the map at launch, I want to trigger it when a user taps the "locate me" button which launches the permission flow. So when the map view first shows, .showUserLocation needs to be false. Only once authorization has been granted do I want to flip .showUserLocation to true. My code for this looks like this:

    @State var showUserLocation = false
// ...
       MapView(styleURL: styleURL, camera: $camera) {
       // layers setup...
            
        }
            .mapViewModifier { mapView in
                mapView.showsUserLocation = showUserLocation
                mapView.compassViewPosition = .topLeft
            }
            .task {
                for await event in await locationManager.startMonitoringAuthorization() {
                    if event.authorizationStatus == .authorizedWhenInUse {
                        showUserLocation = true
                    } else {
                        showUserLocation = false
                    }
                }
            }

And this code wouldn't work if we only used the make portion of UIViewRepresentable... Unless you see some better way of doing this that I'm missing.

@hactar
Copy link
Collaborator Author

hactar commented Jan 22, 2024

Alright - applied changes, updated documentation, added an example and added a test for ShapeSource. Personally I think adding it to "init" would make it too prominent - I don't see a way of making this a "permanent" solution because the more modifiers are added, the more likely someone will try to override something that they should be controlling via a DSL modifier that already exists. But happy to remove the modifier and replace it with an init closure instead if you guys feel thats more in line with what you have planned.

@ianthetechie
Copy link
Collaborator

@hactar thanks for your thorough comments!

@Archdoog and I discussed this today and we agree that...

  1. You have a valid use case which we can't yet think of a better way to address
  2. It isn't going to be detrimental in our current "real-world use case" (building a navigation map view in Ferrostar) as we just won't be exposing this detail to library users there anyways, and
  3. Marking it clearly unsafe and explaining the pitfalls is more of a courtesy to new users than SwiftUI ever gave us 😂

So long story short, we're cool with merging this. I just have a minor suggestion to the docs to make things more explicit as to why it's potentially unsafe.

@hactar
Copy link
Collaborator Author

hactar commented Jan 23, 2024

Done - thanks :)

@hactar hactar requested a review from ianthetechie January 23, 2024 09:22
@ianthetechie ianthetechie merged commit a8a2792 into maplibre:main Jan 23, 2024
1 check passed
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

Successfully merging this pull request may close these issues.

3 participants