geoext / geoext2

GeoExt 2 — JavaScript Toolkit for Rich Web Mapping Applications
http://geoext.github.io/geoext2/
Other
142 stars 106 forks source link

Add additional check whether the tree node has a 'layer' property #373

Closed annarieger closed 8 years ago

annarieger commented 8 years ago

Hi devs,

I would suggest to add an additional check in class GeoExt.tree.Util.js to ensure if a tree node really has a property 'layer' before its visibility is updated.

This mini check could be useful for applications, that use complex layer trees with nested folder structures (e.g. layer folder has checkboxes to check/uncheck all layers inside). In this case the folder itself has obviously no corresponding layer.

See also PR #355, especially df4ec85 - it handles the same issue, just solved in a bit other way ;)

marcjansen commented 8 years ago

Hey @annarieger I agree we need the check. Can you possibly change you code a little, so that we can reuse the layer variable?

E.g.

        // …
        updateLayerVisibilityByNode: function(node, checked) {
            var layer = node.get('layer');
            if(layer && checked != layer.getVisibility()) {
                node._visibilityChanging = true;
                if(checked && layer.isBaseLayer && layer.map) {
                    layer.map.setBaseLayer(layer);
                } else if(!checked && layer.isBaseLayer && layer.map &&
                          layer.map.baseLayer && layer.id == layer.map.baseLayer.id) {
                    // Must prevent the unchecking of radio buttons
                    node.set('checked', layer.getVisibility());
                } else {
                    layer.setVisibility(checked);
                }
                delete node._visibilityChanging;
            }
            GeoExt.tree.Util.enforceOneLayerVisible(node);
        }
        // …

Otherwise this looks great to me.

annarieger commented 8 years ago

Thx for review @marcjansen!

Your suggested solution is commited now.

marcjansen commented 8 years ago

Very nice.

marcjansen commented 8 years ago

Thanks, Anna!

marcjansen commented 8 years ago

Should we backport this to the 2.0-branch? /cc @chrismayer?

chrismayer commented 8 years ago

As far as I remember the GeoExt.tree.Util.js class was introduced in the 2.1.x-series. But if there is equivalent code in the 2.0.x-branch I am +1 for a backport.