angular-ui / ui-leaflet

AngularJS directive to embed an interact with maps managed by Leaflet library
http://angular-ui.github.io/ui-leaflet
Other
314 stars 134 forks source link

args.model of leafletDirectiveMarker doesn't return correct marker if one of marker is deleted #221

Open hkcity1111 opened 8 years ago

hkcity1111 commented 8 years ago

Hi all

If I define 3 marker in the map and use $scope.$on(leafletDirectiveMarker.mapid.click, function(event, args)){..}) to extract args.model which contain "marker 2" if I click "marker 2" Marker.

But if I delete "marker 2" in $scope.markers and click "marker 3" in the map, arg.model in leafletDirectiveMarker.click event listener return "marker 2" data to which is deleted already in $scope.markers.

Would anyone know to fix this issue? Thanks a lot

Regards, Kenneth

$scope.markers = [{
        lat : -700, lng : -200, focus : true, title : "Marker 1",
        label : {
            message : "Mark 1",
            options : {
                noHide : true
            }}}, {
        lat : -300, lng : -300, focus : true, title : "Marker 2",
        label : {
            message : "Mark 2",
            options : {
                noHide : true,
            }}}, {
        lat : -600, lng : -100, focus : true, title : "Marker 3",
        label : {
            message : "Mark 3",
            options : {
                noHide : true,
            }}}];
elesdoar commented 8 years ago

Hi @hkcity1111, please write code into markdown tags for code. https://help.github.com/articles/creating-and-highlighting-code-blocks/

nmccready commented 8 years ago

Hah, I just fixed it

hkcity1111 commented 8 years ago

Thanks nmcredy. I use '' before which make the code concatenate into one line. I know use triple ' now. Thanks all for reminder.

BlakeDraper commented 8 years ago

@nmccready Does that mean that ui-leaflet is now updating the markers watchers in the background? I am still struggling with this - not sure it is fixed. Random string key value assignment for the individual markers seems like the only way to ensure the args.model gets updated when swapping out the markers with a new set.

marsmith commented 8 years ago

also referenced in #239 -- plnkr that demonstrates this issue:

http://plnkr.co/edit/R6GqVkdCKDl6BgB50sTq?p=preview

MattSidor commented 8 years ago

Is the proposed change from @vincent-ollivier-everysens in issue #240 relevant to this?

Drj5 commented 8 years ago

Hi,

to fix this issue, I think that you just have to modify _seeWhatWeAlreadyHave function in markers directive. This line doesn't detect changes to markers who were after deleted marker : equals = angular.equals(newMarker, oldMarker) && isEqual You can replace it with : equals = angular.equals(newMarker, oldMarker) === isEqual

It seems working well, because isEqual==false when _seeWhatWeAlreadyHave is called to test if marker's data is modified. After, it will call cb function, which is going to refresh current marker's data. if (!isDefined(markerModels) || !Object.keys(markerModels).length || !isDefined(markerModels[name]) || !Object.keys(markerModels[name]).length || equals) { if (cb && Helpers.isFunction(cb)) cb(newMarker, oldMarker, name); }

oenie commented 6 years ago

It looks to me as if the proposed changes from issue #240 don't work. In the same issue there is a reference to a merged solution #273. The suggest fix of @Drj5 however seems to do the trick for me. Any chance this could get fixed ? Or is this isEquals check there for a wholly different purpose ?

oenie commented 6 years ago

OK, I think I have found the problem that the original poster was experiencing, and it's related to the way he's (and we too) using ui-leaflet:

You have to provide the marker array (in this case $scope.markers) as a associative array instead of an indexed array.

By just using an indexed array, the _seeWhatWeAlreadyHave function will match the wrong indexes and remove the wrong data from the markers list.

@Drj5, although your 'fix' seemed to work, it did cause other tests in the ui-leaflet test-suite to fail.

Drj5 commented 6 years ago

Hi,

I'm working with this "fix" from 2 years now without any problem. But you're right, if test-suite is failing that's not a good solution. I prefer using indexed arrays because it's a little bit faster than associative arrays (objects in JS)...