preserveOverlays now handles both this.overlays and addOverlay()

[ShadOoW]

Hello,

Following 561 pull request discussion.
Please review and let me know if any modification are needed.

Edit: the test is failling probably because this.preserveOverlays is set to true.

[ShadOoW]

Tests for overlay are easy to fix, but it seem that I havn't taken in consideration viewers with multiple pages viewer.goToPage( 1 ); since each define it own overlays array.

I've never used pages and so I'm not sure what it is the expected behavior for overlays in the case of multiple pages, any ideas on how to go about this?

[iangilman]

This change changes how the overlays option behaves as well as how Viewer.addOverlay() behaves; please update their documentation as well.

[iangilman]

It's possible to include overlays for each tile source in the tilesources option. The intention with those is that they come and go as you open and close each tile source (with goToPage, etc). If we always carry over currentOverlays, then they'll bleed from one image to another.

I suppose one solution would be to go through the currentOverlays on close and pull out any that came from the current tile source.

As for the tests, they'll probably need to be updated...they're testing the old behavior, and this patch changes that behavior.

Another thing to include in this patch is clearing out all of the overlays in Viewer.destroy().

[ShadOoW]

About Viewer.destroy() I will do this next, for now i will work the overlay/pages issue first, i think i've understood it behavior.

Some thoughts:
One requirement i've implemented this function in respect to, is that we shoudn't only unload and then reload overlays on each page, because sometimes the overlays are created dynamically and are keeping track internaly of some informations (like it internal visibility based on the viewer actualzoom), so they shoud not be deleted and recreated between pages (open()s). If the opposite behavior is required the user could simply set preserveOverlay = false, and put the overlays adding code in the open event. (the only drawback back is memory usage)

And so if we apply this to pages then we should 'hide' and not 'destroy' the overlays between pages switches.

one way would be to set this.overlays = this.currentOverlays in the close event, which will save all currentOverlays until the next open();

[iangilman]

I agree with that principle for overlays added to the viewer in general, but I think it may be different for overlays provided with the individual tile sources. Imagine you have fifty pages, and they each have twenty overlays associated with them. That's 980 hidden overlays for any page you're on (since only 20 of them will be relevant on any page).

At any rate, you certainly wouldn't want to just do this.overlays = this.currentOverlays because the developer doesn't want to see the overlays for page 1 appearing on page 2.

[ShadOoW]

He could set preserveOverlays to false to avoid this issue, we could also 'force' preserveOverlay to false between page switches, but we will sacrifice the flexibility of being able to choose...

I will do some tests to confirm all this.

[iangilman]

Well, I think it's our responsibility to make the scenario work if we're going to default to having preserveOverlays on. If we leave it defaulted to off, then it's ok that it's not compatible with the tile source overlays option.

Just so we're on the same page, let's see if I can summarize where we've been and where we're going:

Original

• Global overlays option: applied on initial open, reapplied on all subsequent opens
• Tile source overlays option: applied only when (and every time) that tile source is opened
• addOverlay function: applied only when called
• close function: removes all overlays
• destroy function: overlays are already gone; nothing to do

After #561, with preserveOverlays = false

Same as original.

After #561, with preserveOverlays = true

• Global overlays option: ignored
• Tile source overlays option: applied only when that tile source is opened, but then kept around on all future opens. If you open the tile source a second time, you'll get a second copy of the overlay
• addOverlay function: applied when called and kept around for future opens
• close function: doesn't remove overlays
• destroy function: allows overlays to leak

Current state of #580, with preserveOverlays = false

Same as original, except:

• Global overlays option: applied only on initial open

Current state of #580, with preserveOverlays = true

• Global overlays option: applied on initial open, reapplied on all subsequent opens
• Tile source overlays option: applied only when that tile source is opened, but then kept around on all future opens. If you open the tile source a second time, you'll get a second copy of the overlay
• addOverlay function: applied when called and kept around for future opens
• close function: doesn't remove overlays
• destroy function: allows overlays to leak

Possible future, with preserveOverlays = false

Same as original, except:

• Global overlays option: applied only on initial open

Possible future, with preserveOverlays = true

• Global overlays option: applied on initial open, reapplied on all subsequent opens
• Tile source overlays option: applied only when (and every time) that tile source is opened; not kept around for other opens
• addOverlay function: applied when called and kept around for future opens
• close function: doesn't remove overlays
• destroy function: removes all overlays

Does all that seem right?

[ShadOoW]

I will work on the possible future with preserveOverlays = true
All seem correct thanks.

[iangilman]

Sounds good. How would you feel about my merging #446 into master? It's getting about time to do that. That branch has some changes in how we deal with overlays, but whatever you do with this patch will have to be merged into it eventually anyway. Sound fair?

[ShadOoW]

Yeah, please do, I will have to adapt to it so the sooner the better.

[iangilman]

Right on. :)

[iangilman]

@ShadOoW Ok, #446 has been merged in. You'll want to pull the latest openseadragon master into your fork. The way we handle overlays in the viewer is a little different; please take a look.

[ShadOoW]

Thanks, Noted.

[ShadOoW]

This is way above my js skills and understanding of openseadragon code base.
Persisting overlays between pages has proven to be more complex than persisting them between open()s, a lot of code need to be modified.

The IDEA

-Overlays if preserveOverlays is set to true, need to be kept alive (not destroyed then recreated) while switching between pages, or loading a new image.

Feedback

-I guess when switching to a different page, the previous page overlays visibility should change to collapsed, while on viewer.open(), they should simply persist.
-An enumeration is probably needed, to define the scope; what overlays are persisted (all, none, global, source, addOverlay()).
-I have no idea what to do with this.overlaysContainer.innerHTML, the overlays HTML should probably not be cleared all at once, but selectively depending on the enumeration state.

Conclusions

I think we should rollback, to the original state meanwhile, but i would love to see this implemented.
The actual preserveOverlays, does the job for people not using Pages, and using ShadowDom as overlay with internal state, so it may still be useful.

Open For Grabs!

[iangilman]

@ShadOoW Cool, thank you for trying, and thank you for passing it on so gracefully.

I think I'll leave #561 in but add a little more in the way of docs (not to be mixed with other overlay techniques) and take care of the leak on destroy(). Beyond that, we'll see if someone else wants to pick up this thread and take it further.

[iangilman]

Minor cleanup done (#601)