Ticket #58 (closed enhancement: fixed)

Opened 11 months ago

Last modified 6 months ago

layer and feature record copy should not clone layer and feature

Reported by: tschaub Assigned to: bbinet
Priority: major Milestone: 0.6
Component: GeoExt.data Version:
Keywords: Cc:
State: Commit

Description (Last modified by tschaub)

When we copy a layer or feature record, we clone the OpenLayers object. If you clone a layer, you can add it to a map that already has the original. If you copy a layer record and add it to a store that is bound to a map, the original record gets replaced in the store and the cloned layer gets added to the map (see #57).

We should not be using copy if we expect a deep copy. This is the case with any Ext record.

>>> var R = new Ext.data.Record.create([{name: "foo"}]);
>>> var r = new R({foo: {bar: "deep"}});
>>> var r2 = r.copy();
>>> r2.get("foo").bar = "shallow";
"shallow"
>>> r.get("foo").bar
"shallow"

So, the same should be true for the layer/feature. If we want a deep copy, we should add a different method.

Attachments

58.patch (3.7 kB) - added by tschaub on 05/15/09 01:49:15.
keep what we get from ext
patch_58-r1327-A0.diff (2.2 kB) - added by bbinet on 08/17/09 17:25:35.

Change History

05/01/09 22:52:19 changed by sbenthall

While I agree that if the layer is cloned, then the copied record should not have the same id, I think it's not ideal to break from the Ext's spec for copy() unless very necessary. (Ext's Record.copy() will preserve the original's id by default). I think that could be shooting us in the foot if we make heavier use of Ext's data-handling functionality down the line. Based on my sense of the Store and Records API, I think there might be good reason copy() is designed that way.

Why not two methods: copy(), which maintains the Ext model of keeping the old id by default, and doesn't clone the layer; and clone(), which clones the layer and guarantees a new id?

05/01/09 23:37:16 changed by tschaub

Ok, we're in agreement here now.

So, we should not be using copy if we expect a deep copy. This is the case with any Ext record.

>>> var R = new Ext.data.Record.create([{name: "foo"}]);
>>> var r = new R({foo: {bar: "deep"}});
>>> var r2 = r.copy();
>>> r2.get("foo").bar = "shallow";
"shallow"
>>> r.get("foo").bar
"shallow"

So, the same should be true for the layer.

05/01/09 23:37:45 changed by tschaub

  • summary changed from layer and feature record copy should not maintain id by default to layer and feature record copy should not clone layer and feature.

05/14/09 17:15:17 changed by tschaub

  • owner set to tschaub.
  • status changed from new to assigned.

05/15/09 01:40:33 changed by tschaub

  • description changed.

I'm changing the ticket description to clarify things. The initial comments here reflect the history that lead to a better understanding.

05/15/09 01:41:01 changed by tschaub

  • description changed.

05/15/09 01:49:15 changed by tschaub

  • attachment 58.patch added.

keep what we get from ext

05/15/09 01:49:43 changed by tschaub

  • state changed from None to Review.

Tests pass. Thanks for the review.

05/18/09 16:49:16 changed by sbenthall

  • state changed from Review to Commit.

Looks good to me. Thanks, Tim!

05/18/09 19:04:47 changed by tschaub

  • status changed from assigned to closed.
  • resolution set to fixed.

(In [805]) The copy method for layer and feature records does *not* clone the respective OL object. r=sbenthall (closes #58)

08/17/09 17:16:40 changed by bbinet

  • status changed from closed to reopened.
  • resolution deleted.

I am currently interested in having a clone method that would be similar to the old copy method as discussed in previous comments.

This clone method would be useful at least for LayerRecord to be able to make a deep copy of an existing layer and apply a custom style to it.

08/17/09 17:25:35 changed by bbinet

  • attachment patch_58-r1327-A0.diff added.

08/17/09 17:27:20 changed by bbinet

  • state changed from Commit to Review.
  • type changed from defect to enhancement.

patch_58-r1327-A0.diff adds a clone method to LayerRecord. I don't know if the same clone method for FeatureRecord would be useful.

09/09/09 13:58:01 changed by elemoine

  • milestone changed from 0.5 to 0.6.

This needs to be deal with for 0.6.

09/11/09 21:11:45 changed by sbenthall

  • state changed from Review to Commit.

Looks good to me. Please commit.

09/11/09 21:41:56 changed by sbenthall

  • owner changed from tschaub to bbinet.
  • status changed from reopened to new.

09/14/09 10:09:33 changed by bbinet

  • status changed from new to closed.
  • resolution set to fixed.

(In [1362]) adds a clone method to LayerRecord in order to perform a deep copy. r=sbenthall (closes #58)