BBOX strategy register for layer visibilitychanged

[igrcic]

I just changed original strategy-bbox example just a bit. I added photos.setVisibility(false); and minScale:10M, maxScale: 1 to bbox layer.

OK, here's how you reproduce it:
0)when u load the map, photos are not laoded cuz the initial zoom is at 14M
1)zoom 1 level more to get under 10M (it gets on 7M)
2)turn on Photos layer in switcher => no call is requested to get photos
3)nudge map a bit - 1px is enough => pictures are loaded


I dont know if some of the previous patched you made to bbox have to do something with this, but as far as i folowed i think this is seperate one!

Regards, Ivan

[igrcic]

And an example:  http://dev.openlayers.org/sandbox/igrcic/openlayers/examples/strategy-bbox.html

[elemoine]

this is probably due to Layer.redraw not triggering moveend after calling moveTo

[igrcic]

Hi Eric....i applied patch#1831 and #1834, if u were pointing to that? And still I get the same behaviour. Also, in #1831 it says its only for base layers, not overlays...

[tschaub]

Tests pass, example demonstrates that strategy is updated when visibility changes. Please review.

[elemoine]

i disagree with this patch because I think it hides the actual bug. setVisibility calls redraw, which calls moveTo. redraw should trigger a moveend event after calling moveTo.

[crschmidt]

Why should moveend be triggered when the layer didn't move?

[elemoine]

well, setVisibility() calls redraw() which calls moveTo(); so from that perspective the layer does move.

But yes, from the user perspective, making the layer visible does not (always) correspond to layer move.

The question is: should we always trigger moveend after moveTo is called, or should we trigger it only if the layer has actually changed location.

To me moveend should be triggered each time moveTo is called. moveend listeners may need to take action each time moveTo is called and not only each time the layer did actually move.

Hope i'm clear enough...

[crschmidt]

Conceptually, the function 'moveTo' is really just 'updateDisplayOfLayer' -- the name is not the only way to consider whether events have actually taken place, in my opinion.

In any case, I think that this change to the strategy *does* fix the problem -- and then the question should perhaps be "Should we look at changing the way these events behave, and possibly change the strategy after we do so" as a seperate task, since I bet this isn't the only case where moveend isn't called and moveTo is...

[elemoine]

yeah, I'm now convinced that Tim's patch makes sense.

I'm also now definitely convinced that Layer.moveTo is not the good place for triggering moveend, Map.moveTo is.

The one thing that I don't like too much is that data-fetching strategies have to listen to many types of event. For example, shouldn't the Fixed strategy also listen to visibilitychanged and fetch data at init only if the layer is visible?

I'll review Tim's patch...

[igrcic]

TNx guys, patch works for THAT case. But I supplied you with another one :-)

What if the layer is added only after we zoom in? Then loading features is not triggered until we move, refresh or change visibility of the layer.

Check out the:  http://dev.openlayers.org/sandbox/igrcic/openlayers/examples/strategy-bbox.html

I tried to add inside activate: function() :

if(activated) {

//this.update(); // OR registering it with map addlayer event this.layer.map.events.on({

"addlayer": this.update, scope:this

});

And they both worked...

Regards, Ivan

[tschaub]

register for layer visibilitychanged and update if map already has center

[tschaub]

Replying to igrcic:

TNx guys, patch works for THAT case. But I supplied you with another one :-) What if the layer is added only after we zoom in? Then loading features is not triggered until we move, refresh or change visibility of the layer.

When a layer is added to the map, if the map already has a center, the layer's "moveend" event should be triggered.

I don't get where this becomes a problem.

Check out the:  http://dev.openlayers.org/sandbox/igrcic/openlayers/examples/strategy-bbox.html I tried to add inside activate: function() : if(activated) { //this.update(); // OR registering it with map addlayer event this.layer.map.events.on({ "addlayer": this.update, scope:this });

Checking the map for any "addlayer" is a bit coarse. Let's make sure we understand the problem before going that far.

Can someone better describe how things might slip through with adding a layer after the map has a center?

(I updated the patch to check for the map center, but I actually don't like that change because I can't see why it is necessary.)

And they both worked... Regards, Ivan

[elemoine]

Tim, when an overlay is added to the map its redraw method is called. redraw moves the layer but does not trigger moveend.

[tschaub]

Replying to elemoine:

Tim, when an overlay is added to the map its redraw method is called. redraw moves the layer but does not trigger moveend.

That, it seems to me, is a problem.

[elemoine]

patch-1835-r8407-A0.diff is the patch I was initially proposing. Its drawback is that "moveend" events may be triggered while the map hasn't actually moved, on setVisibility(true) for example. But it should fix the problems reported in this ticket. All unit tests pass.

[tschaub]

trigger moveend with layer redraw

[tschaub]

Latest patch tweaks the test a little. I like the idea of always having the test methods called, instead of conditionally calling them. This way if things fail we see exactly where (instead of having to read all messages and see which one is missing when one of the tests is not run).

Eric, if you agree, and confirm that other tests pass, I think this is good to go.

[tschaub]

Reminder to elemoine that this is marked for commit.

[elemoine]

Replying to tschaub:

Reminder to elemoine that this is marked for commit.

This is committed already ([9195]), but for some reason the ticket didn't get closed.