Added ability to store markers by a user provided id, rather than the markers position

[wpalahnuk]

I've made it so that the user can set an id in the marker options, which is then used to store the marker in the markers hash. Currently if no id is set it will still use the marker position as the hash. I thought I'd see what you thought about this approach before I go any further.

This approach is a bit unwieldy, but it should be backwards compatible with peoples current code

What I'd like to do is take out all the "internal" code that stores the markers by position and make the id mandatory, similar to gm-get-lat-lng. Users could still store markers by position by setting this new required id attribute to the markers position.

I need to add more to the comments and tests, but I thought I'd share what I've done so far to get some feedback.

[kevin1024]

Just some feedback that this was more or less a drop-in fix for my problem. :+1:

The Travis tests are failing due to some jshint errors.

[dylanfprice]

Hey wpalahunk,

First of all thanks a ton for doing this. Sorry it took me so long to get around to reviewing it.

Since it's mostly a proof of concept at this stage I didn't give any inline feedback, but here are my general thoughts on the matter:

• Why did you choose to put the id in the marker options? Not that there's anything wrong with it, just wondering if there's any good reason to put it there vs in another attribute (i.e. 'gm-marker-id'). IMO the attribute approach is more in line with the current api.
• What is the purpose of the getMarker, getMarkers, and setMarkers functions in angulargmContainer? I assume it's so that you can retrieve marker objects if you want to. Not sure how I feel about this. The getMapPromise function is unfortunately necessary but being able to get the markers from anywhere is not. If someone needs to access the marker objects they can write a directive and use the angulargmMapController. That's what it's there for. Hmmm, seems like a lot of people are wanting to get at the markers so maybe I need to include better documentation about the proper way to go about doing that.
• My vision for this is that internally we completely replace the concept of hash with id. For backwards-compatibility we can use the lat/lng as our id, but this should basically be a couple lines of code in a couple places and everything else just assumes we have an id. This will pave the way for requiring the id in the future.
  • gm-events will also switch to using ids, but for backwards compatibility we can accept google.maps.LatLng objects and convert those to 'position based ids'.
  • a new requirement of the id-based api will be that ids must be unique
  • in the case where someone has multiple markers in the same location, but has not switched to the id-based api, I am fine with silently failing on the extra markers since only one useful marker can show up on the map anyway and the current api doesn't specify any behavior in this case
  • we should switch gm-polylines to an id-based api at the same time. This should allow us to more easily implement events for that directive as well.

I'll be thinking about this some more and hopefully we can put together a decent implementation in the next week. Let me know if you have any thoughts. Once again, thanks for your contributions so far!

[wpalahnuk]

• I just put the id in the marker options because it was easier and makes it more backwards compatible. Having gm-markers-id makes more sense, I was just gonna wait and see what you said before I implemented it.
• The purpose of the getMarker, getMarkers, and setMarkers is like you said, to retrieve the marker objects. My use case for this is programmatically opening infoWindows on certain markers, for example you could open a certain markers infoWindow based on a URL parameter. Having a separate directive is probably a better idea, but again just putting it in the container was simpler for now.

When I have some time over the next few days I'll update this PR

[dylanfprice]

Cool, I'll trying to find a good chunk of time to merge your PR and finish this feature off. Hopefully soon...

[wpalahnuk]

I've updated the PR,

I moved the id to an attribute and deleted the old latlng bases functions getMarker, addMarker, etc and replaced them with id based ones getMarkerById, addMarkerById, etc.

[dylanfprice]

Sweet. I have started merging in your PR, I'm combining it with a refactor I've wanted to do for a while. You can see the progress in the markers-by-id branch.

I decided to just go ahead and make breaking api changes since I don't have the bandwidth to deal with the added complexity of supporting legacy apis. Expect to see a release soon!