amay077 / Xamarin.Forms.GoogleMaps

Map library for Xamarin.Forms using Google maps API
https://www.nuget.org/packages/Xamarin.Forms.GoogleMaps/
MIT License
546 stars 347 forks source link

Improved Binding Support #521

Closed dansiegel closed 5 years ago

dansiegel commented 6 years ago

Description

Adds ItemsSource with ItemTemplate and SelectedItemCommand/Event. fixes #519

amay077 commented 6 years ago

Great and beautiful PR!

I have few suggestions and request.

Changing property names

Because I want to follow original Xamarin.Forms.Maps specification .

Duplicate event and EventArg class

Seems SelectedPinChanged(exists) is the same as SelectedItemChanged(new).

Do you need SelectedItemChanged and SelectedItemChangedEventArgs?

Request

Could you add the sample for this feature to XFGoogleMapSample ? Because I'm not very familiar with how to use DataTemplate.

dansiegel commented 6 years ago

Happy to add it to the sample. As for the other issues you brought up.

  • ItemsSource -> PinsSource
  • ItemTemplate -> PinTemplate
  • ItemSelectedCommand -> PinSelectedCommand

The naming was done to be consistent with other Xamarin.Forms API's such as the ListView and TabbedPage. Also it is only loosely tied to Pins, so naming it Pin{whatever} would get confusing as you would lead people to thinking it is an actual Pin. The items bound to the ItemsSource are not necessarily Pins. In fact if you're following an MVVM design they should never be Pins.

Because I want to follow original Xamarin.Forms.Maps specification

If you look at the Xamarin.Forms.Maps codebase you'll see that the most of the files haven't even been touched since the initial import 2 years ago, a few classes had some very minor tweaks around internals and what they're visible to. Beyond that there have been no changes making it stagnant. Point being the Xamarin.Forms.Maps API was done in an era when developers were writing all of their Pages in code. Today with XAML and MVVM design being the prominent design pattern developers have different needs.

Seems SelectedPinChanged(exists) is the same as SelectedItemChanged(new).

Do you need SelectedItemChanged and SelectedItemChangedEventArgs?

These actually aren't duplicates. They are similar in concept, but not duplicate. The SelectedPinChanged raises the Pin, where the SelectedItemChanged as well as the ItemSelectedCommand are both focused on the Item. The item is a abstract concept that would be in the ItemsSource and used as the BindingContext of the Pin rather than the Pin itself. Whereas the SelectedPinChanged is all about providing the Pin itself.

dansiegel commented 6 years ago

worth noting, if I add the sample it won't build given that you are referencing the published NuGet instead of the code. I'm still happy to add it if that's what you want.

amay077 commented 6 years ago

You can use Xamarin.Forms.GoogleMaps_with_sample.sln what is all projects contained solution (not using nuget) .

I hope you add sample code to it. :wink:

galadril commented 6 years ago

+1

I've also extended the GoogleMaps project to bind more items: Pins Polylines CameraPosition

So i can use this map component like:


    <!--  Map  -->
         <cntrl:ExtendedMap
            x:Name="map"
            Grid.Row="0"
            CameraPosition="{Binding CameraPosition}"
            IsVisible="{Binding Status, Converter={StaticResource StatusConverter}, ConverterParameter=Data}"
            MapType="Street"
            Pins="{Binding Pins}"
            Polylines="{Binding Polylines}" />

So if your extending the bindings.. :) Please do Pins, Polylines and CameraPosition as well 👍

dansiegel commented 6 years ago

@amay077 are you good with the addition of Bindable Pins, Polylines, and CameraPosition? Also I'm going to update the sample app so the platforms are using PackageReferences instead of the older package.config style... it'll be nicer to restore from the global cache rather than duplicating a cache for the project. If you're ok with it I can add the Mobile.BuildTools package as well to handle the injection of the API Key's

seanyda commented 5 years ago

I have also extended this control to handle the same thing but I write the UI in C#.

new Map {
                        Behaviors = {
                            new UpdateRegionBehavior {
                                Region = new Binding("Region")
                            },
                            new BindingPolylinesBehavior {
                                ItemSource = new Binding("TrackingInfo")
                            },
                            new BindingPinsBehavior {
                                ItemSource = new Binding("MapPins")
                            }
                        }
                    },

It would be neat to see something like this merged into master :)

andreinitescu commented 5 years ago

@dansiegel It's not clear to me how the ItemTemplate is supposed to work. Looking at your implementation here you expect the DataTemplate to be materiazed into a Pin:

        if (content is Pin pin)
        {
            Pins.Add(pin);
        }
        else
        {
            Forms.Internals.Log.Warning("ERROR", $"The DataTemplate returned a '{content.GetType().Name}' instead of expected type 'Pin'.");
        }

But Pin cannot be used in XAML within a DataTemplate, the following won't compile:

        <PinTemplate>
            <DataTemplate>
                <Pin />
            </DataTemplate>
        </PinTemplate>

Could you please give some info on how it needs to work? Thanks!

amay077 commented 5 years ago

Sorry I'm very very late. I merged this PR but I will refactor according like @andreinitescu's PR.