Page MenuHome

Outliner: Selection cleanup
ClosedPublic

Authored by Nathan Craddock (natecraddock) on Sep 16 2019, 6:09 PM.

Details

Summary

Edit: I reworked the entire patch to have a smaller scope, and to be much simpler.

Motivation
A cleanup of the outliner selection functions is needed to fix T71812: Outliner: Active selection not in sync when navigating with arrow keys, and is also to allow other changes during my 2020 gsoc project. It would be great to have this in master rather than my soc-2020-outliner branch so the mentioned task (from 2.81) can be finished.

Changes
There are no functional changes. The main change is access to the do_outliner_item_activate_tree_element function, which used to have two entry functions. Now all access to the outliner selection/activation functions starts in outliner_item_select.

  • Update outliner_item_select to use a bit flag for selection types. This function can be used anywhere from the outliner code to select and activate elements. I chose to use a bit flag over 5-6 booleans because I think it makes the code generally cleaner.
  • Remove outliner-item_do_activate_from_tree_element in favor of outliner_item_select.
  • Move mode toggling logic to a separate function outliner_item_mode_toggle. This function is still called when activating object data, so no behavior has changed, but this will make it easier to remove from the selection code in the future when T68498: Outliner: Mode Toggling is implemented.
  • A few functions were renamed to make purposes more clear.

I'm fairly certain no behavior has changed because the same functions are still used for the actual selection and activation functionality, but additional testing would be nice to verify that. The only thing that might need updating is the naming of functions and enums.

Diff Detail

Repository
rB Blender
Branch
outliner-selection-refactor (branched from master)
Build Status
Buildable 6851
Build 6851: arc lint + arc unit

Event Timeline

source/blender/editors/space_outliner/outliner_select.c
1151

This is a wrapper around the functions to activate certain types of data, with some checks to ensure that cameras, collections, and scenes are not activated on selection.

1228

When the recursive argument of outliner_item_select is true, this is called to select the subtree. This is only used for the object select hierarchy context menu option, but could be expanded to other operators later.

1321

No longer needed as outliner_item_select can offer the same behavior

1378

outliner_item_select now contains a call to outliner_item_select_activate which is called when the activate (2nd) argument is true.

The refactor is welcome, I just looked at it with Campbell. And we agree with the motivation. But before we dive into the code itself could you please consider:

  • Please rebase to latest master.
  • There is a bug: Shift+D (twice) the initial objects; in outline click in the topmost, and try to navigate, it jumps every now and then. it seems that clicking in an object in the outliner to make it active, doesn't reset the walking current position.

As for active object change on walking:

  • In object mode it makes sense to change active object.
  • In any edit mode, it should not change active object.
  • Walking through datablocks should not bring objects in/out of current edit mode.

@William Reynish (billreynish) your 2 cents here? ^^ this is similar to the behaviour we have when clicking in the outliner names.

Dalai Felinto (dfelinto) requested changes to this revision.Feb 4 2020, 12:53 PM
This revision now requires changes to proceed.Feb 4 2020, 12:53 PM

Rebasing to master now that I can finally compile :)

I have noticed a few small things that need some cleanup after the changes over the last 6 months. I will address them soon.

I noticed that one of the changes since I made this patch was to always activate the selected (by mouse) element, even when synced selection is disabled. This patch doesn't include that behavior, but I can work it in if needed. I recall discussions during GSoC that when synced selection is disabled, selection would be isolated to the outliner. I believe it was @William Reynish (billreynish) and I having that discussion. Let me know if that has changed and I can update it.

@Dalai Felinto (dfelinto) You mention some walk selection issues. My intent was to update walk navigation after this is accepted, but I can incorporate it into this patch if that what you are suggesting.

source/blender/editors/space_outliner/outliner_select.c
1373

If I understand the current master, this ensures that clicking on the name always activates, regardless of the synced selection toggle. This patch doesn't replicate that behavior, but I can update if needed.

Because current master always activates the clicked item regardless of synced selection, update the patch to replicate that behavior.

Also includes walk navigation that moves the active element. Because walk navigation now requires an active element from which to walk, walk navigation also activates with synced selection disabled.

  • Outliner: Always activate on item name click
  • Outliner: Move active element on walk select

For pose objects it is a bit strange that arrow navigation is still behaving very different than if you were to click manually in the outliner. Is this intentional?

This revision is now accepted and ready to land.Mar 2 2020, 3:45 PM
Dalai Felinto (dfelinto) requested changes to this revision.Mar 2 2020, 3:45 PM
This revision now requires changes to proceed.Mar 2 2020, 3:45 PM

For pose objects it is a bit strange that arrow navigation is still behaving very different than if you were to click manually in the outliner. Is this intentional?

Are you referring to mode toggling? When the walk navigation walks over the Pose datablock and then toggles pose mode? If so, yes, that needs to be fixed, thanks for pointing that out. Otherwise, once pose mode is toggled, walking over the individual bones looks good to me.

Yes, the change of modes. Poke me again when that is fixed then :)

Merged current master.
Reworked the entire patch to have a smaller scope.

Nathan Craddock (natecraddock) retitled this revision from Outliner: Selection refactor to Outliner: Selection cleanup.Jun 3 2020, 6:00 AM
Nathan Craddock (natecraddock) edited the summary of this revision. (Show Details)
  • Outliner: Use outliner_item_select for activation
  • Outliner: Separate mode toggle from activation
  • Outliner: Use bitflag for selection
  • Outliner: cleanup item activate

Update to latest master

  • Merge branch 'master' into outliner-select-cleanup

@Dalai Felinto (dfelinto) or maybe @Julian Eisel (Severin) I believe this is ready now, in functionality I haven't found issues. The only thing that might need changing is naming/style. I debated between booleans and a bitflag for outliner_item_select but eventually chose the bitflag because I think it is overall more readable.

Looks fine, just two minor comments :)

source/blender/editors/space_outliner/outliner_intern.h
222

Would call it TreeItemSelectAction, otherwise it's easy to mistake it for the actual selection flags.

source/blender/editors/space_outliner/outliner_select.c
1220

For readability I suggest you avoid the negation (!extend) and swap the values instead.

This revision is now accepted and ready to land.Jun 19 2020, 8:27 PM
  • Merge branch 'master' into outliner-select-cleanup
  • Cleanup
This revision was automatically updated to reflect the committed changes.