Map Component/Panel

I have worked with both and I do like the fact that the MapPanel creates the map at render, so I am in favour of that.

I did some adaptations to the MapPanel, which can be found here:

http://www.osgis.nl/geoext/legend/MapPanel.js

Now that the mappanel does not have a way to add layers and controls, the center, zoom and extent options do not make sense any more. These can be passed with the map object or the map configuration object. As a consequence of this, the gx_onRender method would have to take center, zoom and extent from the map object instead of the mappanel.

Also, it would probably be better to do the setCenter/zoomToExtent stuff only if the map has layers already, so people who create their layers dynamically (e.g. from WMC) can do that after the mappanel is rendered.

I'm not sure I agree with relying on the map object's "center", "zoom" and "extent" properties. Those are private properties that aren't meant to be configured through the config object passed to the map constructor (the "extent" property doesn't even exist). With the current OpenLayers code what is the effect of setting "center" and "zoom" in the config options?

I agree that setCenter/zoomToExtent should be called only if the map has layers.

Thanks for looking at the code Andreas.

Replying to elemoine:

I'm not sure I agree with relying on the map object's "center", "zoom" and "extent" properties. Those are private properties that aren't meant to be configured through the config object passed to the map constructor (the "extent" property doesn't even exist). With the current OpenLayers code what is the effect of setting "center" and "zoom" in the config options?

You are right Eric. I had not thought about that.

I'm currently reviewing the drake application to see if the MapPanel meets every requirement there.

patch-4-r186-A1.diff complies with the new patch for OpenLayers #1901 and takes Andreas' comments into account (setCenter/zoomToExtent should be called only if the map has layers).

mappanel.2.patch adds a layers property to the mappanel. This is a store with records representing the map's layers array. The store is synced with the layers array and vice versa.

Some comments:

• I think it would be good to have a method for creating layer records, something like that:

    /**
     * Property: LayerRecordType
     * {Function} The layer record constructor.
     */
     LayerRecordType: Ext.data.Record.create({"name": "layer"}),

    /**
     * Method: createLayerRecord
     * Given a layer create a layer record.
     *
     * Parameters:
     * layer - {OpenLayers.Layer} A layer object.
     *
     * Returns:
     * {Ext.data.Record} The layer record.
     */
    createLayerRecord: function(layer) {
        return new this.LayerRecordType({layer: layer}, layer.id);
    },

We can make this method private for now, and consider to make it public later, possibly by moving it outside the MapPanel.

• if a record is added to the store then onAddLayerRecords triggers then onAddLayer triggers, onAddaLayer will attempt to add a new layer record, maybe that's not a problem because Ext will check the record id before actually adding the record, can you confirm this? (unit tests should help here)

Eric: thanks for your comments. I separated the LayerStore out to its own class, and it has addLayer and removeLayer methods now (addLayer is like your suggested createLayerRecord method). Adding layers to the map and layer records to the store will not lead to duplicate layers, because the methods for both actions make sure that the layer/record is not already there.

Apart from that, I admit that unit tests are still missing. So if anyone feels like writing some, that would be greatly appreciated.

mappanel.4.patch contains tests for the MapPanel/LayerStore interaction.

However, the tests crash when mapPanel.layers.removeLayer(layer) is called.

(Also, I just noticed that L40 of the MapPanel.html tests file should be

t.plan(6)

not

t.plan(5)

new patch mappanel.5.patch. The issue with the failing tests is not yet resolved, but I added a guess function to the GeoExt.data.LayerStore namespace. This can be used in applications that have just one mappanel to guess the layerstore.

I'll make sure that all tests pass as soon as I can. Thanks sbenthall for writing the tests!

mappanel.6.patch fixes the layer store issue unveiled by sbenthall's unit tests, and adds additional tests.

Tests pass in FF3, patch depends on OpenLayers #1901

Thanks for any review.

I'd like to review this, if that can wait until the 16th.

My review comments:

• the "layers" property in the MapPanel class is documented as being of type Ext.data.Store, isn't always of type GeoExt.data.LayerStore?

• I'm concerned with the LayerStore class inheriting from Ext.data.Store (especially if we're considering other uses of that class, outside the MapPanel). That class could not be used if one would for example need a GroupingStore of layer records.

• LayerStore.js does not have the same copyright header as MapPanel.js. We can also make our code BSD now that we've settled down on BSD during our last PSC meeting.

• the "map" property of the LayerStore class is declared with "configProperty" in the API doc, is this an ND keyword?

• the examples still refer to version 2.2 of Ext

nd config for additional keywords like "ConfigProperty"

Replying to elemoine:

My review comments: * the "layers" property in the MapPanel class is documented as being of type Ext.data.Store, isn't always of type GeoExt.data.LayerStore?

Right now it is, but it won't any more after the change I have to make to address your next comment.

* I'm concerned with the LayerStore class inheriting from Ext.data.Store (especially if we're considering other uses of that class, outside the MapPanel). That class could not be used if one would for example need a GroupingStore of layer records.

Ok, will create factory methods to create a store that syncs with the map's layers and to create layer records. Patch to come.

* LayerStore.js does not have the same copyright header as MapPanel.js. We can also make our code BSD now that we've settled down on BSD during our last PSC meeting.

Ok, we can change the license, but we have no "go" on the OSGeo copyright yet. We can bulk change that on the whole trunk without extra tickets once we have that.

* the "map" property of the LayerStore class is declared with "configProperty" in the API doc, is this an ND keyword?

It is a custom keyword, see attachemnt Topics.txt on this ticket. We will want a separate ticket for creation of an apidoc_config folder on trunk with all nd settings.

* the examples still refer to version 2.2 of Ext

ok, will change in the patch to come.

mappanel.7.patch is a new patch which addresses all of Erics concerns. Please review again, and I really hope we can get this into trunk today to finally start growing the codebase.

Tests pass in FF2, FF3 and IE7, examples work in all mentioned browsers.

Hi Andreas, thanks for taking my comments into account and for the new patch. Some new comments:

• in the loadMapPanel method of the MapPanel.html and data.html test files the mapPanel variable is global, is this intentional?

• although I like the factories better than what was in the previous patch, I'm still concerned with one thing: there's no way to unregister the listeners registered by the createLayerStore function. What if one uses the createLayerStore to create a layer store synchronized with a map instance, and later wants to deactivate that synchronization?

Regarding the above point: that's why I've been implementing classes to deal with such synchronization work, look at my new LayerStoreMediator implementation for an example. That said, I don't want to block the process of committing this MapPanel into the trunk, especially if I'm the only one concerned with this. If you and others still think we should go ahead and commit this patch as-is, could you just make the factory functions non-API so that we can go back and fix this if we need to? Yet I'd really like to have other opinions; Tim would you take a look at this?

Replying to elemoine:

Hi Andreas, thanks for taking my comments into account and for the new patch. Some new comments: * in the loadMapPanel method of the MapPanel.html and data.html test files the mapPanel variable is global, is this intentional?

Do we care? I have taken the tests and examples as sbenthall and tcoulter created them, and made a few tweaks. I did not feel bad about these global functions. I am grateful that they contributed tests and examples.

* although I like the factories better than what was in the previous patch, I'm still concerned with one thing: there's no way to unregister the listeners registered by the createLayerStore function. What if one uses the createLayerStore to create a layer store synchronized with a map instance, and later wants to deactivate that synchronization?

Is this a common use case? I'd say if someone wants that, then this can get its own ticket, and I can think of many ways to accomplish this (even with the factory).

Regarding the above point: that's why I've been implementing classes to deal with such synchronization work, look at my new LayerStoreMediator implementation for an example. That said, I don't want to block the process of committing this MapPanel into the trunk, especially if I'm the only one concerned with this. If you and others still think we should go ahead and commit this patch as-is, could you just make the factory functions non-API so that we can go back and fix this if we need to?

Are we really committing to a stable API before 1.0?

In my opinion, for these initial patches towards a 0.1 release we should not let the perfect be the enemy of the good. Something with fewer features is better than nothing with lots of features. And it will be easier for others to develop the patches for features they care about once some code is in the trunk.

I agree with Seb here. +1 to commit this and start on the codebase.

A few comments on irc.

Basically, we have less control if we have MapPanel (or similar components) register for their own events.

If you modify the mappanel-window.html example and MapPanel.js a bit, you can see this pretty clearly. I've added the following to the example (just below maxExtent config property):

                listeners: {
                    "render": function() {
                        console.log("app render");
                    },
                    "bodyresize": function() {
                        console.log("app bodyresize");
                    }
                }

Then in MapPanel.js, I threw some console.log calls in gx_onRender and gx_onBodyResize. The result in the console is this:

app render
lib render
app bodyresize
lib bodyresize

If we do things this way, we don't let app designers register for events with the promise that the map (or whatever) is available when they might guess it is.

I also agree 100% that we just need to get some code in the trunk. However, I think the design where we mimic Ext is better than creating brand new conventions (a bunch of factory methods).

Eric, I think you and I share the inheritance aversion. However, I think we keep things simpler for new users if we copy Ext style. People expect object constructors with capital letters. I know that seems trivial, but it is sort of the only difference. Of course we want to avoid code duplication. It is easy to make a LayerStore and a GroupLayerStore share code.

A fine example (in my mind) is the JsonStore. This is basically a convenience constructor for configuring a Sore with a specific Reader. Instead of being named createJsonStore it is named JsonStore.

An obvious practical difference that has some benefits is that by extending ext constructor prototypes, we can give things xtypes (not an issue for a store I know) and other people, if they wish, can extend our constructor prototypes and do the same. Finally, if there is a need, people can use the instanceof operator because we have a sensible prototype chain.

with inheritance that does not hurt

Ok, so mappanel.patch takes Tim's suggestions into account:

• override onRender and onResize instead of registering events
• created a LayerStoreBase object which can be passed as overrides argument to the new GeoExt.extendWithBase function. By doing so, any store object inheriting from Ext.data.Store can be extended to become a LayerStore.
• added a lightweight LayerRecord class, similar to Ext.data.JsonStore

Replying to tschaub:

Basically, we have less control if we have MapPanel (or similar components) register for their own events.

I agree that we get better control (determined exec order) by overloading (onRender) instead of listening to events ("render"). My choice to listen to events instead of overloading was to actually rely on the API of Ext - overloading onRender and friends can potentially cause trouble when going from one Ext version to another. But I'm pretty sure there are many people overloading things like onRender out there, so the Ext developers may want to maintain backward compatibility here. Anyway, I'm fine with overloading onRender and onResize.

Replying to tschaub:

I also agree 100% that we just need to get some code in the trunk. However, I think the design where we mimic Ext is better than creating brand new conventions (a bunch of factory methods). Eric, I think you and I share the inheritance aversion. However, I think we keep things simpler for new users if we copy Ext style. People expect object constructors with capital letters. I know that seems trivial, but it is sort of the only difference.

I fully agree with that. I thing having helper classes like FeatureStore, LayerStore, etc. is great. I'm saying that having these helper classes rely on more generic stuff provides interesting flexibility.

Of course we want to avoid code duplication. It is easy to make a LayerStore and a GroupLayerStore share code. A fine example (in my mind) is the JsonStore. This is basically a convenience constructor for configuring a Sore with a specific Reader. Instead of being named createJsonStore it is named JsonStore.

This is indeed a good example. Ext provides a helper class to create a JSON store, but it also provides everything your to create, for example, a JSON grouping store (GroupingStore + JsonReader). I've just been just advocating that we do the same in GeoExt.

An obvious practical difference that has some benefits is that by extending ext constructor prototypes, we can give things xtypes (not an issue for a store I know) and other people, if they wish, can extend our constructor prototypes and do the same. Finally, if there is a need, people can use the instanceof operator because we have a sensible prototype chain.

with a clean abstract LayerStore class

mappanel.8.patch makes a few improvements:

• no more extendWithBase method (somehow felt not right)
• LayerStore is now a clean abstract class which can extend Ext.data.Store or any subclass of it
• Added docs explaining how to use this new abstract class

With the new LayerRecord, any store can be used to store layers. LayerStore is only needed if synchronization with a map is desired.

I think this is now straightforward enough to use, and still modular enough to allow for "interesting flexibility", as Eric called it.

I'd really like to move on quickly with the code base, so can someone please do a final review and be brave enough to set the state of this ticket to "Commit"? Or at least provide an alternative patch that can be committed? I'm somewhat sick of moving in circles here.

now this looks really good.

mappanel.9.patch is the result of a very good discussion between elemoine and me (see irc).

We found a very clean way to provide mixins that can be used to extend classes to add certain functionality. We are doing this now in data/LayerStore.js. This file provides a mixin (GeoExt.data.LayerStoreMixin) that can be used to extend Ext.data.Store and all of its subclasses. At the bottom, the file provides a default implementation that extends GeoExt.data.Store. And if someone wants to have LayerStore functionality for a GeoExt.data.GroupingStore, such a store could easily be created:

var myGroupingStore = new (Ext.extend(
    GeoExt.data.GroupingStore,
    GeoExt.data.LayerStoreMixin
))(myConfig);

Generally speaking, this provides a very nice way of flexible inheritance. The mixin defines the contract of the extended class, and it can be applied to different classes inheriting from a known superclass:

ExtendingMixin = {

    additionalProperty: null,

    constructor: function() {
        arguments.callee.superclass.constructor.apply(this, arguments);
        // extended constructor
    },

    additionalMethod: function(){}
};

ExtendedClass = Ext.extend(Class, ExtendingMixin);

The LayerRecord class and its usage in LayerStore also got a big improvement, because I had missed the Ext way to create records. elemoine explained it to me and now this really looks good: GeoExt.data.LayerRecord provides the basic contract of a layer record (ensuring that it contains a "layer" and a "title" field). Additional fields can be created on the application level or by LayerReaders that we may want to create in the future.

After a bit of angryness on my end, because this process took so long, I am finally glad that the discussions led to such a neat solution.

(In [238]) Added MapPanel, LayerRecord and LayerStore with examples and tests; also contains license.txt file. Big thanks to sbenthall and tcoulter for their original input, to elemoine for the good discussion today about the LayerStoreMixin and his patience despite my angryness, to tschaub for the constructive comments, and finally to everyone who participated in the discussions on the mailing list about what we want this component to be. r=elemoine,tschaub (closes #4)