Map.setCenter() should not call Layer.moveTo if inRange has changed to false

(In [4069]) Stop WFS layer from requesting data when layer is not in range. (Closes #685) Note that this is not a complete fix, but instead a patch for 2.5: after 2.5, this should be changed/removed when map.setCenter() is fixed. (See #937)

A patch for 1) and 2) in Map.js

A patch for 3) is required yet.

Layer.moveTo looks like:

    moveTo:function(bounds, zoomChanged, dragging) {
        var display = this.visibility;
        if (!this.isBaseLayer) {
            display = display && this.inRange;
        }
        this.display(display);
    },

if we don't need to call display(display), the method do nothing?

penyaskito: For the layer base class, yes. For subclasses, there is a lot of functionality in moveTo that we don't want to invoke when a layer is off -- essentially, moveTo is what does the redraw of all the tiles. If we move the display() call into setVisibility or similar, then this machinery doesn't need to be invoked just to call display(false)

See also:

http://openlayers.org/pipermail/users/2007-October/003124.html

From #1055, which I'm closing as a dupe:

I have corrected it in Map.js setCenter function, with some additional lines
of checks and conditions inside 'for' loop.
It works for me, but I do not know if it is good for any case.
Code below:
>
> for (var i = 0; i < this.layers.length; i++) {
>                 var layer = this.layers[i];
>                 if (!layer.isBaseLayer) {
>
>                     var moveLayer;
>                     var inRange = layer.calculateInRange();
>                     if (layer.inRange != inRange) {
>                         // Layer property has changed. We are going
>                         // to call moveLayer so that the layer can be
> turned
>                         // off or on.
>                         layer.inRange = inRange;
>                         //do NOT download tiles if not in scale limit
>                         moveLayer = (layer.visibility && layer.inRange);
> //
> was ... = true;
>                         //switch off layer after going beyond scale limit
>                         if (layer.visibility && !layer.inRange)
>                             layer.setVisibility(false);
>                         this.events.triggerEvent("changelayer");
>                     } else {
>                         // If nothing has changed, then we only move the
> layer
>                         // if it is visible and inrange.
>                         moveLayer = (layer.visibility && layer.inRange);
>                         //switch off layer after going beyond scale limit
>                         if (layer.visibility && !layer.inRange)
>                             layer.setVisibility(false);
>                     }
>                     if (moveLayer) {
>                         layer.moveTo(bounds, zoomChanged, dragging);
>                     }
>                 }
>             }



Greg

After much investigation on this ticket, and especially on what is done into the setCenter method to call moveTo on each layer, here are few notes :

• first, it seems to concern only non-baseLayer layers,
• the check for layer inRange property change is done in SetCenter,
• if layer inRange property changed, the "changelayer" event is triggered and the layer moveTo method is called, so that the diplay method can be called, apparently. But the moveTo method is called and for grid layers, it means that tiles are loaded whenever range matches or not (what user doesn't see because display is already set to none).
• if layer inRange property didn't change, the moveTo method is only called if visibility and inRange are both true.

What I propose is to move the inRange value change check into the layer calculateInrange method. There we can also set the inRange property to its new value, we can trigger the "changelayer" event, and we can change the layer display (call display method). Then the only thing we need to do in setCenter is to call the calculateInRange method so that the inRange value is updated, and check the layer visibility and inRange properties values.

The proposed patch also reverses the r4069 commit not required any more.

There is still something to check. The moveTo method in Layer.js calls the display method. Is it still required ? In what particular case ?

Please review the attached patch.

In OpenLayers.Map:setCenter() moveTo() is called unconditionally on the base layer. So, if you make OpenLayers.Layer.moveTo() empty, Layer.display() won't be called for the base layer. I'm not quite sure about the consequences of this, but I just want to mention that this change could have side-effects. Thinking a bit more about, I guess the map's current base layer is always visible (map.baseLayer.visibility is always true), so it doesn't make sense to call display(true) on the base layer on each map.setCenter(). So from my analysis, with your patch, OpenLayers.Layer.moveTo() could be empty and implemented only in subclasses.

Your patch looks good to me, but I'd like someone else's opinion on it. Someone that has more experience with these quite sensitive bits.

I think this looks good, and running through the tests, it doesn't seem to hurt anything. I'd say this looks fine to commit, and hopefully this can be the end of this particular problem :)

Pierre's patch should first accommodate changeset r5396.

With something like that:

var options = {
    displayInLayerSwitcher: false,
    // indicate that the temp vector layer will never be out of range
    // without this, resolution properties must be specified at the
    // map-level for this temporary layer to init its resolutions
    // correctly
    calculateInRange: function() { this.inRange = true; }
};

Note that I haven't tested this change. First I thought I'd have to use OpenLayers.Function.bind because of the reference to 'this' in the function, but I'm no longer sure that's needed.

In addition, I think we should check whether OpenLayers.Layer.moveTo() can now be empty before commit.

Oh, crap. You're right that you don't need to use bind here -- the function is only ever called with 'this' being the right thing, so it will work -- but I've advised people to use calculateInRange() {return true; } for a long time now :/ So that mechanism probably isn't the right one. (My bad on that one.)

So, I think that this code needs to change to make it so that the behavior we were previously using in the handler works. We can make it legacy code marked for removal, but the return value of calculateInRange should still be able to override the layer state...

Which may mean that this patch won't work at all...

In patch-937-r5542-B0.diff setCenter checks the return val of calculateInRange so code like Handler.Point that does calculateInRange = function() {return true;} continues to work. Also, the patch makes Layer.moveTo an empty function (to overridden by subclasses).

Tests pass on FF. Please review.

Ahh, I like it. Though moving display() into calculateInRange is perhaps the wrong path forward? Does it seem to a non-me person that you would expect calculateInRange to have side effects like changing the layer visibility?

Actually, I don't think this does what I want it to do (I'm sorry, I know I'm not being helpful here) -- the problem is that if someone overrides calculateInRange, I don't see when display() will ever be called. So although this works for the case where it's just 'return true', it doens't work if someone has updated calculateInRange to only display for certain zoom levels. Perhaps this use case doesn't matter -- calculateInRange should only be overridden to either do 'return true' or not overridden at all -- but I'd like to make sure that other people at least feel that way before we commit this...

Replying to crschmidt:

Ahh, I like it. Though moving display() into calculateInRange is perhaps the wrong path forward? Does it seem to a non-me person that you would expect calculateInRange to have side effects like changing the layer visibility? Actually, I don't think this does what I want it to do (I'm sorry, I know I'm not being helpful here) -- the problem is that if someone overrides calculateInRange, I don't see when display() will ever be called. So although this works for the case where it's just 'return true', it doens't work if someone has updated calculateInRange to only display for certain zoom levels. Perhaps this use case doesn't matter -- calculateInRange should only be overridden to either do 'return true' or not overridden at all -- but I'd like to make sure that other people at least feel that way before we commit this...

---

Right, having calculateInRange change the layer visibility might indeed not be appropriate. I'll work on another patch...

patch-937-r5899-C0.diff is a new, simpler patch. With this patch, if the layer is no longer in range layer.display(false) is called to turn the layer off but moveTo isn't called, which, for tiled layers, should prevent tiles from being needlessly loaded.

Unit tests pass on FF2/Linux.

The manual test draw-feature.html passes. This test relies on the overloading of the calculateInRange method done in Handler.Point.

r5901 includes a manual test show-casing the issues reported in this ticket. Test 1 and Test 3 of the manual test fail with current trunk. patch-937-r5899-C0.diff makes all tests of the manual test pass.

Looks good. Please commit.

final patch including pgiraud's modification to Layer/WFS.js (remove inRange check)

(In [5913]) Map.setCenter() should not call Layer.moveTo if inRange has changed to false. With this patch you should no longer see tiles loading if your layer is out of range or not visible. Hopefully! r=crschmidt (closes #937)

this is a great patch. very elegant work, elemoine!