Async layer Image Tiles never generate 'loadstart' events if draw() is called again before getURLasync() returns the first time

[chthonicionic]

OpenLayers v2.12 All browsers/OS as far as I can see - this is a logic error

Background: We use a Layer with .async set to true as the URLs of image tiles aren't known until the tile itself is generated. Moving to v2.12 we noticed the addin LoadingPanel control was still showing tiles as loading well after they'd loaded. A quick check showed that newly visible layers had .numLoadingTiles set to a negative number.

Reproducing the bug: Set up a map with two base layers, the second with .async = true and initially invisible. Show the second layer, then check numLoadingTiles for that layer. It will be negative.

What is happening is that the draw() function on a tile is getting called twice in rapid succession. I have yet to determine why, but that is not really the issue here - the double call means that the internal ._loadEvent property of the Image tile is set first to "loadstart" and then overwritten with "reload" before initImage gets a chance to trigger the event.

This means anything listening for "loadstart" events on a tile never gets that event. Layer/Grid.js then miscounts the number of loading tiles and never fires "loadend" events and backBuffer stuff never triggers. This is bad :)

The Fix: Tile/Image.js draw() should only set the _loadEvent property to "reload" if _loadEvent is empty Tile/Image.js initImage() should clear _loadEvent after triggering it.

Tile/Image.js line 167 becomes if (this.isLoading && !this._loadEvent) {

Tile/Image.js line 296 becomes this.events.triggerEvent(this._loadEvent); this._loadEvent = '';

(I wish I could find why the double call to .draw() is happening, but this logic error needs fixing regardless)

[chthonicionic]

I should add, this behaviour is new to v2.12

[chthonicionic]

Aha - found why .draw() gets called twice:

in Map.js, setBaseLayer() first calls this.baseLayer.setVisiblity(true) at line 1233 (which calls redraw() on the Layer) then it later calls this.setCenter(center, newZoom, false, true) at line 1248 (which also calls redraw() on the Layer) (it will get called a third time at line 1237 if the new layer is not inRange)

so basically switching baselayers to an async baselayer will trigger this bug.

[ahocevar]

Your issue might be fixed after applying #702. It would be interesting to hear if it is back when configuring a TileManager for your map - after applying #702.

[chthonicionic]

OK - I can see how the TileManager might help by making sure draw() is only called once when shifting baseLayers but it doesn't address the logic error in Tile/Image.js where it clearly will not generate a "loadstart" event in some circumstances (which is a problem).

I'll have a go at folding your TileManager in when I get a chance to see if it solves the problem. I'm guessing not many people use async layers.

--additional--

Actually, reading through the diffs for TileManager, that doesn't address the problem - the overwriting of ._loadevent property is still there if Tile.draw() is called again before Tile.initImage() runs. And it does get called at least twice when a baselayer changes due to both setVisibility() and setCenter() calling for a redraw() and that calling Layer/Grid.js .initGriddedTiles() which runs through the tiles and draw()s them.

[ahocevar]

I see the problem. A potential fix could include an initImage method that starts like this:

initImage: function(loadEvent) {
    this.events.triggerEvent(loadEvent || this._loadEvent);

and a renderTile method that remembers the loadEvent for async calls:

        var loadEvent = this._loadEvent; // remember the loadEvent type
        this.layer.getURLasync(this.bounds, function(url) {
            if (id == this.asyncRequestId) {
                this.url = url;
                this.initImage(loadEvent); // apply the remembered loadEvent type
            }
        }, this);

It would be great if you could try that out and report back. This should all work the same with and without the TileManager.

[chthonicionic]

Sorry - been on other projects. Yes. Using a closure is the only correct solution to async stuff. And your solution tries to ensure both the reload and load events fire if you call draw() more than once. The whole point of this fix is so that image tiles generate the correct events reliably.

Except, this code doesn't work!

Only the reload event triggers due to the if (id == this.asyncRequestId) { test. The first set of calls to renderTile come back, and are ignored because by then asyncRequestId has incremented.

So renderTile() has to emit events too to ensure we fulfil our contract. I've taken the liberty of eliminating the ._loadEvent property too as it's never going to work with asynchronous calls and made it a formal parameter of renderTile():

 draw: function() {
        var loadEvent = "";
        var drawn = OpenLayers.Tile.prototype.draw.apply(this, arguments);
        if (drawn) {
            // The layer's reproject option is deprecated.
            if (this.layer != this.layer.map.baseLayer && this.layer.reproject) {
                // getBoundsFromBaseLayer is defined in deprecated.js.
                this.bounds = this.getBoundsFromBaseLayer(this.position);
            }
            if (this.isLoading) { 
                loadEvent = "reload"; 
            } else {
                this.isLoading = true;
                loadEvent = "loadstart";
            }
            this.positionTile();
            this.renderTile(loadEvent);
        } else {
            this.unload();
        }
        return drawn;
    },

    /**
     * Method: renderTile
     * Internal function to actually initialize the image tile,
     *     position it correctly, and set its url.
     */
    renderTile: function(loadEvent) {
        this.layer.div.appendChild(this.getTile());
        if (this.layer.async) {
            // Asynchronous image requests call the asynchronous getURL method
            // on the layer to fetch an image that covers 'this.bounds'.
            var id = this.asyncRequestId = (this.asyncRequestId || 0) + 1;
            this.layer.getURLasync(this.bounds, function(url) {
                if (id == this.asyncRequestId) {
                    this.url = url;
                    this.initImage(loadEvent);
                } else {
                    this.events.triggerEvent(loadEvent); //make sure the event gets triggered regardless
                }
            }, this);
        } else {
            // synchronous image requests get the url immediately.
            this.url = this.layer.getURL(this.bounds);
            this.initImage(loadEvent);
        }
    },

    initImage: function(loadEvent) {
        this.events.triggerEvent(loadEvent);

[flavour]

Thanks @chthonicionic for this fix, which I found was an issue for me in Sahana (never noticed that loadend wasn't firing before with Grid layers)