Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Dec 18, 2015

Conversation

annarieger
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor Author

Thx for review @marcjansen!

Your suggested solution is commited now.

@marcjansen
Copy link
Member

Very nice.

marcjansen added a commit that referenced this pull request Dec 18, 2015
Add additional check whether the tree node has a 'layer' property
@marcjansen marcjansen merged commit 10ded2c into geoext:master Dec 18, 2015
@marcjansen
Copy link
Member

Thanks, Anna!

@marcjansen
Copy link
Member

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

@chrismayer
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants