Make maps better at exiting and reloading. #750
Conversation
Reduce repeated test code. This reuses or replaces the #map node in the headless tests rather than creating different named nodes for many different tests. This reduces the boiler-plate code for creating and destroying maps in the tests. Reusing elements exhibited some problems in map teardown and recreation. Specifically: (a) When the map was exiting some functions could be invoked with no-longer-valid renderer values. (b) An animation frame could fire after the vgl renderer had exited. (c) The scene object reset its children incorrectly. (d) The sliderWidget turned off all layer events for zoom rather than just its own. (e) Tests that mock VGL must clean up their maps before restoring VGL, or when the map is exited it may not have the appropriate state information. Fix some inconsistencies on widgets. For instance, if a widget is position relative to the screen and then changed to relative to the map, it wouldn't update on pan, but if it started as relative to the map, it would update on pan. Update jsdocs on widgets.
| @@ -1482,7 +1482,9 @@ var map = function (arg) { | |||
| if (m_discreteZoom) { | |||
| m_this.zoom(Math.round(m_this.zoom())); | |||
| } | |||
| m_this.interactor().options({discreteZoom: m_discreteZoom}); | |||
| if (m_this.interactor()) { | |||
There was a problem hiding this comment.
can we have a situation where we do not have interactor?
There was a problem hiding this comment.
Yes, if you want to display a static map. We have a test without an interactor.
| @@ -178,7 +178,7 @@ var sceneObject = function (arg) { | |||
| * Free all resources and destroy the object. | |||
| */ | |||
| this._exit = function () { | |||
| m_this.children = []; | |||
| m_children = []; | |||
| @@ -171,21 +206,30 @@ var widget = function (arg) { | |||
| // if the property is a number, add px to it, otherwise set it to the | |||
| // specified value. Setting a property to null clears it. Setting to | |||
| // undefined doesn't alter it. | |||
| if (/^\s*(\-|\+)?(\d+(\.\d*)?|\d*\.\d+)([eE](\-|\+)?\d+)?\s*$/.test(position[cssAttr])) { | |||
| if (/^\s*(-|\+)?(\d+(\.\d*)?|\d*\.\d+)([eE](-|\+)?\d+)?\s*$/.test(position[cssAttr])) { | |||
There was a problem hiding this comment.
whats change here? sorry it is hard to debug for review 😄
There was a problem hiding this comment.
I removed some needless escaping; that is all.
| $('#map-create-map').css({width: '400px', height: '400px'}); | ||
| expect(m.size()).toEqual({width: 500, height: 500}); | ||
| var m = createMap(); | ||
| expect(m.size()).toEqual({width: 640, height: 360}); |
There was a problem hiding this comment.
I'm using a generic test element that is 640 x 360 (for no particular reason that I know of). Before, this create a map that was a different size; since we don't really care what the size is (only that it is what we expect), when calling the generic function, test that it is the size we expect.
|
I think @matthewma7 found a condition we should guard against, so I'll have at least one more commit to this branch. |
Use the proper context for the vgl quads rather than the generic context. This makes cleaning up the layer's context more robust. This also tries to lose a context as soon as possible, if the browser supports doing so.
manthey
force-pushed
the
reuse-map-node-in-tests
branch
from
3696e59 to
2a13128
Compare
|
Before this PR, we weren't exiting the vgl renderer the correct way, so instead of doing a targeted cleanup, it did a general one. When that was fixed, it exposed the fact that the quads were using a generic context rather than a specific context for their shader cache. This would hang on to the context. |
|
@manthey Yes, I tried it out. It is totally fixed for me. 👍 |
|
@aashish24 Since I made changes since you approved it, can you make sure you are happy with those changes? (or @matthewma7 could also approve). |
| prog.addShader(vgl.getCachedShader( | ||
| vgl.GL.FRAGMENT_SHADER, vgl.GL, fragmentShaderImageSource)); | ||
| vgl.GL.FRAGMENT_SHADER, context, fragmentShaderImageSource)); |
There was a problem hiding this comment.
Historically, this was really only using the context to access constants, so we thought the general class was sufficient.
Reduce repeated test code. This reuses or replaces the #map node in the headless tests rather than creating different named nodes for many different tests. This reduces the boiler-plate code for creating and destroying maps in the tests.
Reusing elements exhibited some problems in map teardown and recreation. Specifically: (a) When the map was exiting some functions could be invoked with no-longer-valid renderer values. (b) An animation frame could fire after the vgl renderer had exited. (c) The scene object reset its children incorrectly. (d) The sliderWidget turned off all layer events for zoom rather than just its own. (e) Tests that mock VGL must clean up their maps before restoring VGL, or when the map is exited it may not have the appropriate state information.
Fix some inconsistencies on widgets. For instance, if a widget is position relative to the screen and then changed to relative to the map, it wouldn't update on pan, but if it started as relative to the map, it would update on pan.
Update jsdocs on widgets.