UI Code Quality: Cleanup ui_but_update_from_old_block
ClosedPublic
Actions

Authored by Hans Goudey (HooglyBoogly) on Oct 6 2020, 12:37 AM.

Details

Reviewers

Julian Eisel (Severin)
Campbell Barton (campbellbarton)

Group Reviewers

User Interface

Commits

rB1d9499bbbc2c: UI Code Quality: Cleanup ui_but_update_from_old_block

Summary

This patch contains some improvements to this function to hopefully make
the code more purposeful and readable.

Split updating oldbut information to a new function. This helps the code become more explicit.
Remove some 7 year old #if 0'd out code.
Return early in the "active" case.
Add comments explaining some of the less obvious (to me) aspects of the function.

This is a relatively low priority patch, and I would want to land it
early in a Bcon1 just in case, but IMO it would be a nice improvement,
especially because ui_but_update_old_active_from_new will continue
to get longer as more type-specific button stuff is added there.

Diff Detail

Repository: rB Blender

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Oct 6 2020, 12:37 AM

Hans Goudey (HooglyBoogly) created this revision.

Harbormaster completed remote builds in B10576: Diff 29614.Oct 6 2020, 12:37 AM

Hans Goudey (HooglyBoogly) added a reviewer: User Interface.Oct 6 2020, 12:38 AM

Hans Goudey (HooglyBoogly) added a project: User Interface.

This easily breaks all kinds of things, e.g. crashes when just hovering any templateID button, hovering a workspace tab, using properties search, ...

I'm not surprised, the swapping is very intentional. uiBut contains owning pointers, the ownership has to be moved out of the button to be freed. When the button is freed all these owning pointers are freed as well. If they still point to memory referenced by another button, well boom. So they have to be set to some different valid value.
Using swapping is the typical way of implementing C++ move-semantics too, see the copy-swap-idom.

As for the type specific logic - I expect that these will be at some point be moved into a uiButType struct, or eventually be replaced by C++ object methods. So I'm not too worried about that at this point.

Besides that, +1 for the general cleanups.

This revision now requires changes to proceed.Oct 6 2020, 2:09 PM

Thanks for looking at this. I meant to "plan changes" last night to do more testing. I guess I forgot though..

And thanks for the explanation about the swapping. I'll put that in a comment and remove those changes from the patch : )

Fix dummy mistake
Add to and improve new comments

Harbormaster completed remote builds in B10596: Diff 29658.Oct 6 2020, 10:04 PM

Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)Oct 7 2020, 6:08 AM

I like the changes here. Campbell has some concerns about the early-exiting, but I don't have any strong feelings about this. At least the other changes are for the better I think.

LGTM, one minor request.

source/blender/editors/interface/interface.c
915–929	I'd rather avoid using early returns when free functions are involved, it makes it harder to reason about changes when freeing. AFAICS this part can be left as-is.

This revision now requires changes to proceed.Oct 13 2020, 1:15 PM