Page MenuHome
Differential D9758

UI: Properties editor popover & outliner sync dropdown
ClosedPublic
Actions

Authored by Nathan Craddock (natecraddock) on Dec 6 2020, 6:29 AM.

Details

Reviewers
Hans Goudey (HooglyBoogly)
Group Reviewers
User Interface
Commits
rBffacce5be41d: UI: Properties editor popover and outliner sync
Summary

This adds a popover to the properties editor to display a dropdown to specify the type of outliner to properties sync.

Because we cannot define a perfect heuristic to deterimine when properties editors will change tabs based on outliner icon selection, we need an option to enable or disable this behavior per properties editor. As the properties editor lacks popovers and menus, this patch adds a popover to display this new option.

The dropdown has 3 options (Auto, Off, On) and defaults to Auto. Auto uses the heuristic to only allow tab switching when a properties editor and outliner share a border.

I tried removing the dummy icon for centering the properties search field, but the centering was still not perfect.

T83326

Diff Detail

Repository
rB Blender
Branch
properties-popover (branched from master)
Build Status
Buildable 11815
Build 11815: arc lint + arc unit

Event Timeline

Nathan Craddock (natecraddock) created this revision.Dec 6 2020, 6:29 AM
Harbormaster completed remote builds in B11576: Diff 31645.Dec 6 2020, 6:29 AM
Nathan Craddock (natecraddock) requested review of this revision.Dec 6 2020, 6:29 AM
Nathan Craddock (natecraddock) edited the summary of this revision. (Show Details)Dec 6 2020, 6:32 AM
Nathan Craddock (natecraddock) added a reviewer: User Interface.
Nathan Craddock (natecraddock) added a project: User Interface.
Nathan Craddock (natecraddock) edited the summary of this revision. (Show Details)Dec 6 2020, 6:36 AM
Oleg (DotBow) awarded a token.Dec 6 2020, 10:22 PM
Nathan Craddock (natecraddock) updated this revision to Diff 31750.Dec 8 2020, 7:09 PM

Show enum expanded

Harbormaster completed remote builds in B11626: Diff 31750.Dec 8 2020, 7:09 PM
Nathan Craddock (natecraddock) edited the summary of this revision. (Show Details)Dec 8 2020, 7:09 PM
Nathan Craddock (natecraddock) edited the summary of this revision. (Show Details)Dec 10 2020, 11:36 PM
Nathan Craddock (natecraddock) updated this revision to Diff 31845.Dec 10 2020, 11:57 PM

Cleanup versioning

Harbormaster completed remote builds in B11683: Diff 31845.Dec 10 2020, 11:57 PM
Nathan Craddock (natecraddock) updated this revision to Diff 31846.Dec 11 2020, 12:02 AM

Cleanup UI labels

Harbormaster completed remote builds in B11684: Diff 31846.Dec 11 2020, 12:02 AM
Hans Goudey (HooglyBoogly) requested changes to this revision.Dec 11 2020, 12:32 AM
Hans Goudey (HooglyBoogly) added a subscriber: Hans Goudey (HooglyBoogly).

I like this change, and I like the design and layout you chose.

Side note, I tried this, but I like the version in the patch better I think.

However, it's hard for me to accept this with the way the popover arrow is drawn. It's better on higher DPI, but for me, the arrow doesn't even touch the button:

I know it's way outside the scope of this patch, but I also don't think it would be very hard to tweak ui_draw_popover_back to better place the arrow on the far edge of the popover when it needs to.
This change isn't a hard requirement for this patch to me, but it would be very nice to solve at the same time, it looks a bit silly without it.

release/scripts/startup/bl_ui/space_properties.py
85

We generally use "Sync" instead of "Synchronize".

This revision now requires changes to proceed.Dec 11 2020, 12:32 AM
Hans Goudey (HooglyBoogly) added inline comments.Dec 11 2020, 12:55 AM
release/scripts/startup/bl_ui/space_properties.py
85

Actually, I'm wondering, do you think it is important to emphasize the "direction" of the syncing? Sync with Outliner sounds a bit more natural, but maybe the "direction" of the action is significant?

source/blender/makesdna/DNA_space_types.h
170

Instead of adding new padding, you can use a bit from one of the padding fields above.

Also, this should note the enum type in a comment. /* eSpaceButtons_TabSync */

172–173

This white space change should be reverted.

source/blender/makesrna/intern/rna_space.c
4780

Why not be explicit here and say how it actually works: Sync when this editor shares an edge with the outliner or Sync when the property editor and outliner share an edge.

Nathan Craddock (natecraddock) updated this revision to Diff 31852.Dec 11 2020, 1:23 AM
Nathan Craddock (natecraddock) marked 5 inline comments as done.

Update tooltips and reorder the enum
On | Off | Auto sounds better than Auto | Off | On

Harbormaster completed remote builds in B11686: Diff 31852.Dec 11 2020, 1:23 AM
Nathan Craddock (natecraddock) added inline comments.Dec 11 2020, 1:24 AM
release/scripts/startup/bl_ui/space_properties.py
85

I chose "from" because it's not bidirectional, but I think it's fine to lose accuracy and make it more natural with "with".

Nathan Craddock (natecraddock) mentioned this in D9865: UI: Popover triangle alignment tweaks.Dec 16 2020, 12:54 PM
Nathan Craddock (natecraddock) added a subscriber: Yevgeny Makarov (jenkm).Dec 17 2020, 6:30 PM

@Yevgeny Makarov (jenkm) suggested using ICON_OPTIONS. I prefer this to no icon (And preferred to the filter icon in the outliner too but that's a different topic.)

@Hans Goudey (HooglyBoogly) I think with this icon the patch could be merged without waiting for the fixes in D9865

Hans Goudey (HooglyBoogly) requested changes to this revision.Dec 18 2020, 1:00 AM

Almost there. I just suggested a small visual tweak and a refactor.

I strongly prefer the version without an icon.

  1. The "settings" or "extras" icons are redundant-- what else do we expect to find in a popover besides settings?
  2. A "sync" icon makes it weird adding other settings to the popover
  3. An icon is visually noisy, drawing attention to a relatively unimportant setting that will probably be used less than others.

I'm sure we can find a way to make the arrow work.

In D9758#246610, @Zachman wrote:

I think with this icon the patch could be merged without waiting for the fixes in D9865

Now that we're already working on the other patch, I'd be fine committing this first. It's polish anyway, with my first comment my idea was to make sure we wouldn't forget about dealing with it.

release/scripts/startup/bl_ui/space_properties.py
84–87

I'd suggest this, just to bring the label a little bit closer to the enum.

col = layout.column()
col.label(text="Sync with Outliner")
col.row().prop(space, "outliner_sync", expand=True)
source/blender/editors/space_buttons/buttons_context.c
758–765

This function isn't really doing what it's name suggests now, there are checks specific to the outliner property editor syncing now. It probably makes either rename the function (though that's weird because then there would be an outliner-specific function in the property editor code, or do some refactoring to split off the general checks.

Maybe this?

1. Get region type with shared border
2. Do property editor specific sync flag checks
3. Set property editor context with this function

That's a bit more elaborate, but it's all a bit more organized and general, so it's probably better.

This revision now requires changes to proceed.Dec 18 2020, 1:00 AM
Nathan Craddock (natecraddock) updated this revision to Diff 32131.Dec 18 2020, 3:50 AM
Nathan Craddock (natecraddock) marked 2 inline comments as done.

Cleanup & layout tweaks

Harbormaster completed remote builds in B11815: Diff 32131.Dec 18 2020, 3:50 AM
Nathan Craddock (natecraddock) added a comment.Dec 18 2020, 3:52 AM

I cleaned up the code a bit. From what you said the most important thing was to separate ED_buttons_set_context from the outliner to properties editor specific case.

Hans Goudey (HooglyBoogly) accepted this revision.Dec 21 2020, 6:20 AM

This looks ready to me-- fine to commit now I think. Let's not worry about that little arrow at this very moment, it's brought up a few too many questions than I expected / hoped for anyway.

source/blender/editors/space_buttons/buttons_context.c
755

Since this function is really about whether it should sync with the outliner for this specific purpose, maybe it should be called ED_buttons_should_sync_with_outliner.

This revision is now accepted and ready to land.Dec 21 2020, 6:20 AM
Nathan Craddock (natecraddock) updated this revision to Diff 32226.Dec 21 2020, 10:14 PM
Nathan Craddock (natecraddock) marked an inline comment as done.

Rename function

Harbormaster completed remote builds in B11848: Diff 32226.Dec 21 2020, 10:14 PM
Closed by commit rBffacce5be41d: UI: Properties editor popover and outliner sync (authored by Nathan Craddock (natecraddock)). · Explain WhyDec 21 2020, 10:39 PM
This revision was automatically updated to reflect the committed changes.
Nathan Craddock (natecraddock) added a commit: rBffacce5be41d: UI: Properties editor popover and outliner sync.
Blender Defender (Blender_Defender) awarded a token.Dec 22 2020, 12:17 PM

Revision Contents
Changeset List

PathSize
release/
scripts/
startup/
bl_ui/
18 lines
source/
blender/
blenloader/
intern/
12 lines
editors/
include/
9 lines
space_buttons/
31 lines
space_outliner/
20 lines
makesdna/
12 lines
makesrna/
intern/
18 lines

Diff 32131

View Options

release/scripts/startup/bl_ui/space_properties.py

Loading...
View Options

source/blender/blenloader/intern/versioning_290.c

Loading...
View Options

source/blender/editors/include/ED_buttons.h

Loading...
View Options

source/blender/editors/space_buttons/buttons_context.c

Loading...
View Options

source/blender/editors/space_outliner/outliner_select.c

Loading...
View Options

source/blender/makesdna/DNA_space_types.h

Loading...
View Options

source/blender/makesrna/intern/rna_space.c

Loading...