UI: Region "Hover" Refactor
Needs RevisionPublic
Actions

Authored by Harley Acheson (harley) on Mar 11 2020, 9:38 PM.

Details

Reviewers

Brecht Van Lommel (brecht)
William Reynish (billreynish)
Julian Eisel (Severin)

Summary

This is a bit of a refactoring for how we deal with mouse movement and hovering over regions. In order to support mouse hover in File Browser and Outliner we had to add new operators to just for that.

It is a bit weird having such operators bound to "Mouse Move" since they would not work with any other bindings. I also can't imagine having to make new operators for every place where we might want hover.

This patch specifically adds new callbacks for "mouse_enter", "mouse_leave" and "mouse_move" to ARegionType. Together they can be used for hover effects, or setting the mouse cursor based on special processing, etc.

After applying this patch and compiling, go into Preferences / Keymap and delete both operators bound to "Mouse Move":

Afterward the hovering in Outliner and File Browser will continue to work as usual.

Diff Detail

Repository: rB Blender

Event Timeline

Harley Acheson (harley) created this revision.Mar 11 2020, 9:38 PM

Whoops. fixing some warnings.

Indeed it’s super weird that the Outliner has operators just for rollover highlighting. That seems weak, and this is more appropriate.

@William Reynish (billreynish) - Indeed it’s super weird that the Outliner has operators just for rollover highlighting.

Yes, and then I've had people asking for rollover highlighting for Info and Animation channel lists, etc.

But it was also this, https://developer.blender.org/D6883, with a request to change the mouse cursor if over the strip drag handles. I can't think of a straightforward way to do that right now without adding something like this first.

Sending the full wmEvent to the mouse_move callback since it might be nice to know if a mouse or keyboard key is being pressed, or other details about the event.

Richard Antalik (ISS) added a subscriber: Richard Antalik (ISS).Mar 12 2020, 11:30 AM

William Reynish (billreynish) added a reviewer: Julian Eisel (Severin).Mar 13 2020, 6:08 PM

Also passing wmWindow to the callbacks as well. Not needed for these examples with File Browser or Outliner, but would be necessary if setting mouse cursor.

Generally I think it's fine to decouple this from operators. I'd still suggest thinking about some simple alternatives (I'm not against this patch, just good to consider the alternatives):

Rather than pre-defining specific handlers for region types, regions could register general region event handlers. Just like regions already register a UI handler for widget handling, see UI_region_handlers_add(). This would be done in the _init callback of a region. For that we'd need specific events for entering and leaving a region. E.g. EVT_REGION_CHANGE with an wmEvent.val of EVT_REGION_ENTER or EVT_REGION_LEAVE. In a way this makes sense because windows also communicate activation changes via events, why shouldn't our sub-windows as well?
Rather than having three different callbacks have 1 that gets a type passed.
Wrap callbacks into a sub-struct (e.g. ARegionType.immediate_handlers).

I'd love to hear @Brecht Van Lommel (brecht)'s opinion on this.

With the changes here, SpaceType.deactivate() isn't used anymore. I don't see a reason to keep it if we have these new region level handlers.

source/blender/editors/screen/screen_edit.c
730	`&& region->type->mouse_leave` is redundant and incorrect here: It is already called below and causes `SpaceType.deactivate()` to only be called if there's also a `ARegionType.mouse_leave()` set.
source/blender/editors/space_file/file_ops.c
1199–1238	The highlight operator is removed, so this code doesn't belong to an operator anymore and should be moved out of `file_ops.c`. Since this changes `FileSelectParams.highlight_file`, I think this should go to `filesel.c`.
source/blender/editors/space_file/space_file.c
689	`win` is unused in all these callbacks, which should give a compiler warning. Just use `UNUSED(win)`.

This revision now requires changes to proceed.Mar 26 2020, 12:48 PM

Just changing the name to make clear this isn't about widget level hovering (there's plenty of stuff to cleanup there too :) ).

In D7119#173192, @Julian Eisel (Severin) wrote:

Generally I think it's fine to decouple this from operators. I'd still suggest thinking about some simple alternatives (I'm not against this patch, just good to consider the alternatives):

Rather than pre-defining specific handlers for region types, regions could register general region event handlers. Just like regions already register a UI handler for widget handling, see UI_region_handlers_add(). This would be done in the _init callback of a region. For that we'd need specific events for entering and leaving a region. E.g. EVT_REGION_CHANGE with an wmEvent.val of EVT_REGION_ENTER or EVT_REGION_LEAVE. In a way this makes sense because windows also communicate activation changes via events, why shouldn't our sub-windows as well?

Rather than having three different callbacks have 1 (say focus_change) that gets a type passed (say FOCUS_CHANGE_ENTER, FOCUS_CHANGE_LEAVE, FOCUS_CHANGE_INNER_MOVEMENT).

Wrap callbacks into a sub-struct (e.g. ARegionType.immediate_handlers).

I would go with the second option. We want the region to get the callback immediately when it becomes active. If the mouse move events generates another event while it is being handled, things may get complicated?

If there is a clear picture of how this affects the event queue it may be ok to handle them as events. But we would have to be sure events are handled in the proper order, that they are properly blocked by modal operators but not blocked in other cases, etc. Safer is to just change as little as possible here though, not sure it's worth opening that can of worms with this being an internal API.

source/blender/editors/screen/screen_edit.c
689–691	`const int xy[2] = {event->x, event->y}`;

@Julian Eisel (Severin) - regions could register general region event handlers...

Event handlers would work for "enter" and "move" but not for "leave", as areas stop processing messages when they are no longer active. This was the reason for me adding that deactivate, to deal with items that remain hover highlighted after leaving an area.

With the changes here, SpaceType.deactivate() isn't used anymore.

Yes, for sure.

Rather than having three different callbacks have 1 (say focus_change) that gets a type passed...

That could work, but could seem a lot less logical. It would basically be a single "mouse move" event (and be called as often) even if you just want to know "leave". Being passed an "enter" type would just be informing that this was the first (possibly) so wouldn't serve much purpose. And being passed "leave" would be basically a mouse move outside the area, so would be an invalid "mouse move" event with a position outside the region. Would certainly need more complex comments than needed for "enter", "leave", and "move"

@Brecht Van Lommel (brecht) - Safer is to just change as little as possible here though, not sure it's worth opening that can of worms with this being an internal API.

Not doing this a good possibility too. I had originally thought of this as a nice way of getting hover for Info and animation channels, and maybe the mouse cursor change wanted for NLA. I didn't like the idea of adding more operators in the keymap for this. But I am starting a time of the year when I have less time to do this so might not get to these things myself soon.

stefano severi (Tetone) awarded a token.Mar 27 2020, 8:52 AM