Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#66 closed defect (fixed)

LayerStore should fire update event when layer property changes

Reported by: bartvde Owned by:
Priority: major Milestone: 0.5
Component: GeoExt.data.LayerStore Version:
Keywords: Cc:
State: Commit

Description

E.g. when the layer's visibility changes, it needs to fire an update event, so that e.g. the LegendPanel can know that the layer should be hidden from the legend.

        /**
         * @event update
         * Fires when a Record has been updated
         * @param {Store} this
         * @param {Ext.data.Record} record The Record that was updated
         * @param {String} operation The update operation being performed.  Value may be one of:
         * <pre><code>
 Ext.data.Record.EDIT
 Ext.data.Record.REJECT
 Ext.data.Record.COMMIT
         * </code></pre>
         */
        'update',

Attachments (2)

ticket66.patch (737 bytes) - added by bartvde 8 years ago.
updated patch with protection against recordIndex of -1
66.patch (5.6 KB) - added by tschaub 8 years ago.
fire update when layer properties change

Download all attachments as: .zip

Change History (16)

comment:1 Changed 8 years ago by bartvde

  • State changed from None to Review

Changed 8 years ago by bartvde

updated patch with protection against recordIndex of -1

comment:2 follow-up: Changed 8 years ago by elemoine

Do we really want to fire an update event whereas no record data has actually been updated?

Instead of duplicating things, couldn't we imagine having the layer store relay all layer-related map events? This may sound overkill as the legend panel could register to map events using layerStore.map.events, but, still, I think it would be nice to just do layerStore.on and layerStore.un to register and unregister map event listeners.

comment:3 in reply to: ↑ 2 ; follow-up: Changed 8 years ago by tschaub

Replying to elemoine:

Do we really want to fire an update event whereas no record data has actually been updated?

Instead of duplicating things, couldn't we imagine having the layer store relay all layer-related map events? This may sound overkill as the legend panel could register to map events using layerStore.map.events, but, still, I think it would be nice to just do layerStore.on and layerStore.un to register and unregister map event listeners.

I'm confused, having the store fire the same events as the map seems like duplication to me. I like the idea of sticking with Ext events as far as we can. It seems entirely sensible that when a record is updated, the update event is fired. Eric, what do you mean "when no record data has been updated"? I consider changing visibility to be changing record data.

comment:4 in reply to: ↑ 3 ; follow-up: Changed 8 years ago by elemoine

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

Replying to tschaub:

Replying to elemoine:

Do we really want to fire an update event whereas no record data has actually been updated?

Instead of duplicating things, couldn't we imagine having the layer store relay all layer-related map events? This may sound overkill as the legend panel could register to map events using layerStore.map.events, but, still, I think it would be nice to just do layerStore.on and layerStore.un to register and unregister map event listeners.

I'm confused, having the store fire the same events as the map seems like duplication to me.

Yes, but I thought we may be able to do that in a generic way, i.e. without having to list (duplicating) every map event type in the LayerStore class. This may actually not be feasible because there's no way to know whether a map event type is layer-related or not.

I like the idea of sticking with Ext events as far as we can. It seems entirely sensible that when a record is updated, the update event is fired. Eric, what do you mean "when no record data has been updated"? I consider changing visibility to be changing record data.

I'm saying that because "visibility" is not a layer record field - layerRecord.get("visibility") returns undefined.

comment:5 Changed 8 years ago by elemoine

  • Resolution fixed deleted
  • Status changed from closed to reopened

Sorry, I by mistake closed this ticket. Reopening.

comment:6 in reply to: ↑ 4 ; follow-up: Changed 8 years ago by tschaub

Replying to elemoine:

Replying to tschaub:

Replying to elemoine:

Do we really want to fire an update event whereas no record data has actually been updated?

Instead of duplicating things, couldn't we imagine having the layer store relay all layer-related map events? This may sound overkill as the legend panel could register to map events using layerStore.map.events, but, still, I think it would be nice to just do layerStore.on and layerStore.un to register and unregister map event listeners.

I'm confused, having the store fire the same events as the map seems like duplication to me.

Yes, but I thought we may be able to do that in a generic way, i.e. without having to list (duplicating) every map event type in the LayerStore class. This may actually not be feasible because there's no way to know whether a map event type is layer-related or not.

I like the idea of sticking with Ext events as far as we can. It seems entirely sensible that when a record is updated, the update event is fired. Eric, what do you mean "when no record data has been updated"? I consider changing visibility to be changing record data.

I'm saying that because "visibility" is not a layer record field - layerRecord.get("visibility") returns undefined.

Of course, but record.get("layer") returns something different before and after the "update" event, right?

comment:7 in reply to: ↑ 6 Changed 8 years ago by elemoine

Replying to tschaub:

Replying to elemoine:

Replying to tschaub:

Replying to elemoine:

Do we really want to fire an update event whereas no record data has actually been updated?

Instead of duplicating things, couldn't we imagine having the layer store relay all layer-related map events? This may sound overkill as the legend panel could register to map events using layerStore.map.events, but, still, I think it would be nice to just do layerStore.on and layerStore.un to register and unregister map event listeners.

I'm confused, having the store fire the same events as the map seems like duplication to me.

Yes, but I thought we may be able to do that in a generic way, i.e. without having to list (duplicating) every map event type in the LayerStore class. This may actually not be feasible because there's no way to know whether a map event type is layer-related or not.

I like the idea of sticking with Ext events as far as we can. It seems entirely sensible that when a record is updated, the update event is fired. Eric, what do you mean "when no record data has been updated"? I consider changing visibility to be changing record data.

I'm saying that because "visibility" is not a layer record field - layerRecord.get("visibility") returns undefined.

Of course, but record.get("layer") returns something different before and after the "update" event, right?

You're right. I'm taking my comment back, Bart's patch is valid. Thanks Tim.

comment:8 Changed 8 years ago by dwins

This patch didn't apply cleanly for me, but looks solid otherwise. I'm just using the GNU patch utility and it's complaining about the patch ending halfway through a line. Something similar is going on for #65.

comment:9 Changed 8 years ago by bartvde

Another question, currently the event is only thrown when the property is visibility, Eric already commented on that, should we throw the event regardless of property?

Possible OL values currently:

  • name, this influences GUI components, so we need to incorporate this as well, although probably rarely used
  • order
  • visibility

But order will give us an add, remove and update event in that case. Does anyone see issues with that?

comment:10 Changed 8 years ago by bartvde

  • State changed from Review to Needs Discussion

Changed 8 years ago by tschaub

fire update when layer properties change

comment:11 Changed 8 years ago by tschaub

  • State changed from Needs Discussion to Review

Ok, with the latest patch, we fire "update" with every "changelayer" *except* for when layer order changes. In that case, the record is removed from the store and added again (at the appropriate index). Because the store does not provide a move method, I think this makes sense. I also don't think it makes much sense to get "remove", "add", *and* "update" all when the layer order changes.

Tests demonstrate what is going on.

comment:12 Changed 8 years ago by ahocevar

  • State changed from Review to Commit

IMO the behavior introduced by tschaub's latest patch makes things very clean and consistent. Please commit.

comment:13 Changed 8 years ago by tschaub

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [863]) The LayerStore now triggers update when layer properties change. r=ahocevar (closes #66)

comment:14 Changed 8 years ago by tschaub

Changes from #45 were erroneously included in r863. Those changes pulled out in r904.

Note: See TracTickets for help on using tickets.