Ticket #65 (closed defect: invalid)

Opened 3 years ago

Last modified 3 years ago

LayerStore should move record instead of removing and adding

Reported by: bartvde Assigned to:
Priority: major Milestone: 0.5
Component: GeoExt.data.LayerStore Version:
Keywords: Cc:
State: Needs more work

Description

From the Extjs docs: 

        /**
         * @event datachanged
         * Fires when the data cache has changed in a bulk manner (e.g., it has been sorted, filtered, etc.) and a 
         * widget that is using this Store as a Record cache should refresh its view.
         * @param {Store} this
         */

so layerstore should fire this when the sequence changes. E.g. my legend component needs to know when the order changes.

This patch is trivial so I think this can safely go into 0.1 Please review.

Attachments

datachanged.patch (494 bytes) - added by bartvde on 05/15/09 09:34:30.
ticket65.patch (1.0 kB) - added by bartvde on 05/18/09 08:09:20.
updated the patch so it works against trunk again

Change History

05/15/09 09:34:30 changed by bartvde

  • attachment datachanged.patch added.

05/15/09 09:48:27 changed by bartvde

  • state changed from Review to Needs Discussion.

Hmm, maybe not since instead the LayerStore removes the old record and then adds the new one. So those events get fired which should be enough.

05/15/09 09:54:56 changed by bartvde

On second thought I am not happy with this approach used in the LayerStore. If I have a map with 4 layers, and I move the bottom layer up 1 level, I'll get a remove and add event, and I will draw the added layer on top (since that's the normal procedure). I have no idea that the order of an existing layer has changed, which would cause me to refresh my whole view of the store (datachanged event).

05/15/09 10:13:01 changed by bartvde

  • summary changed from LayerStore should fire datachanged event when order changes to LayerStore should move record instead of removing and adding.

05/15/09 10:15:40 changed by bartvde

  • state changed from Needs Discussion to Review.

05/18/09 08:09:20 changed by bartvde

  • attachment ticket65.patch added.

updated the patch so it works against trunk again

05/18/09 22:18:47 changed by elemoine

Patch looks good to me. I'd just like that we have a unit test showing that we don't trigger add and remove events when a layer changes order.

(follow-up: ↓ 7 ) 05/18/09 22:37:12 changed by tschaub

Apologies if I'm missing something, but "move" doesn't look like a store event.

(in reply to: ↑ 6 ) 05/19/09 08:19:11 changed by bartvde

Replying to tschaub:

Apologies if I'm missing something, but "move" doesn't look like a store event.

Right, it's a proposition for a new event, ofcourse comments would need to be added for the event parameters in an addEvents call. See also:

http://extjs.com/forum/showthread.php?t=13639

Eric, you're right about the unit test, I'll add that as soon as we agree on the aproach.

05/19/09 17:56:55 changed by tschaub

  • state changed from Review to Needs more work.

Ok, so lets keep tickets marked as "needs more work" (or some other state) that are not ready for review.

05/19/09 18:35:52 changed by tschaub

A few reasons I don't think we should add a "move" event:

  • This makes the layer store non-interchangeable with other stores. Anything listening for add/remove (the standard way to detect a move) will miss "move".
  • The "move" event is not layer specific. The things we add to the base classes are layer specific (in the case of a LayerStore).
  • When we can get by with what Ext provides, we should.

The layer container (GeoExt.tree.LayerContainer) listens for add/remove. When you insert a record at a specific index, any "add" listener will be notified of this index. This is all the information you need to order the items in your view.

In the case of the layer container, not all records are represented in the view. Layers with displayInLayerSwitcher set to false are not displayed in the tree. I would imagine the same would be true for a legend panel. So, the index of the record in the store will not always be the index of the item in your view - though it is trivial to determine the proper index. See the recordIndexToNodeIndex and related methods I added to the layer container.

The previous patch for the legend panel was working with respect to reordering layers and not displaying hidden layers. I'm in favor of making the legend panel work without adding a "move" event and sticking with what we get from Ext here (again, because "move" is not layer specific).

05/19/09 19:00:09 changed by bartvde

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

Tim, I agree with your reasoning, marking as INVALID.