-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
BaseLayerPicker improvement & Timeline.prototype.destroy() #798
Changes from 3 commits
ac11dd3
4145a30
587c92c
24cf9ef
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,15 @@ | ||
/*global define*/ | ||
define(['../../Core/DeveloperError', | ||
'../../Scene/SceneMode', | ||
'../../Scene/ImageryLayer', | ||
'../../Scene/ImageryProvider', | ||
'../createCommand', | ||
'../../ThirdParty/knockout' | ||
], function( | ||
DeveloperError, | ||
SceneMode, | ||
ImageryLayer, | ||
ImageryProvider, | ||
createCommand, | ||
knockout) { | ||
"use strict"; | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
imageryLayers.remove(imageryLayers.get(0)); | ||
} | ||
var newLayer = value.creationCommand(); | ||
if (typeof newLayer !== 'undefined') { | ||
imageryLayers.addImageryProvider(newLayer, 0); | ||
var newLayers = value.creationCommand(); | ||
if (typeof newLayers !== 'undefined') { | ||
if (!(newLayers instanceof Array)) { | ||
newLayers = [ newLayers ]; | ||
} | ||
for (var i = 0; i < newLayers.length; ++i) { | ||
if (newLayers[i] instanceof ImageryLayer) { | ||
imageryLayers.add(newLayers[i], i); | ||
} else { | ||
imageryLayers.addImageryProvider(newLayers[i], i); | ||
} | ||
} | ||
} | ||
selectedViewModel(value); | ||
dropDownVisible(false); | ||
|
@@ -104,4 +117,4 @@ define(['../../Core/DeveloperError', | |
}; | ||
|
||
return BaseLayerPickerViewModel; | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,14 +5,16 @@ define([ | |
'../../Core/DeveloperError', | ||
'../../Core/Clock', | ||
'../../Core/ClockRange', | ||
'../../Core/JulianDate' | ||
'../../Core/JulianDate', | ||
'../../Core/destroyObject' | ||
], function ( | ||
TimelineTrack, | ||
TimelineHighlightRange, | ||
DeveloperError, | ||
Clock, | ||
ClockRange, | ||
JulianDate) { | ||
JulianDate, | ||
destroyObject) { | ||
"use strict"; | ||
|
||
var timelineWheelDelta = 1e12; | ||
|
@@ -129,37 +131,33 @@ define([ | |
|
||
this.zoomTo(clock.startTime, clock.stopTime); | ||
|
||
this._timeBarEle.addEventListener('mousedown', function(e) { | ||
widget._handleMouseDown(e); | ||
}, false); | ||
document.addEventListener('mouseup', function(e) { | ||
widget._handleMouseUp(e); | ||
}, false); | ||
document.addEventListener('mousemove', function(e) { | ||
widget._handleMouseMove(e); | ||
}, false); | ||
this._timeBarEle.addEventListener('DOMMouseScroll', function(e) { | ||
widget._handleMouseWheel(e); | ||
}, false); // Mozilla mouse wheel | ||
this._timeBarEle.addEventListener('mousewheel', function(e) { | ||
widget._handleMouseWheel(e); | ||
}, false); | ||
this._timeBarEle.addEventListener('touchstart', function(e) { | ||
widget._handleTouchStart(e); | ||
}, false); | ||
document.addEventListener('touchmove', function(e) { | ||
widget._handleTouchMove(e); | ||
}, false); | ||
document.addEventListener('touchend', function(e) { | ||
widget._handleTouchEnd(e); | ||
}, false); | ||
this._onMouseDown = function(e) { widget._handleMouseDown(e); }; | ||
this._timeBarEle.addEventListener('mousedown', this._onMouseDown, false); | ||
|
||
this._onMouseMove = function(e) { widget._handleMouseMove(e); }; | ||
this._timeBarEle.addEventListener('mousemove', this._onMouseMove, false); | ||
|
||
this._onDOMMouseScroll = function(e) { widget._handleMouseWheel(e); }; | ||
this._timeBarEle.addEventListener('DOMMouseScroll', this._onDOMMouseScroll, false); | ||
|
||
this._onMouseWheel = function(e) { widget._handleMouseWheel(e); }; | ||
this._timeBarEle.addEventListener('mousewheel', this._onMouseWheel, false); | ||
|
||
this._onTouchStart = function(e) { widget._handleTouchStart(e); }; | ||
this._timeBarEle.addEventListener('touchstart', this._onTouchStart, false); | ||
|
||
this._onTouchMove = function(e) { widget._handleTouchMove(e); }; | ||
this._timeBarEle.addEventListener('touchmove', this._onTouchMove, false); | ||
|
||
this._onTouchEnd = function(e) { widget._handleTouchEnd(e); }; | ||
this._timeBarEle.addEventListener('touchend', this._onTouchEnd, false); | ||
|
||
this.container.oncontextmenu = function() { | ||
return false; | ||
}; | ||
|
||
window.addEventListener('resize', function() { | ||
widget.handleResize(); | ||
}, false); | ||
this._onResize = function() { widget.handleResize(); }; | ||
window.addEventListener('resize', this._onResize); | ||
|
||
this.addEventListener = function(type, listener, useCapture) { | ||
widget.container.addEventListener(type, listener, useCapture); | ||
|
@@ -673,5 +671,40 @@ define([ | |
this._makeTics(); | ||
}; | ||
|
||
/** | ||
* Return if the widget has been destroyed. | ||
* @memberof Timeline | ||
*/ | ||
Timeline.prototype.isDestroyed() { | ||
return false; | ||
} | ||
|
||
/** | ||
* Destroys the widget. Should be called if permanently | ||
* removing the widget from layout. | ||
* @memberof Timeline | ||
*/ | ||
Timeline.prototype.destroy = function() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For completeness, can you add an |
||
var container = this.container; | ||
|
||
this._timeBarEle.removeEventListener('mousedown', this._onMouseDown, false); | ||
this._timeBarEle.removeEventListener('mousemove', this._onMouseMove, false); | ||
this._timeBarEle.removeEventListener('DOMMouseScroll', this._onDOMMouseScroll, false); | ||
this._timeBarEle.removeEventListener('mousewheel', this._onMouseWheel, false); | ||
this._timeBarEle.removeEventListener('touchstart', this._onTouchStart, false); | ||
this._timeBarEle.removeEventListener('touchmove', this._onTouchMove, false); | ||
this._timeBarEle.removeEventListener('touchend', this._onTouchEnd, false); | ||
window.removeEventListener('resize', this._onResize); | ||
|
||
this._clock.onTick.removeEventListener(this.updateFromClock, this); | ||
|
||
while (container.firstChild) { | ||
container.removeChild(container.firstChild); | ||
} | ||
container.className = container.className.replace('cesium-timeline-main', ''); | ||
|
||
return destroyObject(this); | ||
}; | ||
|
||
return Timeline; | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no need to ever return an ImageryLayer instead of an ImageryProvider. It looks like you are doing it because you want to set an alpha value, but you can do that now with the ImageryProvider by setting the
defaultAlpha
property. I just checked and this isn't properly documented, so that's a separate issue which I will write up.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see - indeed, I needed to be able to set the alpha value