DragPan.panMap should not always trigger setCenter

patch file

Any reason not to merge panMap and panMapDone into one function, in that case?

Tim, can you take a look at this one when you get a chance? I'll do the legwork to add tests, but I'd like your feedback on it.

modify Drag handler and DragPan control

I agree things need changing here.

I don't like how the DragPan control sets the start property of the Drag handler. Also, I agree that the panMapDone code is redundant.

With the dragging.patch, the drag handler sets a last property - and updates it appropriately. It also sets its own dragging flag to false when it is done dragging. It also doesn't call the "move" callback unless evt.xy is actually different than last. This is better dealt with inside the handler.

Then, the DragPan control only relies on the handler.dragging flag. If true, it's not done. If false, it's done.

I've written tests for all modifications I made. Current tests for the DragPan control are weak. If more can be written and added as a separate patch to this ticket, that would be great. If no more tests make it in, I'll set this one for review.

I agree with the changes to the handler. It moves more of the logic out of the control and into the handler where it belongs. I've played with this relatively thoroughly. I've gone ahead and added some DragPan tests.

    function test_Control_DragPan_no_drag (t) {
        t.plan(3);
        
        var data = init_map();
        map = data[0]; control = data[1];
        
        count = 0;
        results = [false, true, false];
        map.setCenter = function(arg1, arg2, arg3) {
             t.eq(results[count], arg3, "dragging arg set correctly");
             count++;
        }
        map.events.triggerEvent('mousedown', {'type':'mousedown', 'xy':new OpenLayers.Pixel(0,0), 'which':1});
        map.events.triggerEvent('mousemove', {'type':'mousemove', 'xy':new OpenLayers.Pixel(0,0), 'which':1});
        map.events.triggerEvent('mouseup', {'type':'mouseup', 'xy':new OpenLayers.Pixel(0,0), 'which':1});
        map.events.triggerEvent('mousedown', {'type':'mousedown', 'xy':new OpenLayers.Pixel(0,0), 'which':1});
        map.events.triggerEvent('mousemove', {'type':'mousemove', 'xy':new OpenLayers.Pixel(1,1), 'which':1});
        map.events.triggerEvent('mouseup', {'type':'mouseup', 'xy':new OpenLayers.Pixel(0,0), 'which':1});
    }

This is a test which will fail before the patch, and succeed afterwards: specifically, setCenter should only be called on down/up or an actual move.

With the tests in, this is fine to commit.

in w/ r3891 - thanks for the review and additional tests

As always, more tests would be great. I've covered the drag handler adequately - the DragPan control is lacking in tests. Tim [1] http://trac.openlayers.org/attachment/ticket/774/dragging.patch

I like Tim's patch, but I don't think it addresses this particular ticket. If the user just clicks on the map (without moving), panMap() will call setCenter() (verified on [*]). As I've mentioned on the mailing list, I think setCenter() should not be called in that case. In particular, setCenter() will trigger "moveend" while nothing has moved at all, which doesn't make sense.

What do you think?

[*] http://www.openlayers.org/dev/examples/lite.html

Eric:

I agree that it does currently call setCenter. In fact, the tests are even designed to ensure this. The reason, however, is that:

• when a user clicks down, we want to fire a movestart, because we b elieve they are starting to move.
• If a user does not move and lets go of the mouse, we want to fire a moveend, because there has already been a movestart event, and we want to have those events always match.

It seems like you think this isn't important -- perhaps the contract for the events fired by setCenter could be changed without negatively affecting things, firing setCenter with movestart at the first drag movement.

Thoughts?

don't call setCenter on click

See what you think of the dragPan patch. map.setCenter is only called if the mouse moves. New tests added to ensure this. All tests pass in IE and FF.

Replying to tschaub:

See what you think of the dragPan patch. map.setCenter is only called if the mouse moves. New tests added to ensure this. All tests pass in IE and FF.

Great!

One small comment: couldn't panMap use "true" instead of "this.handler.dragging" as the 3rd argument of setCenter? panMap, as a move handler, knows for sure the map is dragging.

And by the way, couldn't we just get rid of the "dragging" property in Handler/Drag.js? If what I propose above is deemed acceptable, that property will be used at a single place: the click handler in Handler/Drag.js. The mouseup and click handlers could probably be slightly rewritten so that only the "started" property is needed:

mouseup: function (evt) {
    if (this.started) {
        // the click handler will take care of setting
        // "started" to false
 	
        // TBD replace with CSS classes
        this.map.div.style.cursor = "";
        this.callback("up", [evt.xy]);
        this.callback("done", [evt.xy]);
        document.onselectstart = this.oldOnselectstart;
    }
    return true;
},

click: function (evt) {
    // throw away the first left click event that happens after a mouse up
    if (this.started) {
        this.started = false;
        return false;
    }
    return true;
},

Thoughts?

Eric

Replying to elemoine:

One small comment: couldn't panMap use "true" instead of "this.handler.dragging" as the 3rd argument of setCenter? panMap, as a move handler, knows for sure the map is dragging.

The third argument to setCenter is the dragging argument. If it is set to true, movestart and moveend never get triggered by the map. When it is set to false - both movestart and moveend get triggered (it was never the case that movestart got triggered at the start of panning and moveend at the end of panning - both get triggered on the last call to setCenter).

In any case, yes, there is much more that could be changed. Lets focus on the things that are broken.

Replying to tschaub:

Replying to elemoine:

One small comment: couldn't panMap use "true" instead of "this.handler.dragging" as the 3rd argument of setCenter? panMap, as a move handler, knows for sure the map is dragging.

The third argument to setCenter is the dragging argument. If it is set to true, movestart and moveend never get triggered by the map. When it is set to false - both movestart and moveend get triggered (it was never the case that movestart got triggered at the start of panning and moveend at the end of panning - both get triggered on the last call to setCenter).

I'm just saying that handler.dragging will always be true when panMap is called. panMap knows the map is currently dragging so it can just pass true as the 3rd argument to setCenter. This would make de DragPan control less dependant from the Drag Handler. In particular, should we want to remove the dragging property in the Drag handler, we can do so without touching the DrapPan control.

handler.dragging is false the last time panMap is called.

Replying to tschaub:

handler.dragging is false the last time panMap is called.

Oh yes, it's called by panMapDone. Thanks for taking the time to explain this.

Eric:

You're more comfortable with this issue. Can you review this change to ensure that it works in all cases? Once you're done, you can mark the ticket for commit for Tim -- I'm comfortable with the patch, but want confirmation that you are as well.

Thanks for the review. Please suggest any other changes to the drag handler or controls as they come up. There is certainly room for improvement.

In w/ r3902.