From 8178687298626533c48a94aa3099416e0fbbacda Mon Sep 17 00:00:00 2001 From: Steve Halasz Date: Thu, 10 Sep 2020 17:40:30 -0400 Subject: [PATCH 1/3] Better handle destruction when navigator in custom location --- src/button.js | 24 ++++++++++++++++-------- src/control.js | 4 +++- src/viewer.js | 10 +++++++++- 3 files changed, 28 insertions(+), 10 deletions(-) diff --git a/src/button.js b/src/button.js index 7b00f9ee..5668c871 100644 --- a/src/button.js +++ b/src/button.js @@ -399,14 +399,22 @@ $.extend( $.Button.prototype, $.EventSource.prototype, /** @lends OpenSeadragon. }, destroy: function() { - this.element.removeChild(this.imgRest); - this.imgRest = null; - this.element.removeChild(this.imgGroup); - this.imgGroup = null; - this.element.removeChild(this.imgHover); - this.imgHover = null; - this.element.removeChild(this.imgDown); - this.imgDown = null; + if (this.imgRest) { + this.element.removeChild(this.imgRest); + this.imgRest = null; + } + if (this.imgGroup) { + this.element.removeChild(this.imgGroup); + this.imgGroup = null; + } + if (this.imgHover) { + this.element.removeChild(this.imgHover); + this.imgHover = null; + } + if (this.imgDown) { + this.element.removeChild(this.imgDown); + this.imgDown = null; + } this.removeAllHandlers(); this.tracker.destroy(); this.element = null; diff --git a/src/control.js b/src/control.js index 64f762d7..6cb229f8 100644 --- a/src/control.js +++ b/src/control.js @@ -161,7 +161,9 @@ $.Control.prototype = { */ destroy: function() { this.wrapper.removeChild( this.element ); - this.container.removeChild( this.wrapper ); + if (this.anchor !== $.ControlAnchor.NONE) { + this.container.removeChild(this.wrapper); + } }, /** diff --git a/src/viewer.js b/src/viewer.js index b5b24318..9f6e767a 100644 --- a/src/viewer.js +++ b/src/viewer.js @@ -775,7 +775,13 @@ $.extend( $.Viewer.prototype, $.EventSource.prototype, $.ControlDock.prototype, this.removeAllHandlers(); if (this.buttons) { - this.buttons.destroy(); + if (this.buttons instanceof $.ButtonGroup) { + this.buttons.destroy(); + } else { + while (this.buttons.length) { + this.buttons.pop().destroy(); + } + } } if (this.paging) { @@ -1869,6 +1875,8 @@ $.extend( $.Viewer.prototype, $.EventSource.prototype, $.ControlDock.prototype, {anchor: this.navigationControlAnchor || $.ControlAnchor.TOP_LEFT} ); } + } else { + this.buttons = buttons; } } From 0ede8992de954e2a1641b7ce60fb865d3c3849a8 Mon Sep 17 00:00:00 2001 From: Steve Halasz Date: Fri, 18 Dec 2020 13:25:29 -0500 Subject: [PATCH 2/3] Separate properties for buttonGroup and customButtons This makes it more clear what we're testing for or calling methods on, vs. assigning to this.buttons in both the useGroup true and false cases. --- src/viewer.js | 40 ++++++++++++++++++++-------------------- test/modules/controls.js | 12 ++++++------ 2 files changed, 26 insertions(+), 26 deletions(-) diff --git a/src/viewer.js b/src/viewer.js index 9f6e767a..07d55e9e 100644 --- a/src/viewer.js +++ b/src/viewer.js @@ -409,14 +409,14 @@ $.Viewer = function( options ) { if (!this.drawer.canRotate()) { // Disable/remove the rotate left/right buttons since they aren't supported if (this.rotateLeft) { - i = this.buttons.buttons.indexOf(this.rotateLeft); - this.buttons.buttons.splice(i, 1); - this.buttons.element.removeChild(this.rotateLeft.element); + i = this.buttonGroup.buttons.indexOf(this.rotateLeft); + this.buttonGroup.buttons.splice(i, 1); + this.buttonGroup.element.removeChild(this.rotateLeft.element); } if (this.rotateRight) { - i = this.buttons.buttons.indexOf(this.rotateRight); - this.buttons.buttons.splice(i, 1); - this.buttons.element.removeChild(this.rotateRight.element); + i = this.buttonGroup.buttons.indexOf(this.rotateRight); + this.buttonGroup.buttons.splice(i, 1); + this.buttonGroup.element.removeChild(this.rotateRight.element); } } @@ -774,13 +774,11 @@ $.extend( $.Viewer.prototype, $.EventSource.prototype, $.ControlDock.prototype, this.removeAllHandlers(); - if (this.buttons) { - if (this.buttons instanceof $.ButtonGroup) { - this.buttons.destroy(); - } else { - while (this.buttons.length) { - this.buttons.pop().destroy(); - } + if (this.buttonGroup) { + this.buttonGroup.destroy(); + } else if (this.customButtons) { + while (this.customButtons.length) { + this.customButtons.pop().destroy(); } } @@ -1855,13 +1853,13 @@ $.extend( $.Viewer.prototype, $.EventSource.prototype, $.ControlDock.prototype, } if ( useGroup ) { - this.buttons = new $.ButtonGroup({ + this.buttonGroup = new $.ButtonGroup({ buttons: buttons, clickTimeThreshold: this.clickTimeThreshold, clickDistThreshold: this.clickDistThreshold }); - this.navControl = this.buttons.element; + this.navControl = this.buttonGroup.element; this.addHandler( 'open', $.delegate( this, lightUp ) ); if( this.toolbar ){ @@ -1876,7 +1874,7 @@ $.extend( $.Viewer.prototype, $.EventSource.prototype, $.ControlDock.prototype, ); } } else { - this.buttons = buttons; + this.customButtons = buttons; } } @@ -3490,8 +3488,10 @@ function doSingleZoomOut() { function lightUp() { - this.buttons.emulateEnter(); - this.buttons.emulateExit(); + if (this.buttonGroup) { + this.buttonGroup.emulateEnter(); + this.buttonGroup.emulateExit(); + } } @@ -3510,8 +3510,8 @@ function onFullScreen() { this.setFullScreen( !this.isFullPage() ); } // correct for no mouseout event on change - if ( this.buttons ) { - this.buttons.emulateExit(); + if ( this.buttonGroup ) { + this.buttonGroup.emulateExit(); } this.fullPageButton.element.focus(); if ( this.viewport ) { diff --git a/test/modules/controls.js b/test/modules/controls.js index 57d827c9..0e6bdf7c 100644 --- a/test/modules/controls.js +++ b/test/modules/controls.js @@ -53,9 +53,9 @@ assert.ok(viewer.showZoomControl, 'showZoomControl should be on'); assert.ok(!!viewer.zoomInButton, "zoomIn button should not be null"); assert.ok(!!viewer.zoomOutButton, "zoomOut button should not be null"); - assert.notEqual(viewer.buttons.buttons.indexOf(viewer.zoomInButton), -1, + assert.notEqual(viewer.buttonGroup.buttons.indexOf(viewer.zoomInButton), -1, "The zoomIn button should be present"); - assert.notEqual(viewer.buttons.buttons.indexOf(viewer.zoomOutButton), -1, + assert.notEqual(viewer.buttonGroup.buttons.indexOf(viewer.zoomOutButton), -1, "The zoomOut button should be present"); var oldZoom = viewer.viewport.getZoom(); @@ -108,7 +108,7 @@ viewer.removeHandler('open', openHandler); assert.ok(viewer.showHomeControl, 'showHomeControl should be on'); assert.ok(!!viewer.homeButton, "Home button should not be null"); - assert.notEqual(viewer.buttons.buttons.indexOf(viewer.homeButton), -1, + assert.notEqual(viewer.buttonGroup.buttons.indexOf(viewer.homeButton), -1, "The home button should be present"); viewer.viewport.zoomBy(1.1); @@ -167,7 +167,7 @@ viewer.removeHandler('open', openHandler); assert.ok(viewer.showHomeControl, 'showFullPageControl should be on'); assert.ok(!!viewer.fullPageButton, "FullPage button should not be null"); - assert.notEqual(viewer.buttons.buttons.indexOf(viewer.fullPageButton), -1, + assert.notEqual(viewer.buttonGroup.buttons.indexOf(viewer.fullPageButton), -1, "The full page button should be present"); assert.ok(!viewer.isFullPage(), "OSD should not be in full page."); @@ -223,9 +223,9 @@ assert.ok(viewer.drawer, 'Drawer exists'); assert.ok(viewer.drawer.canRotate(), 'drawer.canRotate needs to be true'); assert.ok(viewer.showRotationControl, 'showRotationControl should be true'); - assert.notEqual(viewer.buttons.buttons.indexOf(viewer.rotateLeftButton), -1, + assert.notEqual(viewer.buttonGroup.buttons.indexOf(viewer.rotateLeftButton), -1, "rotateLeft should be found"); - assert.notEqual(viewer.buttons.buttons.indexOf(viewer.rotateRightButton), -1, + assert.notEqual(viewer.buttonGroup.buttons.indexOf(viewer.rotateRightButton), -1, "rotateRight should be found"); // Now simulate the left/right button clicks. From 6c317d080775cf7519962a77083b1f053dd61258 Mon Sep 17 00:00:00 2001 From: Steve Halasz Date: Mon, 8 Mar 2021 08:18:40 -0500 Subject: [PATCH 3/3] Changleg for #1878 --- changelog.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/changelog.txt b/changelog.txt index d2e5f249..8dec804a 100644 --- a/changelog.txt +++ b/changelog.txt @@ -6,6 +6,7 @@ OPENSEADRAGON CHANGELOG * Now when "simple image" tile sources are removed from the viewer, they free the memory used by the pyramid they create (#1789 @TakumaKira) * Documentation fix (#1814 @kenanchristian) * Better cleanup on destruction, to avoid memory leaks (#1832 @JoFrMueller) +* Better handle destruction when navigator in custom location (#1884 @woodchuck) * Miscellaneous code cleanup (#1840 @msalsbery) * You can now specify tileSize for the Zoomify Tile Source (#1868 @abrlam) * Better use of IIIF "max" and "full" URL parameters (#1871 @MImranAsghar) @@ -56,7 +57,7 @@ OPENSEADRAGON CHANGELOG * You can now prevent canvas-click events on the navigator (#1416) * The navigator can now be restricted to just horizontal or just vertical panning (#1416) * Fixed DziTileSource so it doesn't load levels above maxLevel or below minLevel, if set (#1492) - + 2.3.1: * Debug mode now uses different colors for different tiled images (customizable via debugGridColor) (#1271)