BaseLayerPicker improvement & Timeline.prototype.destroy() #798
Conversation
and to accept ImageLayers as well as providers
|
Thanks for separating out this pull request @akosmaroy. I'm the wrong person to review this, but I'm sure @mramato will be happy to. |
|
Sorry I never replied to the original thread about this, I've been unable to work on Cesium for the past few days. Now that I'm back, I'm reviewing this change now. |
| document.addEventListener('touchend', function(e) { | ||
| widget._handleTouchEnd(e); | ||
| }, false); | ||
| this.onmousedown = function(e) { widget._handleMouseDown(e); }; |
There was a problem hiding this comment.
Can you please make these new functions start with underscore and match our capitalization rules? Instead of this.onmousedown, use this._onMouseDown. We use underscore as a convention for pseudo-private variables.
|
Other than my two minor comments, the Timeline changes look good. The Timeline itself certainly needs an overhaul, but I appreciate the cleanup you've done in the meantime. |
|
thanks for the feedback, see the above commit that hopefully contains the suggested changes |
| @@ -90,12 +94,21 @@ define(['../../Core/DeveloperError', | |||
| return selectedViewModel(); | |||
| }, | |||
| write : function(value) { | |||
| if (imageryLayers.getLength() > 0) { | |||
| while (imageryLayers.getLength() > 0) { | |||
There was a problem hiding this comment.
You changed the code to simply remove all layers when a layer is switched. This is definitely not what we want. Someone may use the widget to allow base layer swapping, but want to manage additional layers on top of that themselves. With this change, that's no longer possible. The work-around would be to have each viewModel always return the same set of imagery providers, this way on remove, we only remove the ones that we originally added. Since we'll use instance comparison, this should work even if the added provider was manually moved around by the user.
There was a problem hiding this comment.
fair enough. what I wanted to achieve is to be able to pass multiple layers - what would be your suggestion to be able to achieve this?
the use case is that if someone wants to add a 'set of layers' which overlap each other in semi-transparent ways, he should be able to do that.
|
For the BaseLayerPicker changes, I'll ask @emackey, @shunter, and @kring to also chime in with their thoughts. This widget was originally designed to only operate on the base layer and as an interim step to "real" layer management in our soon-to-be-developed Table of Contents widget. I was originally hesitant with this change, but after thinking about it some more I think we can make it work without any issues and it might be useful if there's a common use case for having "imagery sets" get added instead of just the one provider. We also need to make sure any documentation is updated to reflect the new behavior, primarily the What do you guys think? |
|
I think extending a "base layer" to allow multiple makes sense, since as you point out the BaseLayerViewModel can keep track of the layer(s) that it added in order to remove just those independently of any other layers added programmatically. |
|
@shunter I tried to implement such an approach, but the issue with this is that the picker replaces the base layer which is added when creating the viewer (i.e. a default base layer) - in effect a base layer that is not added by the picker. as this layer is not added by the picker, it doesn't know that it has to remove it first. |
|
What's the plan with this pull request? We have this change in the batching branch and don't want to bring it in early. |
|
There's been tons of changes in the widget code recently, so this branch needs to merge in master and fix the definite conflicts that have arisen. For the question @akosmaroy, I would have the BaseLayerPicker not remove any providers except the ones it created. We could then change the Viewer widget code to make sure that it removes all layers before it creates the BaseLayerPicker. @akosmaroy if you don't have time to work on this, I'd be happy to merge this code into my own branch and finish it up so that it can come into master ahead of the batching branch. It's up to you. |
|
@mramato thank you, I won't have time to work on this in the next two weeks. I'm sorry about that |
|
Timeline cleanup is done in #874. Working on BaseLayerPicker changes now. |
please consider these changes, which I originally posted from the batching branch, also see #789 and #790