make drag handler easier to subclass

prepare the drag handler for subclassing

As incentive to get this reviewed, I've *started* writing tests for the handlers - as in, there haven't been any since these went in to the trunk (see r3685, r3688, r3689, r3690).

If this is deemed worthy, any help with the tests would be appreciated. I'm running out of steam.

And, the example mentioned above (of subclassing the drag handler) is the RegularPolygon handler: http://dev.openlayers.org/sandbox/tschaub/feature/examples/regular-polygons.html

patch looks ok but I would put the meat of the mousedown() in down() etc etc. I think it would be easier to subclass that way.

make drag handler easier to subclass

ok, updated patch reflects changes in the trunk.

I'll need to hear more detail on how putting "the meat of mousedown in down etc" would make things easier.

If this is not of any interest or nobody has any better ideas, I'll make other handlers that have (instead of inherit from a) drag handler (as with the box handler).

I'd like to get something figured out in order to get rid of this feature sandbox.

Thanks for any review.

So, Erik and I talked about this live:

10:09:56 AM euzuro: to me 10:10:01 AM euzuro: instead of this new up() function 10:10:11 AM euzuro: you just override mousdown: function() 10:10:18 AM euzuro: and do whatever you want 10:10:23 AM euzuro: then if you want the base functionality 10:10:25 AM euzuro: you say 10:10:51 AM euzuro: OpenLayers.Handler.Drag.prototype.mousedown(this, arguments); 10:11:06 AM euzuro: if you *dont* want to do the bse functionality... you *dont* call that.

I'm not sure I understand why we wouldn't do it that way. I think I heard Tim say he doesn't like the prototype.apply stuff, so I offered that, and Erik pointed out:

10:11:42 AM euzuro: Right but we're not going to change all that *now* 10:11:47 AM euzuro: that's how the whole code base works 10:11:57 AM euzuro: to me, this patch is introducing four new functions that are unnecessary. And i think that if one is used to the OL code 10:12:28 AM euzuro: -- which anyone who is in at the level of hacking around with the handler code they *better* be familiar with it ;-) -- 10:12:46 AM euzuro: ...then they should just follow through with it.

This leads to the conclusion that if we want to change the general way subclassing works, we can examine that -- but changing it in one place is not okay. We need to do a real evalutation of how we want things to work and then change it everywhere if that's what we want to do.

Against that path, I offer that this behavior is more constraining: If I'm subclassing drag and I want to run the main drag stuff, and do stuff both before and after it, I can't do that. The prototype.apply stuff lets me put it where it makes sense -- but with this down() stuff, it' always running 'my' code before the drag handler's, and I can't change that (without subclassing the mousedown) -- and if I have to subclass anyway, then I'm not gaining anything.

So, unless I'm missing something, I think that we should instead stick, for the time being, with the current behavior, which uses prototype.apply if we need it.

so cr5 more or less summed up (err, quoted) my sentiments above. the bottom line is that in my opinion, this patch represents a move towards a different style of doing subclassing and messaging between subclasses.

As I mentioned to cr5, we are completely open to a reevaluation of the way things work in OL, but it would have to be a universal analysis / evaluation of techniques... not just in one corner of the code.

I think one of the strong points of the OpenLayers codebase is that, even if it's at the price of efficiency in certain points, it is consistent. I think that helps people feel comfortable with it, moving around in different parts of the code.

the rewrite of the activate/deactivate code that was in there as part of this patch I think is good. i've reenacted it here... if others like this, we should put it in. also, if we want to move those functions up to the beginning of the file, we can do that too (without making a ticket)

Okay, Tim and I talked, and I now understand the goal here:

There is some logic to say "should this action be called?" That logic should not be duplicated. So, the 'guts' of the inside of that function should move out to a seperate function -- the four names in this patch seem fine -- and be called from within, for example, mousemove.

Then, a subclass can subclass 'move' -- which doesn't have any event handling logic in it -- once it has determined that a move action should be taken. And subclasses can simply override 'move' instead of mousemove.

This avoids the return true/false alternative to the standard inheritance prototype.apply stuff, but also prevents duplication of the event handling code.

I think Tim is going to spend some time working on that route in a bit, otherwise I can take it on.

Replying to crschmidt:

Okay, Tim and I talked, and I now understand the goal here: There is some logic to say "should this action be called?" That logic should not be duplicated. So, the 'guts' of the inside of that function should move out to a seperate function -- the four names in this patch seem fine -- and be called from within, for example, mousemove. Then, a subclass can subclass 'move' -- which doesn't have any event handling logic in it -- once it has determined that a move action should be taken. And subclasses can simply override 'move' instead of mousemove. This avoids the return true/false alternative to the standard inheritance prototype.apply stuff, but also prevents duplication of the event handling code. I think Tim is going to spend some time working on that route in a bit, otherwise I can take it on.

Hmm... i'm still not quite understanding this. If anyone can explain this to me on IM or another comment, I would appreciate.

H'okay! So. Here's the earth. Chillin. Damn, you might... wait, distracted.

So, talked with Erik. Here's what we came to:

• The drag handler currently handles mousemove, etc. for you. Great.
• When subclassing the Drag handler, what you actually want is to be able to override the *drag* action, not the mousemove action. So, we need a new function call -- dragup, dragdown, dragmove, what have you.
  • In the base handler class *these do nothing*
• If there is a handler (a la RegularPolygon) that needs to override these, it can override just the drag methods.
• If there is a handler that really needs to *not execute the boilerplate* in the mousemove function, then it should be explicitly overridding it: not returning true is not a strong enough agreement to the contract of "If you change this, you are turning this into something other than a drag handler unless you're maintaining state yourself."

So!

drag.2.patch is close, but instead of the conditional on the return from the handlers, it can be just a single call to the function. Then, anyone who wants to not have the boilerplate handler mousemove, etc. can just override those functions on their handlers. I can't understand a reason you'd want to subclass the Drag Handler and *not* have the boilerplate -- that's what the Handler is *all about* after all -- so it is possible that in the future the conditional will come back, but at the moment, it seems like if you want to change the boilerplate behavior, you want to be overriding the class anyway.

I'll cook up a new patch for that change later today.

Tim -- Thanks for pushing back on this. Sorry that Erik and I can be so dense :)

submethods

Methods for the subs and tests to show they get called.

Please review.

I should add, tests pass in FF/IE (really, IE too). And the regular poly example works, and OL still works.

excellent! everyone wins!

sometimes its good to toss these things around 4or a bit. we come up with a best solution.

please commit

(In [4096]) Modify drag handler for easier subclassing. Thanks for the team effort on this one (closes #827).