Calling `setVisibleLayers()` on a dynamic layer does not correctly update layer control widget

[duckblaster]

How often can you reproduce it?

• [x] Always
• [ ] Sometimes
• [ ] Rarely
• [ ] Unable
• [ ] I didn’t try

Description:

Calling setVisibleLayers() on a dynamic layer does not correctly update the layer control widget.

Steps to reproduce:

1. Go to http://demo.cmv.io/viewer/
2. Expand all of the Louisvile Public Safety layers and sublayers
3. Untick all sublayers and folders under Louisvile Public Safety
4. Run this line of javascript: app.layers[1].setVisibleLayers([]); (that assumes that app.layers[1] is Louisvile Public Safety, the first layer didn't load - CORS blocked it I think)

Expected results:

All sublayers and folder under Louisvile Public Safety should be unticked and not visible.

Actual results:

All sublayers under Louisvile Public Safety are unticked and hidden, all folders are ticked. It should also be possible to set some sublayers to be ticked in the layer control widget, but have their parent folder unticked (like how Schools under Public Safety is default unticked)

Environment:

Software	Version
CMV Version	v2.0.0-beta.1
Browser	Chrome 56.0.2924.87
Operating system	Windows 10

[green3g]

I think the issue here is you can't pass an empty array to setVisibleLayers it needs to be a -1 instead:

layer.setVisibleLayers([-1]);

Docs mention this here: https://developers.arcgis.com/javascript/3/jsapi/arcgisdynamicmapservicelayer-amd.html#setvisiblelayers

It should also be possible to set some sublayers to be ticked in the layer control widget, but have their parent folder unticked (like how Schools under Public Safety is default unticked)

I agree, it would be ideal if this behavior were possible. But the way that the esri api's dynamic layer works makes this a little bit complicated. Initially, the rest services default visibility is used to display the checkboxes on the layer. This initial array allows group layers in the array. If a group layer is in the array, and a child sublayer is in the array, that sublayer will be visible:

visibleLayers: [0, 1]

0: Parent Group
---- 1: Child 1 --> Visible 
---- 2: Child 2 ---> Not visible

Calling setVisibleLayers with an array overrides this behavior and replaces the array with a new array that have a different set of rules. If a group layer ID is passed, all child layers will be visible. (this is wrong imo but its how the api works). If a Child ID is passed, it is visible, whether or not its parent is passed.

visibleLayers: [0, 1, 4]

0: Parent group
---- 1 : Child 1 --> Visible
---- 2: Child 2 --> Visible inherited because group layer was passed
3: Other Group
----4: Child 3 --> Visible
----5: Child 4 --> Not visible

[duckblaster]

No, it's still broken, try 1,2,3 in the array instead.

On Wed, 22 Mar 2017 at 2:22 AM, Gregg Roemhildt notifications@github.com wrote:

I think the issue here is you can't pass an empty array to setVisibleLayers it needs to be a -1 instead:

layer.setVisibleLayers([-1]);

Docs mention this here: https://developers.arcgis.com/javascript/3/jsapi/arcgisdynamicmapservicelayer-amd.html#setvisiblelayers

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/cmv/cmv-app/issues/700#issuecomment-288076068, or mute the thread https://github.com/notifications/unsubscribe-auth/AAibY2XO17brpJgW_-8fqRfJ-9zkN1Wqks5rn88LgaJpZM4MjGgx .

[green3g]

1,2,3 appears to work for me.

[duckblaster]

Note that "Emergency Operations" and "Event Information" are ticked. Also, there is no way to have "Event Grids" and "Road Blocks" without that "Emergency Operations" and "Event Information" so it doesn't behave the same as the setting in viewer.js On Wed, 22 Mar 2017 at 6:53 AM, Gregg Roemhildt notifications@github.com wrote:

1,2,3 appears to work for me. [image: image] https://cloud.githubusercontent.com/assets/5471079/24162414/6f7caccc-0e35-11e7-8b89-55fd0ef6c949.png

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/cmv/cmv-app/issues/700#issuecomment-288164053, or mute the thread https://github.com/notifications/unsubscribe-auth/AAibYxCGmmVT4cRppdXUe818rxMRj5nfks5roA61gaJpZM4MjGgx .

[green3g]

Note that "Emergency Operations" and "Event Information" are ticked.

Yes, I think that's the behavior that was chosen because the alternative was to uncheck the group layers when setVisibleLayers is called, which might be an unexpected behavior. Especially if you have several levels of group layer nesting.

Is that the issue your pull request is targeting?

[duckblaster]

Yes, as far as I can tell from my testing On Wed, 22 Mar 2017 at 7:12 AM, Gregg Roemhildt notifications@github.com wrote:

Note that "Emergency Operations" and "Event Information" are ticked.

Yes, I think that's the behavior that was chosen because the alternative was to uncheck the group layers when setVisibleLayers is called, which might be an unexpected behavior. Especially if you have several levels of group layer nesting.

Is that the issue your pull request is targeting?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/cmv/cmv-app/issues/700#issuecomment-288170126, or mute the thread https://github.com/notifications/unsubscribe-auth/AAibYzVMX9ScWjDf5LRpcgrkLfg4LeQ8ks5roBMigaJpZM4MjGgx .

[green3g]

Okay, I think I understand what's going on now in your pull request, but correct me if I"m wrong:

1. layer.setVisibleLaysers([1,2,3]) is called
2. layer control is overriding the default setVisibleLayers with its own which does the following: a. Removes sub layers from the array if the parent group id is not in the array. Layer array becomes [] b. Sets the array to [-1] if no layers are in the array after removal. Layer array becomes [-1] c. calls the original setVisibleLayers with the modified array

I like the fact that it allows more flexibility in the layer control. I think there's a problem though. Essentially, its changing the behavior of the api. It is (imo) for the better but regardless changing the api will have unintended consequences.

For instance, any other widget that calls setVisibleLayers will have to pass the group layers in order for the visibility to work, where previously only the child id's were needed. And if the layer control isn't included, then these widgets would not work the same.

[duckblaster]

Yes, It would probably be better somewhere else, but I don't know where. On Wed, 22 Mar 2017 at 8:14 AM, Gregg Roemhildt notifications@github.com wrote:

Okay, I think I understand what's going on now in your pull request, but correct me if I"m wrong:

1. layer.setVisibleLaysers([1,2,3]) is called
2. layer control is overriding the default setVisibleLayers with its own which does the following: a. Removes sub layers from the array if the parent group id is not in the array. Layer array becomes [] b. Sets the array to [-1] if no layers are in the array after removal. Layer array becomes [-1] c. calls the original setVisibleLayers with the modified array

I like the fact that it allows more flexibility in the layer control. I think there's a problem with step b though. Essentially, its changing the behavior of the api. It is (imo) for the better but regardless changing the api will have unintended consequences.

For instance, any other widget that calls setVisibleLayers will have to pass the group layers in order for the visibility to work, where previously only the child id's were needed. And if the layer control isn't included, then these widgets would not work the same.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/cmv/cmv-app/issues/700#issuecomment-288188293, or mute the thread https://github.com/notifications/unsubscribe-auth/AAibYxR4xnXTs2Kf8j9HpCiQWCqdxozrks5roCGqgaJpZM4MjGgx .

[green3g]

Instead, I think the layer control needs to maintain its own visibleLayer array and not modify the original one. We can't change the api even if its written in a way that doesn't work well.

[duckblaster]

Perhaps use the layerControl/setVisibleLayers topic or something? On Wed, 22 Mar 2017 at 8:18 AM, Gregg Roemhildt notifications@github.com wrote:

Instead, I think the layer control needs to maintain its own visibleLayer array and not modify the original one. We can't change the api even if its written in a way that doesn't work well.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/cmv/cmv-app/issues/700#issuecomment-288189375, or mute the thread https://github.com/notifications/unsubscribe-auth/AAibYx4S7UzdwLlG0NtTV62RkV_UAcMQks5roCKegaJpZM4MjGgx .

[tmcgee]

we definitely do not want to modify/override the Esri API. I want to get a handle on exactly what the issue is before commenting further. Might be related to other issues regarding layer visibility noted in the past.

[duckblaster]

Some previous issues: #217 #367 #309 #601 #633

[tmcgee]

thinking out loud:

using aspect.after and aspect.around isn't "changing the api" as I interpreted it when reading earlier. So I could be supportive of this approach. Still need to give it more attention than I can at the moment.

We have to keep in mind that other widgets like identify use the visiblelayers array so maintaining our own array in a single widget like layerControl may not be a good path forward. There is at least one other change applied some time last year (passing subLayerInfos array to layerControl widget) that can break the identify widgets ability to accurately determine which sub layers are visible.

Bottom line: more discussion and testing required...

[green3g]

Yep, need more discussion on this. Take this example:

First there are two types of layers in a Dynamic layer. Lets call them

• Child layers - Fire Stations is a child layer
• Group Layer - Public Safety is a group layer (these can be a child layer also if it has a parent)

The API handles each of these differently when setVisibleLayers is called.

Group Layers: If a group layer is passed, it sets all of the child layers of that group to be visible.

app.layers[1].setVisibleLayers([0]) visibleLayers becomes [0] will set All layers under Public Safety to visible. 🙁 this is not reflected correctly in the layer control

Child Layers: If a child layer id is passed, that child layer becomes visible, regardless of whether the parent group layer id is passed. 😀 this is reflected correctly in the layer control

What if both are passed? app.layers[1].setVisibleLayers([0,1,2,3]) visibleLayers becomes [0,1,2,3] Interestingly, the api handles this the same as if only 0 was passed. 🙁 layer control doesn't handle this correctly

The pull request modifies this behavior by modifying the visible layers array before set visible layers is called. If a layer's parent group layer id is not included in the layers passed, it will be removed from the array and it will not become visible.

[tmcgee]

Yep.

Whatever we do needs to also correctly handle other scenarios such as:

1. a group layer id is included in the layer's imageParameters like we have in the demo:

   imageParameters: buildImageParameters({
   layerIds: [0, 2, 4, 5, 8, 10, 12, 21],
   layerOption: 'show'
   })

2. a group layer id is included in the layerIds array included in layerControlLayerInfos:

layerControlLayerInfos: {
    layerIds: [0, 2, 4, 5, 8, 9, 10, 12, 21]
}

3. a group layer id is included in the subLayerInfos array included in layerControlLayerInfos:

layerControlLayerInfos: {
    subLayerInfos: [
        {
            id: 0
        }, {
            id: 2
        }, {
            id: 4
        }, {
            id: 5
        }, {
            id: 8
        }, {
            id: 9
        }, {
            id: 10
        }, {
            id: 12
        }, {
            id: 21
        }
    ]
}

Each of these examples are supported configurations to set the initial layer visibility yet the layerControl does not handle any of them correctly with respect to group laer. I have not looked at whether the pull request fixes these. Supporting what is in the configuration is just as important if not more important than supporting programmatic control of layer visibility through setVisibleLayers.

I think we also need to consider a tri-state checkbox for the group layer in the layerControl to more accurately reflect the state of the map. That is a topic for another day...

[tmcgee]

Continued thought. # 2 above:

layerControlLayerInfos: {
    layerIds: [0, 2, 4, 5, 8, 9, 10, 12, 21]
}

may provide the correct results. More investigation is needed to be sure...

I prefer # 3 as it provides the most control - you can determine which sub layers and groups are included in the layerControl AND you can set the defaultVisibility to true or false to control the default visibility of each sublayer/group:

layerControlLayerInfos: {
    subLayerInfos: [
        {
            id: 0,
            defaultVisibility: false
        }, {
            id: 2
        }, {
            id: 4
        }, {
            id: 5
        }, {
            id: 8
        }, {
            id: 9
        }, {
            id: 10
        }, {
            id: 12
        }, {
            id: 21
        }
    ]
}

[green3g]

Related: https://github.com/cmv/cmv-app/issues/605

[tmcgee]

I am completing a redesign of how group layers within a Dynamic layer are handled within the layerControl. The behavior will be similar to the newly introduced "Group Layer". More intuitive and consistent - at least from my perspectice.

The changes will address the issue from this PR in a different way. I won't close this PR yet.

[green3g]

Love the new folder icons!

Will this change the way the default visibility is set up @tmcgee? Meaning if I set "Louisville Public Safety" Group to be off by default, and I turn it on using the checkbox, will that set all child layers to a visible state?

[tmcgee]

No, currently toggling the layer visibility would not change the visibility of the sub layers. This is intentional as I think that would be undesirable for many (most?) implementations. There is an existing menu option to set all sub layers visible/invisible that handles this. Definitely open to adjustments of my current view.

[green3g]

Perfect. That was my only concern :+1:

Edit: what if I set "Public Safety" off by default, and then check the box for public safety to visible? Does that set all the sublayers to visible or just the ones that are visible by default?

[tmcgee]

Regarding your edit: The behavior is consistent with what is implemented for the Grouped layer currently and how most 3-state check boxes work on the web (so hopefully familiar to users). If any child sub layers in the layerControl are not visible, clicking the checkbox would make all sub layers (AND child folders) in the layerControl visible. If all of the sublayers are visible, they would all be toggled to not visible.

I think I have handled all the various scenarios using imageParameters for the layer and layerIIDs/subLayerInfos arrays in the layerConftrolLayerLnfos. If I haven't addressed them all, my sense is it will be easier to manage those in the newly re-designed. code because Esri's funky approach to group layer visibility is pretty much ignored.

[green3g]

That makes sense. I think the new behavior will implement some changes that layer publishers will need to address but I do believe it is for the better and will hopefully be a welcome addition by the community.

[tmcgee]

I am hoping to avoid that as much as possible but perhaps may not succeed in all cases. What cannot or should not be changed by republishing can hopefully be managed within configuration.