Smoother dragging of the map on older computers

Didn't have a deep look at your code but before I forget I just noticed that you could use

clearTimeout(this.updateMapTimeout);

instead of

this.updateMapTimeout = null;

but I might be wrong.

I'm also not convinced that Control.DragFeature is the right component for such a ticket.

Using clearTimeout instead of this.updateMapTimeout would neither work nor be necessary. Since it happens inside of the timeout, it is already cleared (or, at least it won't be called again). And clearTimeout clears the timeout with the id in the argument, but the variable that contains the timeout id is not cleared (since it is just an integer).

Btw, about the timeout: The line if(!this.updateMapTimeout) should maybe be changed to if(this.updateMapTimeout == null) just in case a timeout would get the id 0. I'll upload a modified versjon of the patch. :)

Which component would you suggest if it should not be in Control.DragFeature? As I said, I don't know the code structure very well. The reason that I chose Control.DragFeature was that the code was located in Control/DragPan.js.

Hi, I tested the patch - works smoothly. Maybe setting higher timeout would not harm. Running application can be seen at http://tourist.posazavi.com/cz/mapy.asp

+1 for adding this patch.

Replying to haakeyar:

Which component would you suggest if it should not be in Control.DragFeature? As I said, I don't know the code structure very well. The reason that I chose Control.DragFeature was that the code was located in Control/DragPan.js.

I looked at your code and saw that what you do is actually delay the action of the panMap function in Control/DragPan. The panMap function is linked to the move function of Handler/DragPan.

So I would suggest to do the delay in Handler/Drag.js. By doing so, every control interaction that is linked to the move function of the handler will feel more responsive.

I attached a patch with the required change. It should have the same effect on responsiveness of map dragging as the original patch.

I created a patch as an alternative of yours.

Alternative to the original patch, delaying the move function in OpenLayers.Handler.Drag

Replying to ahocevar:

So I would suggest to do the delay in Handler/Drag.js. By doing so, every control interaction that is linked to the move function of the handler will feel more responsive. I attached a patch with the required change. It should have the same effect on responsiveness of map dragging as the original patch. I created a patch as an alternative of yours.

Thanks for your feedback and alternative patch. :)

That could possibly work out well, with a little tweaking. As far as I can see now, though (without testing your patch), I can't imagine that it will work in it's current state. For example, the panMap-function uses the last.x and last.y values of the handler to calculate the new position of the map. Also, I don't understand your logic with the setTimeout/clearTimeout. As far as I can see, with your code, the map won't be updated before you stop dragging for 50ms. (I'm sorry if there is something terribly wrong with my code reading and your patch actually runs perfectly. :))

Another concern with placing it there, is the drawing-functions. Would they be accurate enough with the drag position being updated only every 50 ms or so?

A concern with both patches is that in the call to this.map.setCenter in panMap, the third argument is this.handler.dragging. The value of that could potentionally become false before the timeout is run. I have no idea what that argument does and why it is there in the first place (wouldn't it always be true?), but it will have to bee looked into. (If no one else does, I'll probably look into it on saturday.) Things like that are more likely to appear when you add the timeout up in the event handling, because more code is dependent on it. This way, my patch would probably be more secure (security for bugs, not security-security), while yours would give a better result when tweaked correctly.

Replying to haakeyar:

A concern with both patches is that in the call to this.map.setCenter in panMap, the third argument is this.handler.dragging. The value of that could potentionally become false before the timeout is run. I have no idea what that argument does and why it is there in the first place (wouldn't it always be true?), but it will have to bee looked into. (If no one else does, I'll probably look into it on saturday.)

From the documentation on the third argument to setCenter: "dragging {Boolean} Specifies whether or not to trigger movestart/end events" http://dev.openlayers.org/apidocs/files/OpenLayers/Map-js.html#OpenLayers.Map.setCenter

So, I would guess that we could just set the third argument to true, since that's what it would have been if the action wasn't delayed (as far as I can see), but I might be wrong. I'll have to leave that to you hardcore OpenLayers developers.

Replying to haakeyar:

Replying to haakeyar:

A concern with both patches is that in the call to this.map.setCenter in panMap, the third argument is this.handler.dragging. The value of that could potentionally become false before the timeout is run. I have no idea what that argument does and why it is there in the first place (wouldn't it always be true?), but it will have to bee looked into. (If no one else does, I'll probably look into it on saturday.)

From the documentation on the third argument to setCenter: "dragging {Boolean} Specifies whether or not to trigger movestart/end events" http://dev.openlayers.org/apidocs/files/OpenLayers/Map-js.html#OpenLayers.Map.setCenter So, I would guess that we could just set the third argument to true, since that's what it would have been if the action wasn't delayed (as far as I can see), but I might be wrong. I'll have to leave that to you hardcore OpenLayers developers.

In addition to being called on move events, panMap() is also called from within panMapDone(). When called from within panMapDone(), this.handler.dragging is set to false since we're done panning. For that reason, we just can't set dragging to true.

In addition, setCenter() dragging parameter doesn't only specify whether or not to trigger movestart/end events. The doc is a bit misleading with respect to that. See <http://trac.openlayers.org/ticket/967>.

-- Eric

Replying to ahocevar:

Replying to haakeyar:

Which component would you suggest if it should not be in Control.DragFeature? As I said, I don't know the code structure very well. The reason that I chose Control.DragFeature was that the code was located in Control/DragPan.js.

I looked at your code and saw that what you do is actually delay the action of the panMap function in Control/DragPan. The panMap function is linked to the move function of Handler/DragPan. So I would suggest to do the delay in Handler/Drag.js. By doing so, every control interaction that is linked to the move function of the handler will feel more responsive. I attached a patch with the required change. It should have the same effect on responsiveness of map dragging as the original patch. I created a patch as an alternative of yours.

I feel that Handler.Drag should stay generic as some controls may not want delayed events.

If this delay mechanism should really be implemented in Handler.Drag, which I'm still not sure, one option would be to add a "delay" option to Handler.Drag.

Replying to elemoine:

In addition to being called on move events, panMap() is also called from within panMapDone(). When called from within panMapDone(), this.handler.dragging is set to false since we're done panning. For that reason, we just can't set dragging to true. In addition, setCenter() dragging parameter doesn't only specify whether or not to trigger movestart/end events. The doc is a bit misleading with respect to that. See <http://trac.openlayers.org/ticket/967>. -- Eric

So, maybe my patch is fine without modification then? When the drag is finished, the dragging argument would be false. Otherwise, it will be true. Or maybe the last call to map.setCenter should be called separately, and not be "bundled" with some of the drag-events? That would not be too hard to implement either.

In discussion at our mini-sprint in NYC, we decided that although this may be a sane change to make, it's not something that we want to make right before a release, but instead right after: bumping to 2.7 for now.