Middle button to swap between move and previously selected tool

[zindy]

As agreed in the forum.sc post the following combinations of key / mouse button presses will need checking to make sure the proposed changes won't affect existing functionality in QuPath:

• For all operating systems (Windows, Linux [or at least Ubuntu], macOS)
  • For both mouse and trackpad
    • For all relevant combinations under View → Multitouch gestures
      • ‘Normal’ behavior for zoom/pan etc.
      • Behavior with shortcut keys pressed e.g.
        • Scroll + Ctrl/Cmd – should adjust opacity
        • Scroll + Shift – can switch pan to zoom (depending upon prefs)
      • Behavior with the ‘Invert scrolling’ option selected

[petebankhead]

Thanks @zindy I'll try to have a look later.

First thought is that I'd rather not have the second commit, unless you have a clear need for a public getPreviouslySelectedTool() method?

In general, I'd rather shrink the API and make it more stable, and avoid adding public methods just-in-case. But it could be added if it is really needed somewhere.

[zindy]

I totally understand, this does look like a "just-in-case" method. I was testing gestures with the jpen extension and thought that with previousTool being private, there wasn't a mechanism to swap tools (I mean from move to previously selected) from anywhere else than within the QuPathGUI class.

I'll revert the change.

[zindy]

I tested some old mice (that got hidden away for a reason), and their wheel button triggered multiple events at times. This behaviour was corrected with a 10ms debounce.

Am I OK to add a (if you're happy with the variable name) :

	private long lastMousePressedWheel = 0L;

and then, the e.isMiddleButtonDown() event would be handled as such:

			else if (e.isMiddleButtonDown()) {
				//For 10ms after the first click, ignore all additional events
				long time = System.currentTimeMillis();
				if (time - lastMousePressedWheel < 10)
					return;

				// Here we toggle between the MOVE tool and any previously selected tool
				if (getSelectedTool() == PathTools.MOVE)
					setSelectedTool(previousTool);
				else
					setSelectedTool(PathTools.MOVE);
				lastMousePressedWheel = time;
			}

I'm hoping to reuse the same variable to debounce the horizontal scroll buttons as well, hence the name.

[petebankhead]

Before doing that, could you check if there's anything is in https://openjfx.io/javadoc/11/javafx.graphics/javafx/scene/input/MouseEvent.html that could help?

I think the issue may be because there's no test for the event type in your current code - and so it could be fired on a mouse move event as well (with the middle button down). So I think you need to check for a pressed, released or click event (I forget which... this is a place were cross-platform stuff can be annoying).

I've never used isStillSincePressed() because I don't discovered it recently, but perhaps it can also help.

[zindy]

Hi Pete,

in retrospect, I shouldn't have added the middle button code to one of your existing events. Instead, I gave the code its own viewer.getView().addEventHandler(MouseEvent.MOUSE_PRESSED, e -> {}) event, which hopefully makes it easier to follow. This has also solved my debouncing issues, it seems. I actually had code that logged whether a bounce was detected within 10ms (as detailed above) but never saw such an event.

Interestingly, and this is code I left commented out in my commit, checking for isStillSincePressed() as you suggested led to missed clicks. That, or maybe my logic is flawed:

			if (e.isMiddleButtonDown()) {
				/* 
				if (!e.isStillSincePress() ) {
					logger.warn("The mouse moved! {}", System.currentTimeMillis());
					return;
				}
				*/
				...
			}

However, moving the mouse while pressing the middle button doesn't have any adverse effect.

Cheers,
Egor

[petebankhead]

Thanks, I'll need to find time to explore this in more detail - it probably reveals an ugliness in how QuPath handles viewer interactions.

I worry a bit about adding new event handlers, because it can become confusing which is called and when. Note that the move tool (as with other tools) defines a mousePressed(MouseEvent) method. Conceivably, this or some other tool might do something with a middle button press. If so, then it might be hard to predict which method will actually be called. It's quite possible that both would be called.

One way to ensure that one method is called before another EventHandler is to use an EventFilter... although adding multiple event filters would presumably lead to the same kind of confusion regarding their order.

For global application behavior that doesn't need to be customized, then I think it's best to include the logic in a single EventHandler or EventFilter rather than adding multiple ones. Having both an EventFilter and an EventHandler is fine because then we know the filter will be called first, but having more than one of either of them attached to a UI component is where the confusion starts.

For that reason, my guess (without looking in detail!) is that the tool toggling should be implemented using an EventFilter attached to the scene (not viewer, because it's global to the application), somewhere around here. Where exactly would depend upon whether the middle click should switch the tool when the UI is blocked or not. Either way, you should probably make sure to consume the event after it has performed the switch.

However I'm not sure... since I reached this conclusion by thinking about it rather than testing anything.

[zindy]

Last code reshuffle! I'm back to where I started and exactly where you wrote the middle button code should go, somewhere around here. In this iteration, if you have time to test it, I've also added the side-to-side wheel clicks (or shift + normal mousewheel scrolling).

I've spent some time trying to understand how Mouse Events are handled in JavaFX and I think I managed to sort out the issues I was initially having: Behaviour when dragging with middle button pressed, fast clicks, when to consume events or not, debouncing... Incidentally, the debouncing code is not strictly necessary, but it was an interesting exercise in working with a broken mouse.

Also, I think MouseEvent.MOUSE_RELEASED should be added to the list of ignored events here as without, I had to deal with the (middle) button release event multiple times as part of the events generated by JavaFX in the course of pressing a mouse button.

Finally, I've also UX tested the behaviours we discussed in the forum.sc post and reproduced in the first comment. For now,

• Windows,
  • with my "fancy" mouse
  • with the shift+scrollwheel substitute for side-to-side scrolling
  • my Lenovo T460 touchpad
  • with scroll touch gestures enabled or disabled
  • with inverted mouse enabled or disabled
  • with ctrl to modify opacity

I couldn't use "Zoom" or "Rotate" touch gestures, because I don't think my very basic touchpad understands them, so I couldn't see any effects on QuPath. Panning does work as expected (with my very last commit).

todo I will edit my comment when I test a Linux build on my old powerbook with Mint, which (maybe) will let me use more touch gestures. I haven't been able to use touch gestures with QuPath in my Cinnamon desktop environment so far, but with a mouse, my code behaves in Mint/Ubuntu the way it does on Windows.

For now, my code is quite verbose, both in terms of comments and in debug messages. I will clean this up if/when you're happy with this pull request.

[zindy]

Hi Pete,

there's no much else I can do on this for now. It's ready to be tested by someone else in case I missed anything, and I don't have a mac to test on so I just can't guarantee this will work on all platforms. Also, the code is probably over-annotated and over-verbose on the console side, but that's just so that you can check what's going on in case there are issues I didn't notice myself. Once okayed, the code can be both toned down and cleaned up :-)

If this is too much to test before the new release, we can reconsider this change for later. In fact, for this to be complete, I would also need to document my change (wouldn't be fair to let you do it if I wrote the feature) and add a middle button to your "input viewer".

Cheers,
Egor

[petebankhead]

Hi Egor, I was meaning to get to this - but ended up swamped by so many huge things to sort before the next release, and this one requires switching to an old computer with a small screen and no battery life... which I never quite found time to do.

I've just merged in the >200 commits since this. If it builds ok I'll try to have a look in the next day or two.

[zindy]

I've just merged in the >200 commits since this. If it builds ok I'll try to have a look in the next day or two.

I saw, that's a crazy amount! Anything I can do to help, and don't let this stop you from doing anything more important, that's all I'm saying :-)

Cheers,
Egor

[petebankhead]

Ok, had a quick check on my Mac since you mentioned it - and I'm afraid the scroll isn't working very nicely :( The 'magic mouse' makes horizontal scrolling really easy - which is handy for panning around the image if you turn View → Multi-touch gestures → Scroll gestures. However here it means that tools switch really easily... and continually happen by accident during normal use.

How useful is this aspect of the PR? Would you miss it badly if that part was removed...?

I think (but haven't thoroughly checked) that the middle button should be less troublesome, partly because Mac mice don't have middle buttons. But I remain a bit apprehensive about needing a debounce delay, and worry about adding even more complexity to the various event filters and handlers that are active when interacting with the viewer.

Can you say a bit more about how useful you've found this, and when?

The option remains to add the functionality through an extension or startup script, to give it a bit more time before possibly integrating it into the core software later.

[zindy]

Hi Pete,

Ok, had a quick check on my Mac since you mentioned it - and I'm afraid the scroll isn't working very nicely :( The 'magic mouse' makes horizontal scrolling really easy - which is handy for panning around the image if you turn View → Multi-touch gestures → Scroll gestures. However here it means that tools switch really easily... and continually happen by accident during normal use.

So, maybe this is because I use QuPath in Windows, usually with a mouse rather than a touchpad. Since I don't (know how to) use many Multi-touch gestures, I thought it would be enough to only enable my tool selector when "use scroll gestures" is disabled. I just checked that this is still the case and you don't even need to close the preference window to test that.

How useful is this aspect of the PR? Would you miss it badly if that part was removed...?

Not super important, I put it there for my convenience as I don't scroll the image side to side (I use the move tool) and prefer using the mouse wheel for zooming.

I think (but haven't thoroughly checked) that the middle button should be less troublesome, partly because Mac mice don't have middle buttons.

Of the two proposed changes (side to side tool selection, middle button to come back to the previously selected tool), the middle button is the one I would say is the more useful one.

But I remain a bit apprehensive about needing a debounce delay, and worry about adding even more complexity to the various event filters and handlers that are active when interacting with the viewer.

I agree that debounce should really be part of the OS. QuPath shouldn't be blamed for not working quite right with a broken mouse.

Can you say a bit more about how useful you've found this, and when?

• First the one I found most useful: For me, this feature really shines when I need to draw lots of small regions for a pixel classifier. I draw either rectangles or use the polygon tool and yes, I could just try and remember that "m" is for move, "r" is for rectangle and "p" is for polygon. From my testing, I just find using the middle button more practical.
• Side-to-side selection: That came as an afterthought after implementing the middle button. I then use it because it's there, but honestly, the keyboard shortcuts do make sense.
• Debounce: Like I said, it helps with worn out buttons, but this isn't normally something anyone will have issues with. I'll keep it at the back of my mind and let you know if there is a real test case for including it in QuPath.

The option remains to add the functionality through an extension or startup script, to give it a bit more time before possibly integrating it into the core software later.

I'm all for this. What I can do now is scale back the PR to "middle button functionality" (no side-to-side or debounce), and also remove any logging that isn't strictly necessary.

Cheers,
Egor

[zindy]

OK, the code left is just the middle button, without side-to-side tool selection and without the debounce code.

One comment I left in is this:

// As part of MouseEvent.ANY, both MOUSE_RELEASED and MOUSE_PRESSED can be generated (and detected) separately, so maybe worth adding MOUSE_RELEASED to ignoreTypes?

For me, pressing the middle button quickly is what generated extra MOUSE_RELEASED events, which would then need to be filtered somewhere within

		stage.getScene().addEventFilter(MouseEvent.ANY, e -> {
		}

Instead, I added MouseEvent.MOUSE_RELEASED to your ignoreTypes but left the original line commented out.

If this is OK, I'll remove the line and amend my comment to mention MouseEvent.MOUSE_RELEASED is to do with pressing the middle button multiple times.

[petebankhead]

It's a long time since I wrote that bit (and I probably should have used Set.of(...) instead of the HashSet...), but I'm not sure that adding MOUSE_RELEASED would be correct.

The purpose of the EventFilter in general is to block UI events under some circumstances, e.g. when a script is running. We'd want mouse pressed & released events to be blocked (and not ignored... since ignoring them would let them through. I realise that's not entirely intuitive naming...).

To make minimal changes I think you'd just need to check for the event you want and leave the rest as it was, e.g.

  ...
} else if (e.getEventType() == MouseEvent.MOUSE_CLICKED && e.getButton() == MouseButton.MIDDLE) {
  ...
}

(Not certain I've understood, since that comment is only from reading - not running)

[zindy]

OK, it turns out that adding MOUSE_RELEASED to ignoreTypes is not needed (any more).

At some point, I made the logic of the middle button robust to fast clicks. I suspect that consuming the extra events generated by subsequent middle click events (e.getClickCount() > 1) is enough.

Then all I do is check is e.isMiddleButtonDown() and I don't see anything weird happening whether I do rapid middle button clicks or if I press the left button or use the mouse wheel while the middle button is pressed.

[petebankhead]

I've tried this now and agree the middle click behavior can be really nice and intuitive once you get used to it (and if you have a suitable mouse...).

Could you check out the comments please and let me know what you think. I feel the logic in the handler is still very involved, and my simplified version appears to work perfectly for me - but realise it may be mouse-specific.

[zindy]

As far as I've tested this, it's all working as expected, with the added benefit of letting the user manually click the move tool and still retain which other tool was previously selected (as you suggested).

Fast clicks not an issue any more, even with the shorter code you suggested.

All reflected in the last (cleaned-up) commit.

[petebankhead]

Merged now, thanks!