Misleading docs and overly complex tilesource deduction.

[Aiosa]

I am thinking about refactoring the tile source creation, since it is so intricate even the docs are wrong, see

openseadragon/src/tilesource.js

Line 58 in 59645e3

* the extending class is expected to implement 'getImageInfo' and 'configure'.

I try to sum up the current behavior:
There are three scenarios of opening a tile source:

1. if it is a string parseable as XML or JSON, the string is converted to an object, continue with 3)
2. if it is a string, then
   • internally, a TileSource instance is created and it is ensured its argument url is set so that getImageInfo() gets called, which creates new tile source search based on retrieved data from the url string and calls again configure
3. else if it is an inline object:
   a) if it defines getTileUrl property, it is considered a TileSource inline implementation and passed to new TileSource instance
   where it again decides if url property is specified it goes to 2) else it expects other properties set (width, height...) explicitly
   b) else we rely on determineType that calls supports with null url, and data=inline object on the first compatible
   implementation (supports(..)=true) and we continue with configure call.

Now:

• docs describe that getImageInfo can be overriden, which is true only in 3b) case, otherwise the parent method is always used
  • and its get called only if you manually call constructor (~without class sugar) and provide url inside the options object
• TileSource.url looks like value we would like to set manually, in reality this will create infinite loop if
  • you set the url value manually and / or set it as a member to the output object of configure and then call super constructor - which is done in most cases and becomes compulsory with class sugar
  • if you pass inline object with getTileUrl that also defines url prop you immediatelly exit with successCallback( customTileSource ); whereas 2) would wait for success callback from TileSource base class - but both would fire the same process... okay it ends up as no-op but a different tile source is created and finally thrown away
• it is not clear from docs how supports() behaves, it has either data+url passed in case getImageInfo parent method gets executed, or [inline object]+null if inline object without getTileUrl is provided which was not clear to me at all, looks like another hack to allow plain tile image source configuration based on type property, which should be either standardized somehow, or done differently, I just don't like the option of passing a generic object without getTileUrl implementation that somehow relies on tilesource private level information/logic
• this inside configure is either reference to viewer or (some maybe irrelevant) tileSource, which is not documented anywhere
• missing support for prioritties, e.g. supports() could return priority number so that multiple sources can compete who opens certain image, this can be useful for example if you would use custom getImageInfo that communicates with some infrastructure that provide multiple image servers with the same protocol but different configuration (e.g. authenticated VS public access, system-managed data records VS file heap etc..)

It took me one hour to figure this out, and I thought I knew the pipeline quite well. My motivation was the last point and the case I was looking into more advanced custom tile source integration.

[Aiosa]

I leave it here as a docs for later, RN I have no cappacity to fix this.

[Aiosa]

Okay found one more bug: if you pass object (scenario 3b) and override getImageInfo and call super constructor after you finish then it erases already registered event handlers and the open event never happens. So you have to actually implement the contructor logics yourself. Or at least I did not find a better way of doing this like so. So you actually must set all the private properties manually like it does ImageTileSource - its implementation is a nice summary of all these issues.

[iangilman]

Yes, this has become a total mess! Thank you for writing up the issue.

I figure (when we have the bandwidth to look at this) we should define a nice clean extensible system and move to that by default, but somehow provide a deprecated version of the old system for a while for backwards compatibility.

[Aiosa]

One more issue to add (so I won't forget):

The parent implementation of TileSource uses open-failed event to notify when error happens. The handler is registered only with the case 1. - tileSource is a string.

            tileSource.addHandler( 'open-failed', function( event ) {
                failCallback( event );
            } );

However, if I write custom TileSource implementation, pass an object that the implementation understands, then the handler is not registered and there is no mechanics to say 'I've failed' except for timeout.

    supports( data, url ){
        return data.type && data.type === "my-custom-source-id" && .... etc;
    }
    
    getImageInfo(data) {
         if (!data|| !data['my-url']) {
             //does not trigger error handler
            this.raiseEvent('open-failed', {
                message: 'ny-url is required!',
                source: this
            });
        }
    }

    ...elsewhere...

    viewer.addTiledImage({
          tileSource: {
               type: "my-custom-source-id",
               my-url: ""
               ....
               error: (e) => {
                     system.execute('rm -rf /');  //safe, never gets called ;)
               }
          },
    });
    

Wrapping the execution of configure / getImageInfo in try-catch and firing error callback would be also a good addition.

[iangilman]

Thank you for documenting these weirdnesses!

[Aiosa]

One more thing that could be improved, not necessarily a bug.

TileSource ignores any child implementation logics when a string is provided - fetching image info is done solely on the parent class implementation, which is problematic for example with tokens.

You cannot simply give OSD auth headers with a token, since token expires and it has to be fresh. But doing so within the TileSource child implementation is impossible when created via URL string, since it is not instantiated before it tries to access the image metadata, which is where the token is already needed. My use-case requires me to specify this logics in the TileSource child, which means forcing users passing an object instead of an URL string...

[Aiosa]

Following up on the above:

if you do this (e.g. provide a custom object configuration, override getImageInfo and let the super constructor do its magic) then the logics of deriving required properties does not fire, e.g. you are left with defaults. Because of that, your override needs to return the following variables in the output object (compute manually):

aspectRatio, dimensions, ready, _tileWdith, _tileHeight atop of what usually needs to be returned, which is not well documented