Fix home bounds with clipping. Fix #891

[avandecreme]

I had to modify the Viewport.viewportToImage conversion methods to always use the TiledImage.viewportToImage methods in order for clipping to be taken into account.

[iangilman]

The code looks reasonable, but I don't understand how TiledImage.viewportToImage... is taking clipping into account and thus helping Viewport.viewportToImage...... there doesn't seem to be any connection. What am I missing?

Also, I don't think those functions should be taking clipping into account anyway...we need to still mostly think in terms of the whole unclipped image, unless we want to dramatically change how we do things.

Anyway, set me straight :)

[avandecreme]

Ha yes this is hard to spot in the diff.
The issue is that now viewport._setContentBounds is passed the world bounds with clipping taken into account.

So, all the viewport._contentXXX fields now take clipping into account. As you said we don't want the conversion methods to take clipping into account so we can't rely on them (at least for conversion, they are still useful to compute viewport constraints and default zoom). So I modified the viewport/image conversion methods to always call the tiledImage ones.
I still left the previous conversions when the viewport as no viewer but I am not sure if that ever happen.

[avandecreme]

I checked if we are always checking for this.viewer existence before using it.
There a few places where we do not:
https://github.com/openseadragon/openseadragon/blob/master/src/viewport.js#L344
https://github.com/openseadragon/openseadragon/blob/master/src/viewport.js#L1229
https://github.com/openseadragon/openseadragon/blob/master/src/viewport.js#L1242
https://github.com/openseadragon/openseadragon/blob/master/src/viewport.js#L1300
https://github.com/openseadragon/openseadragon/blob/master/src/viewport.js#L1312

It wouldn't make sense to call the last four if there is no viewer though.

[iangilman]

I see! That makes a lot of sense... pretty subtle!

Yeah, I'm not sure where the idea that there could be a viewport without a viewer came in... that was before I got back on the project. I kinda feel like we don't really need to support that scenario, though I suppose it could be handy if you just wanted to use our viewport but do all of your own drawing. What do you think?

[avandecreme]

I really don't have much opinion on this. I can't think of a case where I would use just the viewport in a project but I don't mind adding the missing tests or removing all of them (and add an assert in the constructor that a viewer is specified).

[iangilman]

Well, looks like I do in fact use a viewport without a viewer here:

http://www.driftory.com/

... but that was written before we had multi-image; if I were to do it today I would just use multi-image instead.

Anyway, since there are so few tests to add, we might as well keep it up, for now at least. Sound fair?

[avandecreme]

I added a test in setMargins.
Should I add one for the others? What should I do if this.viewer is not defined?

1. Throw error
2. Log an error and return an incorrect value
3. ??

[iangilman]

I think I'm in favor of logging an error and returning an incorrect value (i.e. 0). Calling those if you don't have a viewer is a programmer error.

[iangilman]

... Or you could just stick a $.console.assert at the top of the function and let the function do whatever it will (presumably throw once it hits the unprotected viewer reference). Basically the goal here is just to give the developer a little more info as to what they did wrong, so an assert with a meaningful message should be sufficient.

[avandecreme]

Done!

[iangilman]

Excellent :)