Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#113 closed enhancement (fixed)

LayerContainer: make childNodeType configurable

Reported by: bartvde Owned by:
Priority: minor Milestone: 0.6
Component: GeoExt.tree.LayerContainer Version: trunk
Keywords: Cc:
State: Commit

Description

Currently LayerContainer is tightly coupled to GeoExt.tree.LayerNode, it would be nice if this would be configurable. I'll attach a patch.

Attachments (2)

geoext_113.patch (1.8 KB) - added by bartvde 8 years ago.
geoext-113.patch (844 bytes) - added by ahocevar 8 years ago.
use the commonly used defaults property to pass a nodeType

Download all attachments as: .zip

Change History (14)

Changed 8 years ago by bartvde

comment:1 Changed 8 years ago by bartvde

  • State changed from None to Review

Changed 8 years ago by ahocevar

use the commonly used defaults property to pass a nodeType

comment:2 Changed 8 years ago by ahocevar

  • State changed from Review to Needs Discussion

Bart: good idea, but I think we should use the commonly used defaults property (that we already have) to pass defaults for the children.

So if you have a nodeType "barts_custom_type", you would create a layer container with that node type by saying

var container = new GeoExt.tree.LayerContainer({
    defaults: {nodeType: "barts_custom_type"}
});

My patch adds this functionality.

I would, however, not necessarily change the nodeType, but just the UI, setting the uiProvider in the defaults.

comment:3 Changed 8 years ago by bartvde

Hi Andreas, thanks for your patch, that makes sense.

Wrt only changing the UI, the current layer node always adds a checkbox, and I do not want a checkbox, so that's why I thought I also need a new layer node.

Should we not change GeoExt.tree.LayerContainer to also use defaults for the childNodeType then? I copied that code from there.

comment:4 Changed 8 years ago by ahocevar

I think the LayerNode should not change defaults on the UI as it does currently (e.g. force it to render a checkbox). The UI should do that. Maybe you want to create a patch for that as well?

What do you mean by the other question?

Should we not change GeoExt.tree.LayerContainer to also use defaults for the childNodeType then?

comment:5 Changed 8 years ago by bartvde

I've opened up #114 (LayerNode should not change defaults on the UI) for the UI checkbox issue.

The other question relates to having api: config[childNodeType] in LayerNode, should this not be done using the defaults property as well (similar to your latest patch here)?

comment:6 Changed 8 years ago by ahocevar

Bart: my latest patch was meant as a replacement for your patch, not an addition. So no, there should be no childNodeType config property, because the defaults will get applied to the child nodes, and if defaults.nodeType is provided, that nodeType will be used for child nodes. "defaults" is for children, and nodeType is a standard config property for nodes. So I do not see a need for childNodeType. Does that answer your question?

comment:7 Changed 8 years ago by bartvde

Hi Andreas, I understand the working of your patch. What I am trying to say is that the LayerNode class has a similar piece of code, which also could be done using the defaults property of LayerNode.

comment:8 Changed 8 years ago by ahocevar

Bart, now I understand! Sorry for that taking so long. This childNodeType in the LayerNode is something that should probably be refactored to be called plugins, and work like plugins elsewhere in Ext. Such a plugin could e.g. add a GetLegendGraphic image as child node, or add sublayers, or add other functionality to the node (what the current childNodeType approach cannot). I have been planning to revisit this for a long time, but had no time, and I did not need it for an application urgently.

comment:9 Changed 8 years ago by bartvde

  • State changed from Needs Discussion to Commit

Hi Andreas, right, we'll tackle that some other time. I think your patch is ready for commit. At least it makes things a bit more flexible, even if I'll end up only using a custom UI class.

comment:10 Changed 8 years ago by ahocevar

For the childNodeType / plugins issue, I created #115.

comment:11 Changed 8 years ago by ahocevar

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

(In [1236]) make the nodeType of subnodes configurable by setting it in defaults. r=bartvde (closes #113)

comment:12 Changed 8 years ago by ahocevar

(In [1241]) check for this.defaults. Should have gone in with r1236 (see #113)

Note: See TracTickets for help on using tickets.