Skip to content
This repository has been archived by the owner on Nov 30, 2018. It is now read-only.

Markers positions are not updated when models coords change. #1350

Closed
quentinlampin opened this issue Jun 5, 2015 · 10 comments
Closed

Markers positions are not updated when models coords change. #1350

quentinlampin opened this issue Jun 5, 2015 · 10 comments

Comments

@quentinlampin
Copy link
Contributor

Hello there,
I'm trying to dynamically add and update markers positions according to a data model. Adding new 'markers' to the model works well, i.e. new markers are shown up on the map, but changing their coordinates does not work, markers stay at their original coordinates.

Here is a plunker of the issue : http://plnkr.co/edit/94GZ6QaCYrlonNzkMhe9?p=preview

I'm novice (2 weeks on JS, node, grunt, etc....... x/ ) so I'm certainly wrong but could it be that 'coords' are not $watched? Or is it by design, i.e. involving some sort of perf. trade-off? In that case, how would I implement this, and what kind of performance hit should I expect for, say, 50ish markers ?

I'm sorry if that question has already been answered or if the docs cover that point, I did my best solving this by myself but coffee and good will didn't suffice this time ;)

Regards,

Quentin

@rummelsworth
Copy link
Contributor

At the end of moveNodes I inserted a call to refresh on the map directive's control object, but that didn't seem to help. Then I tried inserting a call to managerDraw on the markers directive's control object, but still no dice. Then I tried setting doRebuildAll to true on the markers directive in the template, and that worked (but performance may suffer with enough markers).

I also updated the plunker to AGM 2.1.2 with lodash 3.9.3 and Angular 1.3.15:
http://plnkr.co/edit/9cGPFUUUoYS0UQhgXLfl?p=preview

I found an earlier issue discussing this briefly with the same resolution, #1022. It seems this or something like it used to be a bug: #946 Maybe it's resurfaced somehow @nmccready? Or is doRebuildAll what's supposed to be used in cases like this?

@nmccready
Copy link
Contributor

It should update . The code responsible for see what is new is here https://github.com/angular-ui/angular-google-maps/blob/master/src/coffee/directives/api/utils/models-watcher.coffee#L12-L31 . It relies on whatever the implementation is for the comparison callback.

@nmccready
Copy link
Contributor

Uses the return from figureOutState and does the deletes , adds and updates here. https://github.com/angular-ui/angular-google-maps/blob/master/src/coffee/directives/api/models/parent/markers-parent-model.coffee#L148-L164

I do not have time to debug this right now. So hopefully someone else has the time.

@rummelsworth
Copy link
Contributor

In the call to comparison at models-watcher.coffee:25, m and child.clonedModel are always deeply equal but not reference-equal, for this issue's example at least. So the updates property is always empty.

comparison is doing its job I think. It seems like whatever entities are creating/maintaining the scope.models and childObjects objects used in figureOutState might have a problem. Spent a few minutes trying to sort out where those objects might be being mishandled. Didn't succeed.

Fwiw, scope and _this.scope in the call to figureOutState in pieceMeal are reference-equal. Not sure if that's an okay/expected/permissible situation.

Will try to pursue this later.

@nmccready
Copy link
Contributor

Fwiw, scope and _this.scope in the call to figureOutState in pieceMeal are reference-equal. Not sure if > that's an okay/expected/permissible situation.

Yes they should be equal, sometimes it is using the local reference of scope if the function was possibly intended to be outsourced as a module to be included .

@nmccready
Copy link
Contributor

Digging appreciated.

@quentinlampin
Copy link
Contributor Author

Hello guys,

I think I got it!

in MarkerChildModel constructor and
MarkerChildModel.prototype.updateModel

the cloneModel is created/updated using
'''
_.extend({}, model)
'''

This must somehow clone the object by reference.

Replacing
'''
_.extend({}, model)
'''
by:
'''
_.clone(model, true)
'''

makes the markers behave the way it's supposed to.

Plunker update to reflect the changes:
http://plnkr.co/edit/PgsU8PPS1CJorTSD1H7B?p=preview

2015-06-08 22:24 GMT+02:00 nmccready notifications@github.com:

Digging appreciated.


Reply to this email directly or view it on GitHub
#1350 (comment)
.

Quentin Lampin

@quentinlampin
Copy link
Contributor Author

I dug a bit more and discovered a comment stating that you made the move from _.clone to _.extend to get rid of the dependency to lodash, which allows to depend on underscore instead.
I took a look at the documentation and here is what it states:

extend_.extend(destination, *sources) 
Copy all of the properties in the source objects over to the destination object, and return the destination object. It's in-order, so the last source will override properties of the same name in previous arguments.

_.extend({name: 'moe'}, {age: 50});
=> {name: 'moe', age: 50}

It doesn't say if the copy is shallow or deep so it did a bit of testing :
http://plnkr.co/edit/K9ltgR5RnweCEE4bs7HD?p=preview

it appears that it's not a deep-copy, i.e. object properties are copied by reference.

If it's really important to get rid of the lodash dep, one could replace _.clone by:

clone: (obj) ->
    if not obj? or typeof obj isnt 'object'
      return obj

    if obj instanceof Date
      return new Date(obj.getTime()) 

    newInstance = new obj.constructor()

    for key of obj
      unless key == '__history__'
        newInstance[key] = @clone obj[key]
      else
        newInstance[key] = []

    return newInstance

@quentinlampin
Copy link
Contributor Author

@nmccready @wrummler Pull-request created : #1356.

@nmccready
Copy link
Contributor

👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants