Page MenuHome

Refactor boundbox handling of event handlers
ClosedPublic

Authored by Jacques Lucke (JacquesLucke) on Apr 18 2019, 2:47 PM.

Details

Summary

I originally started with this, because currently the boundbox
rectangle for the markers region is static, so that the pointer
can be passed along safely.
This does not work anymore, when we move the marker area to the top.
(think about the case when there are two marker area in separate areas).

There are multiple possible solutions.
I need some feedback to decide which way to go.

  1. Allocate a new rectangle for the event handler and free it, when the handler is freed. This also requires one or two new booleans in the wmEventHandler struct. Those store whether the rectangle has to be freed.
  2. Store the rectangles in the event handler by value. I'm not 100% if this actually works, or if it is important, that the rectangle memory is shared e.g. with some &ar->v2d.mask.
  3. Implement a more generic poll mechanism for event handlers. For that I'd store some function pointer and user_data in the event handler struct. Not sure if something like this could be useful for other things in Blender.

Diff Detail

Repository
rB Blender
Branch
refactor-event-handler-boundbox (branched from master)
Build Status
Buildable 3368
Build 3368: arc lint + arc unit

Event Timeline

I think the solution in this patch is fine, assuming the handler is recreated every time the region changes size. It seems to be the case as far as I can tell.

This revision is now accepted and ready to land.Apr 18 2019, 6:56 PM

I actually just talked with @Campbell Barton (campbellbarton) about this. We decided that it would be good to have this as poll function.

Ok, the previous mouse position logic in handler_boundbox_test is a bit obscure and not great to duplicate in many places, but that could be in a utility function.

New optional poll function for event handlers.
This replaces the old optional bounding box.

source/blender/editors/space_outliner/space_outliner.c
78 ↗(On Diff #14824)

I'm not sure if this special case is still needed.
If not then we can get rid of WM_event_add_keymap_handler_region. Not sure if it makes any sense to have it anyway.

source/blender/windowmanager/WM_api.h
211 ↗(On Diff #14824)

This function type is currently declared twice.
Where should I put it, when I want to declare it only once?

Jacques Lucke (JacquesLucke) requested review of this revision.Apr 23 2019, 10:48 AM
Brecht Van Lommel (brecht) added inline comments.
source/blender/editors/space_outliner/space_outliner.c
78 ↗(On Diff #14824)

I can't find anything that breaks when removing this special case. rBfa28e50ac2a7: Region scrollbar fix! probably fixed this, the comment and code here predates that.

source/blender/windowmanager/WM_api.h
211 ↗(On Diff #14824)

I can't find an obvious header to put it in, think we can just leave the declaration twice. If they mismatch we'll still get a compile error anyway.

This revision is now accepted and ready to land.Apr 24 2019, 5:20 PM