LayerStore and bound Map get out of sync when record is replaced through add()

patch with regression test

patch with regression test, without cruft

Sorry about the first bad patch. 57.2.patch is the one.

The same layer can not be added to the map twice. Two records with different layers should not have the same id. So, I think this is a problem in record.copy.

I've created #58 for the copy issue.

I still think there is something that could be done here with the replace listener. Here are my thoughts on this patch:

• I think it is sort of sketchy to call remove in the replace listener for data. Without digging through the code, it is not obvious that this is safe to do. The tests show it works, but they could change the sequence of things so that this breaks without breaking the api (I know this is a common problem we face).

• You could create a removeMapLayer method and have shared code between the onRemove listener and your onReplace listener. (The onRemove method does not actually need to call record.get("layer") twice there.) So the onReplace listener would call removeMapLayer instead of remove.

If #58 seems like a suitable fix, this could also wait (and the tests will need to be tweaked).

I agree that the fact that the same layer gets added twice is weird. record.copy just calls the record constructor with the same data collection (the MixedCollection that is the record's data property) as the copied record, so as far as I can tell the layer object isn't getting duplicated or changed. Maybe there's a problem in the OpenLayers catch on duplicate layers?

But in any case, this problem is more general that just dealing with a copied record where the layer is the same. Records get replaced whenever a new one with the same id is added. But records could have the same id and not have the same layer property.

For example, this could happen:

• a record R gets inserted into the LayerStore
• that recorded is copied to be R' and is inserted into a different store.
• R'.id == R.id at this point.
• Somebody edits R' to have a different layer.
• Somebody adds R' to the first LayerStore.

In this case R would get replaced by R'. The layer of R' would get added to the map, but the layer associated with R would not get removed.

An alternative way I considered writing this was to write in a *beforeadd* event into the LayerStore, and then provide a handler that could check to see if the record would be replaced and do something appropriate in that case. That would work at a higher level and so wouldn't respond to an event on the data collection. Do you think that would be less sketchy?

Otherwise, i agree that sharing some removeMapLayer method between onRemove and onReplace would reduce the sketchiness and be a bit cleaner.

Replying to sbenthall:

I agree that the fact that the same layer gets added twice is weird. record.copy just calls the record constructor with the same data collection (the MixedCollection that is the record's data property) as the copied record, so as far as I can tell the layer object isn't getting duplicated or changed. Maybe there's a problem in the OpenLayers catch on duplicate layers?

Ok, I think some miscommunication. I don't think it is weird that the copied layer gets added to the map. That I expect.

What I wouldn't expect is that the result of record.copy() would replace record when added to a store.

Thanks for clearing that up, and reminding me that the layer gets cloned on copy, which I hadn't realized when I wrote that.

alternative solution (no docs or tests)

57.alternate.patch is another possible solution to this problem that I mentioned above. It doesn't include docs or tests, but if it's a better strategy than 57.2.patch than I'll be happy to fill it out.

Rather than using the non-API data property, it adds a *beforeadd* event to the LayerStore, and then listens for it. If a record with the same layer id as and existing record is added, then the previous record is removed before it is replaced.

Pros:

• LayerStore gets a *beforeadd* method that could be handy.
• doesn't use non-API methods or propertys?

Cons:

• Not sure if what I did to get the superclass and extend the add() method was kosher.
• I think it's more cumbersome than the 57.2.patch solution.

Thoughts?

now with removeMapLayer method

57.3.patch is a new version of 57.patch against current trunk with passing tests and a removeMapLayer method as per tschaub's request.

remove layer on record replace

Thanks for the patch Seb. This looks good to go. I tweaked the tests to be future proof (for when copy does not clone layer #58). If you agree with the changes, please commit.

(In [745]) Prevents LayerStore and Map from getting out of sync when a record is replaced. Thanks to tschaub for future-proofing tests. r=tschaub (closes #57)