Opened 9 years ago

Closed 9 years ago

#65 closed defect (invalid)

LayerStore should move record instead of removing and adding

Reported by: bartvde Owned by:
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 (2)

datachanged.patch (494 bytes) - added by bartvde 9 years ago.
ticket65.patch (1.0 KB) - added by bartvde 9 years ago.
updated the patch so it works against trunk again

Download all attachments as: .zip

Change History (12)

Changed 9 years ago by bartvde

comment:1 Changed 9 years ago 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.

comment:2 Changed 9 years ago 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).

comment:3 Changed 9 years ago by bartvde

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

comment:4 Changed 9 years ago by bartvde

  • State changed from Needs Discussion to Review

Changed 9 years ago by bartvde

updated the patch so it works against trunk again

comment:5 Changed 9 years ago 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.

comment:6 follow-up: Changed 9 years ago by tschaub

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

comment:7 in reply to: ↑ 6 Changed 9 years ago 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.

comment:8 Changed 9 years ago 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.

comment:9 Changed 9 years ago 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).

comment:10 Changed 9 years ago by bartvde

  • Resolution set to invalid
  • Status changed from new to closed

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

Note: See TracTickets for help on using tickets.