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

Bookmark Selection #860

Open
rolson opened this issue Sep 5, 2024 · 8 comments
Open

Bookmark Selection #860

rolson opened this issue Sep 5, 2024 · 8 comments
Labels
bug Something isn't working

Comments

@rolson
Copy link
Contributor

rolson commented Sep 5, 2024

I'm confused about bookmark selection. I can choose a bookmark and it will navigate to that location on the MapView and the bookmark shows as selected:

image

But I can then pan away from that location, or zoom away, and the bookmark stays selected in the Bookmarks component. I'm not sure of the purpose of that.

I wonder if a closure that a bookmark was selected would be a better experience. Because then we wouldn't have to "unselect" a bookmark if the user panned.

@rolson rolson added the bug Something isn't working label Sep 5, 2024
@dfeinzimer
Copy link
Collaborator

dfeinzimer commented Sep 5, 2024

Related issue #246 - original discussion around the selection parameter.

I wonder if a closure that a bookmark was selected would be a better experience. Because then we wouldn't have to "unselect" a bookmark if the user panned.

We have onSelectionChanged(perform:) but deprecated it in #549.

Would niling out the bound selection property on a viewpoint change onInteractivePanningChanged event be an acceptable responsibility of the user?

@philium
Copy link
Contributor

philium commented Sep 5, 2024

Keep in mind that as a mechanism for passing back chosen value, SwiftUI prefers bindings. A selection binding is used by ColorPicker, DatePicker, List, MultiDatePicker, Picker, Table, TextEditor, and TextField. Closure-based callbacks are used for gesture handling (including button taps) and asynchronous file operations (move, export).

What I'm saying is, there's a difference between the mechanism (binding, closure) and how that is reflected in the UI. One may be correct while the other one isn't. It does sound like there may be some UI changes needed, but that doesn't necessarily mean that the selection mechanism needs to change.

@rolson
Copy link
Contributor Author

rolson commented Sep 5, 2024

Would niling out the bound selection property on a viewpoint change be an acceptable responsibility of the user?

Keep in mind that as a mechanism for passing back chosen value, SwiftUI prefers bindings. A selection binding is used by ColorPicker, DatePicker, List, MultiDatePicker, Picker, Table, TextEditor, and TextField. Closure-based callbacks are used for gesture handling (including button taps) and asynchronous file operations (move, export).

Those components listed have a strong concept of a "Selected Value", where that value is then shown in the UI as selected. I'm not sure that really fits this component, does it?

I think the most common use case for this component would be to simply zoom to the bookmark. In that common scenario, I don't think there is any necessary "selected" state like there is for ColorPicker, Picker, etc. Because as soon as the user zooms or pans, the selection should be unset. If we think there is a strong case for selection with this component, I guess it would be better to not put the onus on the user to unset the selection when the map pans/zooms.

@philium
Copy link
Contributor

philium commented Sep 5, 2024

I think the commonality is that they have the concept of picking a value, which may or may not continue to persist in the UI, depending on the UI component and/or how the selected value is used. A picker, for example, may be dismissed as soon as a new value is picked.

Don't get me wrong; I'm actually not necessarily advocating for one option vs. the other. I'm just pointing out that SwiftUI is heavily declarative. Imperative patterns like callback closures are only used when necessary. Declarative patterns like bindings are preferred when possible. That should factor into our decision, whatever we decide.

@dfeinzimer
Copy link
Collaborator

Simulator Screen Recording - iPad Pro 13-inch (M4) - 2024-09-05 at 15 10 10

For large format environments like iPad where the component might remain on screen, I think the persistent selected state is a nice characteristic.

The selection is then easily cleared on a manual pan:

.onInteractivePanningChanged { _ in
    selectedBookmark = nil
}

@rolson
Copy link
Contributor Author

rolson commented Sep 5, 2024

The selection is then easily cleared on a manual pan:

.onInteractivePanningChanged { _ in
selectedBookmark = nil
}

Cool, that's easy enough. I'm happy with that solution. Closing this issue. Thank you.

@rolson rolson closed this as completed Sep 5, 2024
@rolson
Copy link
Contributor Author

rolson commented Sep 6, 2024

I'm going to open this back up for discussion because programmatic setting of the viewpoint (ie, location display) would require more code to unselect the bookmark. How much code is that for the user?

@rolson rolson reopened this Sep 6, 2024
@dfeinzimer
Copy link
Collaborator

programmatic setting of the viewpoint (ie, location display) would require more code to unselect the bookmark. How much code is that for the user?

Maybe minimum 10 lines for basic functionality:

.task {
    for await _ in locationDisplay.$autoPanMode {
        selectedBookmark = nil
    }
}
.task {
    for await _ in locationDisplay.dataSource.$status {
        selectedBookmark = nil
    }
}

Give it a try - on the branch for #862, modify LocationButtonExampleView to merge it with the bookmarks example:

struct LocationButtonExampleView: View {
    @State private var bookmarksIsPresented = false
    
    @State private var locationDisplay = {
        let locationDisplay = LocationDisplay(dataSource: SystemLocationDataSource())
        locationDisplay.autoPanMode = .recenter
        locationDisplay.initialZoomScale = 40_000
        return locationDisplay
    }()
    
    @State private var map = Map(url: URL(string: "https://www.arcgis.com/home/item.html?id=16f1b8ba37b44dc3884afc8d5f454dd2")!)!
    
    @State private var selectedBookmark: Bookmark?
    
    var body: some View {
        MapViewReader { mapViewProxy in
            MapView(map: map)
                .locationDisplay(locationDisplay)
                .task {
                    for await _ in locationDisplay.$autoPanMode {
                        selectedBookmark = nil
                    }
                }
                .task {
                    for await _ in locationDisplay.dataSource.$status {
                        selectedBookmark = nil
                    }
                }
                .overlay(alignment: .topTrailing) {
                    LocationButton(locationDisplay: locationDisplay)
                        .padding()
                        .background(.thinMaterial)
                        .clipShape(RoundedRectangle(cornerRadius: 10, style: .continuous))
                        .padding()
                }
                .toolbar {
                    ToolbarItem(placement: .topBarTrailing) {
                        Button {
                            bookmarksIsPresented = true
                        } label: {
                            Label(
                                "Show Bookmarks",
                                systemImage: "bookmark"
                            )
                        }
                        .popover(isPresented: $bookmarksIsPresented) {
                            Bookmarks(
                                isPresented: $bookmarksIsPresented,
                                geoModel: map,
                                selection: $selectedBookmark,
                                geoViewProxy: mapViewProxy
                            )
                        }
                    }
                }
        }
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants