Simplify moveHandler (#186)

blms commented 7 months ago

Hi @rsimon! Wanted to pass along a fix for #186 working locally in my application. This PR likely needs more careful attention than my other ones since it changes a core drawing functionality and might not work outside of my context.

Notes:

I'm only testing this with the polygon and rectangle tool in a specific application where we've disabled clicking on the canvas after you finish a drawing (until you save or cancel it), so it's possible this is broken in any other context.
I wasn't certain what the purpose of firstDragDone was, but I needed to remove the one check for it to get this working. Maybe that breaks something else!
Like I mentioned in #186, evt.buttons seems to always be 1 whenever I'm able to fire this event.
Though none of the Annotorious 2.x tools have dragstart or dragend event handlers, I left them in just in case it's meant for other tool implementations.

rsimon commented 7 months ago

Thanks! I'll test this. But at first glance this looks worryingly similar to this one: https://github.com/annotorious/annotorious-openseadragon/pull/165 I.e. I fear it might break in other ways.

(Event handling in Annotorious is a pile of Spagetti, frankly. Probably won't be able to fix this anymore, since v3 is almost ready to go.)

blms commented 7 months ago

@rsimon Thanks, I didn't see that one! I do think removing the check for evt.buttons entirely fixes the problem you noted in the comments, but it's entirely possible I'm missing other ways this breaks.

Do you remember what the original thinking was behind firstDragDone?

(Hoping I can fix this for the Princeton Geniza Project without needing to upgrade them to v3 yet, but no worries if that ends up being the case!)

rsimon commented 7 months ago

That firstDragDone thing... I remember removing it multiple times - and then later re-adding it back in when I realized what it did :-D

Once again, I don't remember off the top of my head. But I believe it's only relevant when using the polygon tool. (I really need to write a code comment the next time I realize what breaks when removing it...)

blms commented 7 months ago

@rsimon Thanks to Git Blame, I was able to find the original commit that added this: https://github.com/annotorious/annotorious-openseadragon/commit/cb5b9e5feb7594486785e7486e38e154e334ec08

It looks like that's intended to add the one behavior that we don't allow in Geniza, which is dragging to pan the canvas around while also in the middle of drawing. I also found the release notes that included this change; not sure if it's related to the crosshair bug mentioned there, but I would presume that's mostly referring to 5cb72b87d3aa633fd14f92a90ad311fad3cb5d5a.

Going to try testing this further by disabling that part of geniza and allowing clicks outside the drawing, and see what happens…

blms commented 7 months ago

This was the only difference I was able to observe. First is the examples page, second is with my implementation. I'm wondering if the original behavior is intended? To me the new behavior makes more sense, but maybe there was a particular use case for this.

https://github.com/annotorious/annotorious-openseadragon/assets/7234006/6d51a597-a137-40ee-b961-e15376c0d44f

https://github.com/annotorious/annotorious-openseadragon/assets/7234006/04102f24-aa4f-4aac-aeeb-f11c3bf87f8a

rsimon commented 7 months ago

Ah, of course. Yes, that was the point I introduced the firstDragDone flag. (It's unrelated to the crosshair issue, though.)

The use case for this was deliberate. Users were tracing large polygons. In order to annotate precisely, the wanted to zoom so much that the polygon exceeded the viewer area. In other words: they wanted to start annotating, but still be able to shift the view while they were drawing. IMO this makes sense, otherwise you have no way of annotating things larger than your viewing area.

FWIW: Annotorious 3 will have two different drawing modes you can choose from. A drag mode will lock the image and start drawing on drag. A click mode will work mostly like the above, with drawing starting with a single click, and the image remaining moveable by mouse drag.

blms commented 7 months ago

Thanks, @rsimon! That makes a lot of sense.

I think I've reintroduced the correct behavior with firstDragDone. It seems the keys to solving the original bug were the following:

firstDragDone was being set true on every mouse release, rather than only after a shape had started to be drawn. So it was set true when shift-clicking on the canvas once without dragging, and because the tool complete event wasn't fired, it never got set back to false.
When firstDragDone was true, the tool's onMouseMove and the startSelection event were not firing. As far as I can tell, the only actual behavior we want to disable is when firstDragDone is false, the event should not propagate so that the panning does not happen during the initial line drawing.

rsimon commented 7 months ago

Wow - yes, I think this did the trick. Thanks for figuring out this puzzle!

rsimon commented 7 months ago

I'll try to roll a new release tomorrow, in case you need this fix urgently?

blms commented 7 months ago

@rsimon That would be great, thank you so much!

rsimon commented 7 months ago

Done - new release is out!

annotorious / annotorious-openseadragon

Simplify moveHandler (#186) #190