GraphGadget : add ctrl-click drag selection to remove nodes from sele…

[mattigruener]

Hey John,

here's the control-dragging feature I've mentioned in the meeting today. Thought I'd put this up so we can start talking about it.

Are you ok with this in general? The way I've implemented it now has the side-effect that if you release control while dragging, you essentially toggle this feature on or off. Maybe that's desired, maybe not. We could look into storing the SelectionMode when dragging begins instead of recomputing it in dragMove which I've done only for my own convenience and to keep the changes to a minimum for now.

Cheers,

Matti.

[mattigruener]

Not sure why the tests fail for our mac setup.

======================================================================
FAIL: testAutomaticLayout (GafferUITest.GraphEditorTest.GraphEditorTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/travis/build/GafferHQ/gaffer/install/gaffer-0.54.0.0-osx/python/GafferUITest/GraphEditorTest.py", line 128, in testAutomaticLayout
    self.assertEqual( graphEditor.graphGadget().unpositionedNodeGadgets(), [] )
AssertionError: Lists differ: [GafferUI.StandardNodeGadget( ... != []

First list contains 2 additional elements.
First extra element 0:
GafferUI.StandardNodeGadget( "Gadget" )

+ []
- [GafferUI.StandardNodeGadget( "Gadget" ),
-  GafferUI.StandardNodeGadget( "Gadget1" )]

----------------------------------------------------------------------

[johnhaddon]

Thanks Matti!

Are you ok with this in general?

Yes, being able to remove nodes from the selection seems useful, and Control feels pretty natural to me. A couple of other things we might consider :

• In the QTreeView, Control+click is used to toggle selection, but we are using Shift+click for that in the GraphGadget. Should we change to match? If so, what modifiers should we use for the existing "select downstream" behaviour?
• Should we update the Viewer selection to match whatever we do here? I think we probably should - drag deselection would be useful there for sure.

The way I've implemented it now has the side-effect that if you release control while dragging, you essentially toggle this feature on or off. Maybe that's desired, maybe not.

Personally I quite like that, but I don't feel strongly if someone would strongly prefer it the other way.

Not sure why the tests fail for our mac setup.

Unrelated to this - I've seen this failure on Mac before on Travis, but haven't got to the bottom of it yet.

Cheers...
John

[johnhaddon]

I'm tempted to say that SelectionMode isn't actually helping us here, because all it has really done is forced us to copy this event.modifiers ? ... logic to all the updateDragSelection() call sites. Perhaps we should just pass the modifiers instead?

[mattigruener]

Hey John,

In the QTreeView, Control+click is used to toggle selection, but we are using Shift+click for that in the GraphGadget. Should we change to match? If so, what modifiers should we use for the existing "select downstream" behaviour?

I think making behaviour match is generally a good idea. In SG13854 Chris also asks for toggling via drag-selection. I don't think he's entirely right about how the HierarchyView behaves (ctrl isn't deselecting, but toggling as you pointed out), but there seems to be an expectation of GraphEditor and HierarchyView behaving similarly.
Do we need to provide the "select downstream" feature via click? Can a user make a selection and then hit a hotkey to select downstream nodes? I assume behaviour would change a little because you'd now be able to do it for a bunch of nodes at the same time as opposed to for just the one you clicked on. Maybe that's ok? That would give us a little more freedom regarding which hotkey to pick. That feature has always felt a little esoteric to me and it feels like a waste of shortcut real estate to have it on +click ...

Should we update the Viewer selection to match whatever we do here? I think we probably should - drag deselection would be useful there for sure.

Definitely, people are actually asking for it in SG14612.

Personally I quite like that, but I don't feel strongly if someone would strongly prefer it the other way.

I like it too. Let's keep it that way until someone makes a compelling case against it.

Cheerio for now!

Matti.

[johnhaddon]

Do we need to provide the "select downstream" feature via click? That feature has always felt a little esoteric to me and it feels like a waste of shortcut real estate to have it on +click ...

I think the argument against is that Control+drag is a very quick gesture for moving all downstream nodes, to make space for inserting something above. Personally I don't use it enough to even remember what the shortcut is vs the upstream equivalent, but I'm slightly wary of possible "you broke my workflow" repercussions. We do have menu items in the Edit menu for all sorts of permutations of this though...

[mattigruener]

Just noting that when ctrl+dragging we can drag the node and downstream nodes in one go, for ctrl+alt+dragging that doesn't work. We should probably fix that unless we change all of this anyway.

[mattigruener]

I addressed the issue described in my previous comment in 99626d9. John, can you think of anything I'm breaking there?

Also had a look at the drag-deselecting feature for the SceneView. Could you have a look at a420db2?

Thanks! :)

[medubelko]

I haven't kept up closely with this. Will this require any changes to the shortcuts in the docs?

[johnhaddon]

Should be a bitwise & (the line above should be too, but that's fixed on master I think).

[mattigruener]

Arg, of course. Blindly copying is never a good idea. Thanks!

[mattigruener]

Michael, will need some changes, for sure. I guess John and me will have to spec out a few things a bit more, but we'll definitely need a new entry for ctrl+drag deselection of nodes. These might have to change as well depending on what we come up with:

• Select upstream nodes
• Select downstream nodes

As far as I can see, we currently don't have the equivalent selection entries in the Viewer section that would document drag selection and shift+drag selection. Not sure if we want to add them? If we do, we'll also need to add the new ctrl+drag deselection of objects.

[johnhaddon]

I addressed the issue described in my previous comment in 99626d9. John, can you think of anything I'm breaking there?

It seems to be along the right lines, but it looks like it's maybe inconsistent with the use of Shift+Alt drag for the variable aspect zoom feature? As in, those drags will now be offered to child gadgets first? Perhaps a little MotionType cameraMotionType( const ButtonEvent &event, bool *variableAspect ) utility would allow us to consolidate the repeated event.modifiers == ModifiableEvent::Alt || ( getVariableAspectZoom() && event.modifiers... tests into one spot so its harder to get wrong?

[johnhaddon]

As far as I can see, we currently don't have the equivalent selection entries in the Viewer section that would document drag selection and shift+drag selection. Not sure if we want to add them?

Might as well for completeness I think.

[medubelko]

Might as well for completeness I think.

I agree.

[mattigruener]

Another thing to discuss: 16df5b0

Are you ok with having that function in the CameraController? I like the separation of responsibilities where the ViewportGadget just asks the CameraController whether it can handle it. Maybe we should restructure things further so that in ViewportGadget::dragEnd and dragMove we don't need to handle the shift modifier so that all that responsibility is with the CameraController. I also like that it's a little more explicit now regarding how unmodified MMB drags are given to the children first.

[mattigruener]

@medubelko, I've added a first version of the docs in 09b02d6.

[johnhaddon]

Are you ok with having that function in the CameraController? I like the separation of responsibilities where the ViewportGadget just asks the CameraController whether it can handle it. Maybe we should restructure things further so that in ViewportGadget::dragEnd and dragMove we don't need to handle the shift modifier so that all that responsibility is with the CameraController.

The original separation of responsibilities was that CameraController knew how to perform camera movement, and ViewportGadgets knew how to handle events, and decided how to map those to camera movement requests. CameraController was originally a class in Cortex that had no connection to any GUI toolkit at all. This definitely breaks that original separation, but seems like it might be moving in an appropriate direction, given that Gaffer's event signal handling is meant to allow multiple components to collaborate around the same set of events.

Maybe we should restructure things further so that in ViewportGadget::dragEnd and dragMove we don't need to handle the shift modifier so that all that responsibility is with the CameraController.

I think full separation would mean that the CameraController made its own connections to buttonPressSignal/dragBeginSignal etc, "stealing" any events it wanted before they were processed by the ViewportGadget. I'm not sure how that would work with the drag tracking feature though - ideally the CameraController would deal with that too I guess. And the "unmodified MMB" drag thing might be a bit tricky - I guess we'd need two signal handlers, one that was called before the ViewportGadget and stole Alt+drag and one that was called after to take middle drag if it was still available.

I do feel like there might be some benefits to this in terms of clarity, especially if we want to introduce other camera control methods in the future (I've seen Quake-style controls advocated for navigating environments, with the argument being that the Maya-style controls kindof assume you spend most of your time orbiting a single central model). But I'm also wary of going down a rabbit-hole when we're just trying to improve selection behaviour. One for a chat?

I also like that it's a little more explicit now regarding how unmodified MMB drags are given to the children first.

Agreed. I vaguely recalled that something funky was being achieved in there before, but I couldn't remember what. Calling it out as you have definitely helps.

[mattigruener]

But I'm also wary of going down a rabbit-hole when we're just trying to improve selection behaviour. One for a chat?

For sure. Let's maybe talk about it tomorrow morning if that still works out for you. I'd also like to avoid falling too deep into this rabbit-hole as initially I just picked up this ticket to have something easy to do towards the end of the day and none of this has actually been prioritized. That being said, if we can make life easier for ourselves down the road by cleaning this up a bit more, I'm all for it.

[mattigruener]

@johnhaddon I believe this is now covering the things we talked about today. I opted for not spending a lot more time on separating the responsibilities further and put a comment/todo into ViewportGadget for future reference instead.

@medubelko I updated the docs to address your feedback and to add some additional bits. Wanna take another look? Feel free to put it off til we're ready to merge.

[medubelko]

Shortcuts LGTM. I'll remove the doc label.

[johnhaddon]

Could you resolve the SelectionTool.cpp conflict please Matti?

[mattigruener]

Done. Sorry, hadn't seen this before you mentioned it.

[andrewkaufman]

@mattigruener or @johnhaddon can you remind me the status on this one?

[mattigruener]

Afaik, my work is done for now and John is trying to find time for a review.

[johnhaddon]

This seems to have totally changed the behaviour of Shift+Click in the GraphEditor and Viewer. Before it would add to the selection, and now it clears the selection first. Hopefully this is an oversight and not intended behaviour? I don't see any reason to change it, or for Shift+Click to differ in meaning than Shift+Drag. I thought we were just trying to add useful Ctrl related behaviour.

For clarity, here's a commit that restores the behaviour I would expect, while keeping the nice new Ctrl behaviours. If we cherry-pick that, then LGTM.

[mattigruener]

Hey John :)

Thanks for your response!

This seems to have totally changed the behaviour of Shift+Click in the GraphEditor and Viewer. Before it would add to the selection, and now it clears the selection first. Hopefully this is an oversight and not intended behaviour?

It's been a little while so I'm a little rusty on the details here, but I actually think this is intentional as I also updated the documentation accordingly. I'll try to make sense of it below - apologies if I got the wrong end of the stick somewhere.

Just a slight correction so we talk about the same thing: Afaik previously Shift+Click toggled the selection of a node, it didn't only add to it.

I thought we were just trying to add useful Ctrl related behaviour.

I think that's part of it, but we're also trying to unify UX, aren't we? I'm quoting from our discussion above:

In the QTreeView, Control+click is used to toggle selection, but we are using Shift+click for that in the GraphGadget. Should we change to match?

I think that's what has happened here. I've switched the modifier for toggling which leaves Shift+Click essentially unused which makes it a normal click? That's what you're observing, isn't it?

I don't see any reason to change it, or for Shift+Click to differ in meaning than Shift+Drag.

It differed already, though? Shift+Drag was only adding nodes, and now we also have Ctrl+Drag to remove nodes. The SomeModifier+Click for toggling is somewhere in the middle of the two and it isn't inherently clear whether it should be Shift or Ctrl. We then resort to using Ctrl because that's what the rest of the UI is doing? Is that a fair summary?

Hope that makes sense? :)

Matti.

[johnhaddon]

In the QTreeView, Control+click is used to toggle selection, but we are using Shift+click for that in the GraphGadget. Should we change to match?

I think that's what has happened here. I've switched the modifier for toggling which leaves Shift+Click essentially unused which makes it a normal click? That's what you're observing, isn't it?

That is what I'm observing, yes, but it's not what I expect. Taking the QTreeView as reference, there a Shift+Click extends the selection, and I expect the same thing in the GraphEditor and the Viewer (albeit without the QTreeView's range selection behaviour, which doesn't make sense there).

I can't see any reason to have Shift+Click be the same as just Click. It goes against the universal conventions we were trying to align ourselves with, and everyone's established muscle memory. I can see how we got our wires crossed, but I can't see any reason not to uncross them...

[themissingcow]

I can't see any reason to have Shift+Click be the same as just Click. It goes against the universal conventions we were trying to align ourselves with, and everyone's established muscle memory. I can see how we got our wires crossed, but I can't see any reason not to uncross them...

+1 for having shift-click add to the selection and ctrl-click toggle the selection state of the click-ee.

[mattigruener]

Updated. I squashed in those changes instead of cherry-picking (3edf40f and
6a86878). Hope that's ok. Also added an additional entry to the docs about Shift+Click. @medubelko do you want to have a look at f95a44a?

I hope that uncrosses the wires :)

Thanks!

[johnhaddon]

Thanks Matti! I've merged - @medubelko, if you want to make any docs tweaks, just make a followup PR.