New group folders for dynamic map services

[tmcgee]

I'm not sure this addresses all issues or scenarios.
Submitting for review to identify what has not been addressed.

[green3g]

It looks like this is a bug, if layerIds is not provided, it breaks on 176.

[tmcgee]

updated line 161 to:

var layerIds = this.control.controlOptions.layerIds || [];

[green3g]

layerIds.length throws an error when layerIds is not defined

[green3g]

I think this is a good addition. I might've changed the folder icons to something a little bit darker, its a little hard to see on some screens I think because it is so light, white on white. Just my two cents.

That being said, I don't think I can personally utilize this in my app at the moment because of the way my groups are set up. Essentially I start out with something like this, where each group layer is off, but several children are turned on in each group. No layers are displayed until the user turns on one or several groups. The users like to be able to turn on the group and get all of their related layers turned on.

But I've also included some extra layers in the folder that don't get used regularly but are available for people who need them. Toggling the group now, turns on those extra layers.

That's not a huge issue, I can move them to a different group or something else, there are a few ways to handle it, I just wanted it to be mentioned for future users to find.

Which leads me to my second issue:
I'm not sure why at the moment but when I turn on the folder, like Documentation, it turns on all the other group layers, which renders about 30 layers. I wouldn't expect this to happen and it looks to me like a bug. I'll need to look into it a bit more. I can pm you with a link to the service to test on.

[tmcgee]

@roemhildtg

I don't think I can personally utilize this in my app at the moment because of the way my groups are set up...

Do you think we should attempt to support both approaches to group layers? That adds to the complexity but certainly is possible.

Which leads me to my second issue:
I'm not sure why at the moment but when I turn on the folder, like Documentation, it turns on all the other group layers, which renders about 30 layers. I wouldn't expect this to happen and it looks to me like a bug. I'll need to look into it a bit more. I can pm you with a link to the service to test on.

Did you find time to look into this?

[tmcgee]

@roemhildtg Using the test Map Service you provided to me (thanks), I have corrected how sub layer visibility is determined from their check boxes. Please give it another test and let me know how it works for you.

[green3g]

Do you think we should attempt to support both approaches to group layers? That adds to the complexity but certainly is possible.

I'm not a big fan of this, the dynamic layers have already become pretty complex and having two different cases makes it more difficult. I think the better solution would be to configure our layer setup to benefit from the new style. That being said though, I know there are others who do this same style of set up where they might have extra layers in a group that are invisible by default and might not have control over how the default visibility is published.

Please give it another test and let me know how it works for you.

It still looks like its having issues. Try this and let me know if you see the same thing:

Turn on the "Sanitary System" group layer check box. This will check everything under sanitary system. (which is expected). But what also happens is all of the other sub layers in other groups become visible on the map also.

I'll starting digging in deeper later today to figure out why this is.

[green3g]

Ok, so here's the question. Take a layer that has a parent layer invisible by default. Do we want the sub layer visibility to be not checked by default if its parent group is off by default? (easier)

Or do we want it to be checked, but excluded from the visible layers array when another layer's visibility changes? (harder)

[tmcgee]

Maybe there's an issue with the group code since I am not as familiar with the data. But I think that part is working and there is a different issue that still needs to be resolved - one that has been around for a while when groups are included in ImageParameters for a layer.

Here's what I think I see: The map is being presented initially based on the group layer visibility set in your service. The layerControl and the map are out of sync at this point (same as with ImageParameters problem). Once you change the visibility of any layer in layerControl (or from elsewhere?), the correct layers are made visible and represented accurately in the layerControl (same as with ImageParameters problem). Please confirm that is what you see as well. Then we can discuss a solution.

[green3g]

The map is being presented initially based on the group layer visibility set in your service. The layerControl and the map are out of sync at this point (same as with ImageParameters problem).

Yes, that's what I'm seeing

[tmcgee]

Alternative to your 2 options: Refresh the dynamic layer once it is loaded in layerControl. This "fixes" the visibility based on what is displayed in layerControl. Obvious downside is a new request to the server.

None of these options fix the ImageParameters "problem" if the layerControl widget is not included in the app. But is that a separate problem that needs code to "fix" or is it just a configuration challenge?

[tmcgee]

FWIW: I do think this approach (tri-state check boxes for group layers) is a solution worth pursuing as it is more familiar to non-GIS users. The UI is quite common in other applications unlike the ArcGIS approach which is not at all common. Hopefully you agree and hopefully we can minimize any pain that might be inflicted along the way. 😄

[green3g]

This "fixes" the visibility based on what is displayed in layerControl

This has been mentioned before, "fixing" the api's poor implementation.

I think it would have negative side effects though, like if other widgets are using the event handlers for visible layers they would get different results if layer control is included or excluded.

grrr, why did they make this so difficult?

I do think this approach (tri-state check boxes for group layers) is a solution worth pursuing

I do too and agree with you! Maybe my app is just too big... How would you suggest improving my situation with the new controls? Maybe this is a side topic, and a pm would be more appropriate ?

[tmcgee]

P.S. I am not liking the css I choose for the check boxes. They look oversized and the css does not work cleanly with other themes (like using calcite instead of flat). But that's a conversation for another day and another repo (cmv-themes?)

[tmcgee]

@roemhildtg This is due for another review and discussion. I merged in the functionality from @duckblaster's branch many months ago before falling into abyss of the fire and parental care.

[green3g]

Yeah, it appears to be good to go with several tests I was able to find online. I haven't revisited this in some time with the cities' layers but I remember the edge case had issues before where if I have a group layer turned off with sublayers turned on. I'll revisit and follow up.

[green3g]

So...yes. That definitely still happens. I know you don't have a great way to test it though...every example I could find basically has everything visible by default.

I also don't know how common of a practice it is with cmv's users to group sublayers in a "not checked" parent layer.

[tr3vorm]

We often use the scenario where sublayers are enabled, with parent layer off by default. It just means that when a user does enable the parent, the pertinent underlying layers are shown. Just adding my +1 for this desired behaviour.

[tmcgee]

@roemhildtg @tr3vorm

It has been awhile since I have thought about this too. If I recall correctly, I addressed this by adding the `ignoreDynamicGroupVisibility as a toggle. The intent of

ignoreDynamicGroupVisibility: false

is to restore the original Esri group layer behavior as you describe. This was merged from @duckblaster's branch

Let me know if this works the way you would expect.

[green3g]

Oh nice! Yeah that does work 👍

[tmcgee]

@roemhildtg 👍

Anything remaining to do before this is pushed out within a 2.0 beta 3 release?

[duckblaster]

I noticed that you removedsetAllVisibleLayers in in this commit, I believe this was something I added to make @roemhildtg's AppSettings widget save sublayer visibility correctly, by pointing it at layer.allVisibleLayers instead of layer.visibleLayers

[tmcgee]

@duckblaster Is that something you think should be added to CMV core for other purposes or is it specific to that widget only?

[duckblaster]

It would be helpful to have the layer visibility as sent to esri, and as set by the user, and load them as appropriate

[duckblaster]

It would need to be in core CMV, I don't know how you would get the info otherwise

[tmcgee]

ok, though I suggest perhaps we consider it be done differently (and in a different PR?). I'm not a fan of functionality in one widget (AppSettings) dependent on functionality created by another widget (LayerControl).

[duckblaster]

The other part of the code used for this is

            topic.publish('layerControl/setAllVisibleLayers', {
                id: layer.id,
                allVisibleLayers: allVisibleLayers
            });

The AppSettings widget probably should be talking directly to the LayerControl widget, rather than going via the layer, but I needed to just get something that worked (and I wan't that clear on exactly how everything hooked together)

[duckblaster]

My changes to the AppSettings widget (for reference)

[tmcgee]

@roemhildtg what are your thoughts on the connection between AppSettings and LayerControl? My feeling is whatever changes may come from that specific discussion, it should be handled in a separate PR as it is not directly related to the intent of this PR. thoughts?

[green3g]

Sorry, forgot to get back to you on this...

it should be handled in a separate PR as it is not directly related to the intent of this PR. thoughts?

Yes, I agree. I'm not a super big fan of my app setting implementation anyways, there's still some race conditions in there somewhere where the settings aren't loaded correctly at times.

[tmcgee]

@roemhildtg I had good results moving Brian Bunker's MapNavigationHash widget to a mixin to address race conditions with that widget. Perhaps that approach would work for your Settings widget too.

[tmcgee]

@roemhildtg @duckblaster @tr3vorm Any outstanding items/concerns in this PR? Would love to get it committed.

[duckblaster]

Looks good to me

[green3g]

Yup, lets merge this.

[tmcgee]

@roemhildtg @duckblaster thanks!

@roemhildtg please merge. 😄