Ticket #66 (closed defect: fixed)

Opened 1 year ago

Last modified 1 year ago

LayerStore should fire update event when layer property changes

Reported by: bartvde Assigned to:
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

ticket66.patch (0.7 kB) - added by bartvde on 05/15/09 12:33:22.
updated patch with protection against recordIndex of -1
66.patch (5.6 kB) - added by tschaub on 05/22/09 21:05:26.
fire update when layer properties change

Change History

05/15/09 11:12:32 changed by bartvde

  • state changed from None to Review.

05/15/09 12:33:22 changed by bartvde

  • attachment ticket66.patch added.

updated patch with protection against recordIndex of -1

(follow-up: ↓ 3 ) 05/18/09 22:45:04 changed 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.

(in reply to: ↑ 2 ; follow-up: ↓ 4 ) 05/18/09 23:05:39 changed 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.

(in reply to: ↑ 3 ; follow-up: ↓ 6 ) 05/18/09 23:25:10 changed by elemoine

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

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.

05/18/09 23:28:53 changed by elemoine

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

Sorry, I by mistake closed this ticket. Reopening.

(in reply to: ↑ 4 ; follow-up: ↓ 7 ) 05/18/09 23:30:54 changed 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?

(in reply to: ↑ 6 ) 05/18/09 23:38:28 changed 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.

05/19/09 18:01:01 changed 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.

05/19/09 19:20:21 changed 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?

05/19/09 19:20:30 changed by bartvde

  • state changed from Review to Needs Discussion.

05/22/09 21:05:26 changed by tschaub

  • attachment 66.patch added.

fire update when layer properties change

05/22/09 21:08:01 changed 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.

05/23/09 13:39:14 changed 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.

05/25/09 00:40:21 changed by tschaub

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

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

05/29/09 18:57:36 changed by tschaub

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