Widget tweaks #870

mramato · 2013-06-15T02:09:32Z

Fixes Firefox Sandcastle resize issue on startup... #608 by forcing a resize every 60 frames.
Add a useDefaultRenderLoop property and constructor option to CesiumWidget and Viewer so that users can turn on/off the default render loop at will.
Have Viewer use the new property to disable the CesiumWidget render loop and use it's own instead.

1. Fixes #608 by adding a render counter and always resizing the window every 60 frames. 2. Add a `useDefaultRenderLoop` property and constructor option to `CesiumWidget` so that users can turn on/off the default render loop at will.

shunter · 2013-06-17T15:45:07Z

Source/Widgets/Viewer/Viewer.js

@@ -502,7 +503,8 @@ define([
    };

    Viewer.prototype._onTick = function(clock) {
-        if (this._needResize) {
+        if (this._needResize || this._resizeCounter === 60) {


The code that checks _resizeCounter is in CesiumWidget, but this code is in Viewer. Shouldn't all these changes be in CesiumWidget?

1. Use frameNumber instead of a separate counter for determining when we should resize. 2. Have Viewer disable the default render loop and use it's own. 3. Expose `useDefaultRenderLoop` on Viewer as well.

mramato · 2013-06-17T17:21:23Z

@shunter ready for another look.

shunter · 2013-06-17T17:30:45Z

Source/Widgets/Viewer/Viewer.js

+     * Resizes the widget to match the container size.
+     * This function is called automatically as needed unless
+     * <code>useDefaultRenderLoop</code> is set to false.
+     * @memberof CesiumWidget


Wrong memberof here.

This was in two places in fact, both fixed.

shunter · 2013-06-17T17:32:33Z

Source/Widgets/Viewer/Viewer.js

+            return;
+        }
+
+        this._lastWidth = container.clientWidth;


_lastWidth and _lastHeight should be declared in the constructor like we normally do.

Thanks, fixed.

Also seems slightly more idiomatic to assign to the local var width / height first, then assign those to this_lastWidth / this._lastHeight rather than vice versa.

I agree, I added these after the fact and it didn't occur to me to switch the order. Fixed.

shunter · 2013-06-17T17:47:32Z

Source/Widgets/Viewer/Viewer.js

+
+            if (width > 900) {
+                animationWidth = 169;
+                animationContainer.style.width = '169px';


I checked a JSPerf and there's a significant cost to setting style, even if you're setting it to the same value it already was. Since we call resize all the time now, we should cache these width/heights somewhere, as a number, and check them before setting them.

But we only set them when the window size has actually changed. So I'm not sure why this would be an issue?

`Animation` and `Timeline` no longer listen to `window.resize`. Instead the `Viewer` resizes them as needed. Also added checks to avoid unecessary resizing when the current size and new size are identical.

shunter · 2013-06-17T19:15:44Z

Source/Widgets/Viewer/Viewer.js

+            logoOffset.y = logoBottom;
+
+            if (timelineExists) {
+                this._timeline.container.style.left = animationExists ? animationWidth + 'px' : 0;


I think animationExists must be true here, given animationSizeChanged is true.

Actually, no. animationSizeChanged is now misnamed. My last commit introduced crashes if the Timeline or Animation widget were turned off. I'm fixing it now.

…l cases.

mramato · 2013-06-17T20:07:22Z

Okay, I think everything is covered. You might want to do some extra testing by turning some widgets on/off in CesiumViewer to make sure you are happy with the behavior.

shunter · 2013-06-17T20:16:56Z

Source/Widgets/Viewer/Viewer.js

@@ -228,6 +247,12 @@ define([
            fullscreenContainer.className = 'cesium-viewer-fullscreenContainer';
            viewerContainer.appendChild(fullscreenContainer);
            fullscreenButton = new FullscreenButton(fullscreenContainer, defaultValue(options.fullscreenElement, container));
+            if (!Fullscreen.isFullscreenEnabled()) {
+                fullscreenContainer.style.display = 'none';
+                timeline.container.style.right = 0;


Crashes here if timeline is disabled and Fullscreen is not enabled.

Also, recall that the fullscreen button can be disabled separately via the view model. Perhaps it should observe the isFullscreenEnabled property instead?

shunter · 2013-06-17T20:34:41Z

Source/Widgets/Viewer/Viewer.js

+        var animationExists = typeof this._animation !== 'undefined';
+        var animationContainer;
+
+        var resizeWidgets = false || !animationExists;


What's this false || doing here?

1. Make Specs issue resize and add specs that combine cases that were causing resize to throw. 2. Observe `fullscreenButton.viewModel.isFullscreenEnabled` so that we can hide/show the button as needed. 3. Replace some hard-coded values with clientHeight/clientWidth;

shunter · 2013-06-17T21:39:30Z

Source/Widgets/Viewer/Viewer.js

+                    fullscreenContainer.style.display = 'none';
+                }
+                if (typeof timeline !== 'undefined') {
+                    timeline.container.style.right = fullscreenContainer.clientWidth;


Need to call timeline.resize(); here.

…ptions generated by the default render loop. Also add code to prevent multiple render loops from being started simulataneously.

mramato · 2013-06-17T22:01:47Z

Well this pull request escalated quickly 😄 I fixed @emackey's concerns about the render loop and also added an event that gets raised if an exception is generated inside the default render loop. These exceptions are uncatchable by user code, so an event allows them to be propagated up. Let me know if that's anything else, otherwise this is ready.

emackey · 2013-06-18T03:05:37Z

Source/Widgets/Viewer/Viewer.js

+        this._lastWidth = 0;
+        this._lastHeight = 0;
+        this._useDefaultRenderLoop = undefined;
+        this._renderLoopShutdown = true;


I'm going to nitpick here, feel free to disregard. I don't like upside-down booleans where true means it's not running and false means it's running. Can we rename to _renderLoopRunning or even _expectingAnimationFrame? Yes I know it's private, but we harp on code readability and maintainability around here, so I thought I'd suggest it.

To explain my general thought process here, if you look at the only place it is used (line 525) you'll see if (value && this._renderLoopShutdown) Changing the boolean would mean that the only place we use it needs to negate it to get the value it really wants, that seems more "upside-down" to me than what we have here now. But, since I don't care, and you admit you're bikeshedding, I'll change it just to be a nice guy 😉

shunter · 2013-06-18T14:33:49Z

I fixed one last final bug when enabling/disabling the fullscreen button. This looks good to me. Merging.

Widget tweaks

1. Properly implement Timeline destroy to make sure everyting cleans up after itself 2. Add a sanity check spec that creates/destroys the Timeline as a stopgap until we write complete specs. 3. Fix a bug introduced in #870 that caused the Timeline to not redraw properly on scroll/zoom. This change does not attempt to fix the myriad of other issues with the Timeline, so please don't review it with our normal rigor. I'm hoping to carve out some time soon to finally get around to #754.

Widget tweaks

3e12f0b

1. Fixes #608 by adding a render counter and always resizing the window every 60 frames. 2. Add a `useDefaultRenderLoop` property and constructor option to `CesiumWidget` so that users can turn on/off the default render loop at will.

shunter reviewed Jun 17, 2013
View reviewed changes

Changes after review

a7f5f2f

1. Use frameNumber instead of a separate counter for determining when we should resize. 2. Have Viewer disable the default render loop and use it's own. 3. Expose `useDefaultRenderLoop` on Viewer as well.

Update CHANGES.

6a08676

shunter reviewed Jun 17, 2013
View reviewed changes

Fix copy/paste error.

7cb8c63

shunter reviewed Jun 17, 2013
View reviewed changes

mramato added 2 commits June 17, 2013 13:33

Declare members in the constructor.

2ad8f01

Cleanup variable initialization order.

37f5049

shunter reviewed Jun 17, 2013
View reviewed changes

Optimize resizing for Viewer and related widgets.

97f3e25

`Animation` and `Timeline` no longer listen to `window.resize`. Instead the `Viewer` resizes them as needed. Also added checks to avoid unecessary resizing when the current size and new size are identical.

shunter reviewed Jun 17, 2013
View reviewed changes

Hopefully final code changes to make Viewer resizing work again in al…

aaba55b

…l cases.

shunter reviewed Jun 17, 2013
View reviewed changes

Fix typo in documentation.

a2170c7

shunter reviewed Jun 17, 2013
View reviewed changes

mramato added 2 commits June 17, 2013 17:21

Moew resize cleanup

c63b9ea

1. Make Specs issue resize and add specs that combine cases that were causing resize to throw. 2. Observe `fullscreenButton.viewModel.isFullscreenEnabled` so that we can hide/show the button as needed. 3. Replace some hard-coded values with clientHeight/clientWidth;

Merge branch 'master' into widgetCleanup

9771e8c

shunter reviewed Jun 17, 2013
View reviewed changes

Ad onRenderLoopError events to Viewer and CesiumWidget for exce…

2bb904c

…ptions generated by the default render loop. Also add code to prevent multiple render loops from being started simulataneously.

emackey reviewed Jun 18, 2013
View reviewed changes

mramato and others added 3 commits June 18, 2013 06:58

Paint the bike shed blue.

de46e94

Remove unused module.

71d9b57

Fix missing px when setting style.

21b9fcd

shunter added a commit that referenced this pull request Jun 18, 2013

Merge pull request #870 from AnalyticalGraphicsInc/widgetCleanup

66ee5a3

Widget tweaks

shunter merged commit 66ee5a3 into master Jun 18, 2013

shunter deleted the widgetCleanup branch June 18, 2013 14:33

mramato mentioned this pull request Jun 18, 2013

Timeline destroy cleanup #874

Merged

This was referenced Jun 18, 2013

Return a requestID from requestAnimationFrame, add cancelAnimationFrame. #878

Merged

Viewer.js always defaults to Bing Imagery provider when baseLayerPicker option set to false #879

Closed

emackey mentioned this pull request Mar 11, 2019

Forced Reflow observed while profiling #7631

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Widget tweaks #870

Widget tweaks #870

mramato commented Jun 15, 2013

shunter Jun 17, 2013

mramato commented Jun 17, 2013

shunter Jun 17, 2013

mramato Jun 17, 2013

shunter Jun 17, 2013

mramato Jun 17, 2013

shunter Jun 17, 2013

mramato Jun 17, 2013

shunter Jun 17, 2013

mramato Jun 17, 2013

shunter Jun 17, 2013

mramato Jun 17, 2013

mramato commented Jun 17, 2013

shunter Jun 17, 2013

shunter Jun 17, 2013

shunter Jun 17, 2013

mramato Jun 17, 2013

shunter Jun 17, 2013

mramato Jun 17, 2013

mramato commented Jun 17, 2013

emackey Jun 18, 2013

mramato Jun 18, 2013

shunter commented Jun 18, 2013

Widget tweaks #870

Widget tweaks #870

Conversation

mramato commented Jun 15, 2013

Choose a reason for hiding this comment

mramato commented Jun 17, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mramato commented Jun 17, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mramato commented Jun 17, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shunter commented Jun 18, 2013