Details

Reviewers

Maniphest Tasks

T77408: Continued Outliner Improvements Design

Commits

rB0e47e57eb77d: Outliner: Properties editor sync on selection

Summary

When outliner datablocks are selected, switch the properties editors in the current workspace to the correct tab for that datablock.

Includes logic to ensure the tab doesn't switch to an unavailable tab when a datablock is pinned in a properties editor.

There is a heuristic to only change the properties editor tabs for editors bordering the outliner. This uses the same shared edge checking as the area join operator.

When clicking modifiers, constraints, and shader effects in the outliner, the corresponding panel will be opened in all properties editors.

Diff Detail

Repository

rB Blender

Branch

outliner-properties-sync (branched from master)

Build Status

Buildable 9791
Build 9791: arc lint + arc unit

Event Timeline

Nathan Craddock (natecraddock) requested review of this revision.Aug 19 2020, 8:06 PM

Nathan Craddock (natecraddock) created this revision.

Harbormaster completed remote builds in B9655: Diff 27933.Aug 19 2020, 8:06 PM

Nathan Craddock (natecraddock) mentioned this in T77408: Continued Outliner Improvements Design.Aug 19 2020, 8:08 PM

Nathan Craddock (natecraddock) edited the summary of this revision. (Show Details)Aug 19 2020, 8:12 PM

Nathan Craddock (natecraddock) added a project: User Interface.

The code side looks mostly fine again. But I do see some design issues:

If we apply this we loose the ability to change modes in the Outliner. So we shouldn't commit this without D8641 as well. However there are open technical concerns raised about this (asked Campbell to note them in the differential).
I'm unsure if the design of changing all Properties in the current window will work well in practice. I would like to at least try out the alternative of trying to use connected editors only (ie with a shared edge).
This current design won't work for floating windows. So if you shift+drag the Properties editor corner to duplicate it into a new window and close the non-floating one, changing the Outliner selection won't change the floating Properties. Maybe you should iterate over all visible layouts of the current workspace until you found one with the properties visible?

Also, these are separate changes, which I don't minding reviewing together. But of course they should be committed separately.

source/blender/editors/space_buttons/buttons_context.c
779–781	The active space type is typically checked via `ScrArea.spacetype`.
source/blender/makesdna/DNA_outliner_types.h
112–113 ↗	(On Diff #27933)	These are grease pencil only, right? In that case I'd prefer something like `TSE_GPENCIL_EFFECT`

I'm fine with having the GPencil modifier and shader effects committed to master directly (although there are a few tweaks I'd suggest, see the inline comments).
These changes are straight forward and make sense to me.

source/blender/editors/space_outliner/outliner_tree.c
565 ↗	(On Diff #27933)	There's `LISTBASE_FOREACH_INDEX` now that you can use here.

Fix: Use proper method of finding active space type

Harbormaster completed remote builds in B9785: Diff 28141.Aug 25 2020, 5:21 PM

Just in case you missed it-- looks like outliner_item_mode_toggle is unused with this patch applied.

Rebase on master after committing gpencil modifier & shaders in the tree

Harbormaster completed remote builds in B9791: Diff 28155.Aug 26 2020, 12:04 AM

Rebase master

Harbormaster completed remote builds in B10045: Diff 28643.Sep 8 2020, 6:34 AM

Julian Eisel (Severin) added a task: T77408: Continued Outliner Improvements Design.Sep 10 2020, 2:48 PM

Nathan Craddock (natecraddock) mentioned this in D8646: Outliner: Manual object sorting.Sep 16 2020, 5:31 AM

slowk1d (Geronimooo) awarded a token.Sep 19 2020, 11:34 AM

Nathan Craddock (natecraddock) marked an inline comment as done.Sep 20 2020, 1:06 AM

Nathan Craddock (natecraddock) added a reviewer: Julian Eisel (Severin).

Nathan Craddock (natecraddock) added a project: Restricted Project.

Nathan Craddock (natecraddock) edited the summary of this revision. (Show Details)

Only update for shared edges
Rebase on master

Harbormaster completed remote builds in B10319: Diff 29094.Sep 20 2020, 1:56 AM

I'm curious if this could still be added to 2.91. I know that bcon2 allows for smaller new features, and D8641: Outliner: Add mode toggling column was partially motivated by properties syncing. It feels incomplete to me to have moved mode toggling to the left column but leaving the datablocks without syncing in 2.91.

If we apply this we loose the ability to change modes in the Outliner. So we shouldn't commit this without D8641 as well. However there are open technical concerns raised about this (asked Campbell to note them in the differential).

No longer an issue :)

I'm unsure if the design of changing all Properties in the current window will work well in practice. I would like to at least try out the alternative of trying to use connected editors only (ie with a shared edge).

I just committed a fix to test this behavior. I think it works well. I'm using the same function that is use to determine if areas can be joined, exposed as an ED_area function.

This current design won't work for floating windows. So if you shift+drag the Properties editor corner to duplicate it into a new window and close the non-floating one, changing the Outliner selection won't change the floating Properties. Maybe you should iterate over all visible layouts of the current workspace until you found one with the properties visible?

Maybe I'm misunderstanding, but I see a bit of conflict between this and the previous suggestion. Does it make sense to sync floating properties editors when we do extra work to only sync bordering properties editors?

I recorded a short video showing the shared edges behavior.

Nathan Craddock (natecraddock) edited the summary of this revision. (Show Details)Sep 20 2020, 3:06 AM

Nathan Craddock (natecraddock) edited the summary of this revision. (Show Details)Sep 21 2020, 5:47 PM

Nathan Craddock (natecraddock) edited the summary of this revision. (Show Details)

Nathan Craddock (natecraddock) edited the summary of this revision. (Show Details)Sep 21 2020, 5:50 PM

Looks quite good, simple and intuitive too : )

source/blender/editors/space_buttons/buttons_context.c
780–781	There's an odd combination of `continue` and indentation here. I would suggest something like this: LISTBASE_FOREACH (ScrArea *, area, &screen->areabase) { if (area->spacetype != SPACE_PROPERTIES) { continue; } if (!ED_area_has_shared_border(active_area, area)) { continue; } ...
source/blender/editors/space_outliner/outliner_select.c
1228–1229	Commented out code should be removed? Or inside an `#if 0` with a comment for why it isn't used.

Cleanup: Remove comment
Cleanup: Continue and indentation

Harbormaster completed remote builds in B10338: Diff 29143.Sep 21 2020, 6:45 PM

William Reynish (billreynish) awarded a token.Sep 22 2020, 11:28 AM

Fran Marz (franMarz) awarded a token.Sep 22 2020, 6:05 PM

Pablo Vazquez (pablovazquez) awarded a token.Sep 22 2020, 6:51 PM

slwk1d (Slowwkidd) awarded a token.Sep 22 2020, 9:01 PM

Yuichiro Fujita (chironamo) awarded a token.Sep 22 2020, 9:35 PM

slwk1d (Slowwkidd) added a subscriber: slwk1d (Slowwkidd).Sep 28 2020, 6:23 PM

Gilberto Rodrigues (gilberto_rodrigues) awarded a token.Oct 7 2020, 11:28 PM

Julian Eisel (Severin) mentioned this in D8641: Outliner: Add mode toggling column.Oct 22 2020, 12:44 PM

Code is fine, just suggesting to minor tweaks.

source/blender/editors/space_outliner/outliner_select.c
1152	There are all these different calls to `ED_buttons_set_context()` and the only thing that changes is one parameter. I'd just set a `context` for each case and at the end of he function do: if (ptr.data) { ED_buttons_set_context(C, &ptr, context); } Note that you'll have to initialize `ptr = {0}` for that.
1218–1225	These low level operations bother me. Could we just have a `BKE_modifier_show_ui()` (and similar for the other cases)?