Page MenuHome

UI Code Quality: Use derived structs for search buttons and decorators
ClosedPublic

Authored by Julian Eisel (Severin) on May 3 2020, 10:59 PM.

Details

Summary

Check the changes to structs in interface_handlers.c for a practical overview
of what this is about.

The current on-size-fits-all uiBut creates quite a mess, where it's hard to
reason about which members are free for use, under which conditions they are
used and how. This tries to untangle that mess.
A draw-back is that many casts have to be done although this seems reasonable.

(I don't expect an in-depth review, but would like to have the general design
change reviewed first.)

A complication was that we sometimes change the button type after it's created.
So I had to add logic to reallocate the button for use with the new, possibly
derived struct. Ideally that wouldn't be needed, but for now that's what we have.
This is also something I'd like to have reviewed.

Part of T74432.

Diff Detail

Repository
rB Blender

Event Timeline

Julian Eisel (Severin) requested review of this revision.May 3 2020, 10:59 PM
source/blender/editors/include/UI_interface.h
542

I can just ged rid of this macro and do the checks in place, don't mind really.

source/blender/editors/interface/interface.c
3904

Should do a NULL-check for but->layout in case the button isn't inside any layout.

  • Remove unnecessary uiButSearchData struct
  • Store search item in search-button data, don't reuse general void pointer
  • Fix missing NULL checks for buttons without layout
Julian Eisel (Severin) edited the summary of this revision. (Show Details)Jun 7 2020, 11:51 PM

I think this is a step in the right direction, and I couldn't spot bugs reading through the changes.

This revision is now accepted and ready to land.Jun 8 2020, 2:59 PM
  • Merge branch 'master' into temp-ui-button-type-refactor
Campbell Barton (campbellbarton) accepted this revision.EditedJul 6 2020, 1:57 PM

It's unfortunate that the search buttons need re-allocating, we could try avoid this button conversion in a separate refactor though.

Otherwise code seems fine, and agree we should move to typed buttons, LGTM.