UI: Configurable Layout Separators #117310

Harley Acheson · 2024-01-19T00:48:22+01:00

Harley Acheson commented

2024-01-19 00:48:22 +01:00

Ability to create vertical bar separators in horizontal layouts,
horizontal lines in popovers, and vertical (non-line) spacers in menus.

It was brought up in the UI Module meeting that we might want vertical bar section separators for horizontal layouts like headers. I've tried to make this as unobtrusive as possible. Current uses of layout.separator() continue to work exactly as they do now.

Currently the separator type is inferred from the button type. It makes a horizontal line if in a menu, or just horizontal space if not. To keep this behavior the same, I've added a type argument which has a default value of AUTO which retains this same behavior. So existing uses of separator are unchanged.

However, you can force a different separator if you wish, by using either SPACE or LINE. When using LINE the direction of the line is inferred from the layout direction. This works since I can't find a situation where this doesn't work.

Here are two useless examples of LINE, one on the TopBar and another on the 3D View header:

Following is a before and after comparison of the use of the horizontal rule as a separator between title and content on props dialogs:

The factor argument remains and works as you expect. For horizontal lines this still gives expanded or reduced vertical spacing. For vertical bars it does so for horizontal space, with the bar in the middle of it.

Ability to create vertical bar separators in horizontal layouts, horizontal lines in popovers, and vertical (non-line) spacers in menus. ---- It was brought up in the UI Module meeting that we might want vertical bar section separators for horizontal layouts like headers. I've tried to make this as unobtrusive as possible. Current uses of `layout.separator()` continue to work exactly as they do now. Currently the separator type is inferred from the button type. It makes a horizontal line if in a menu, or just horizontal space if not. To keep this behavior the same, I've added a `type` argument which has a default value of `AUTO` which retains this same behavior. So existing uses of separator are unchanged. However, you can force a different separator if you wish, by using either `SPACE` or `LINE`. When using `LINE` the direction of the line is inferred from the layout direction. This works since I can't find a situation where this doesn't work. ![image](/attachments/f1e65d40-1032-4dbc-ad6f-f3af2c552907) Here are two useless examples of `LINE`, one on the TopBar and another on the 3D View header: ![image](/attachments/b91508ff-85a7-46cc-8777-ec91d9504489) Following is a before and after comparison of the use of the horizontal rule as a separator between title and content on props dialogs: ![image](/attachments/65504ad1-9322-4132-ac50-cceb86a98f08) The `factor` argument remains and works as you expect. For horizontal lines this still gives expanded or reduced vertical spacing. For vertical bars it does so for horizontal space, with the bar in the middle of it.

image.png

12 KiB

image.png

44 KiB

image.png

28 KiB

image.png

48 KiB

image.png

29 KiB

👍 11 👀 4

Harley Acheson added 1 commit 2024-01-19 00:48:31 +01:00

WIP: UI: Vertical Bar Separator 106a183740

Ability to create vertical bar separators in horizontal layouts,
horizontal lines in popovers, and vertical (non-line) spacers in menus.

Harley Acheson added this to the User Interface project 2024-01-19 00:50:25 +01:00

Harley Acheson requested review from Pablo Vazquez 2024-01-19 00:50:32 +01:00

Harley Acheson referenced this pull request

2024-01-24 01:24:42 +01:00

WIP: UI: Operator Props Dialog Changes #117465

Harley Acheson requested review from Hans Goudey 2024-01-25 02:22:41 +01:00

Harley Acheson referenced this pull request

2024-01-27 03:58:02 +01:00

UI: Operator Confirm Dialog Changes #117564

Harley Acheson changed title from ~~WIP: UI: Vertical Bar Separator~~ to UI: Vertical Bar Separator

2024-01-28 22:54:45 +01:00

Hans Goudey requested changes 2024-01-30 04:14:30 +01:00

Hans Goudey left a comment

This looks nice. I don't feel like I can answer where we would use this by default (maybe Pablo can!) but having the option seems reasonable.

One big picture question is whether we actually need LayoutSeparatorType. Maybe using the layout's direction would always be enough. That would definitely simplify some things.

This looks nice. I don't feel like I can answer where we would use this by default (maybe Pablo can!) but having the option seems reasonable. One big picture question is whether we actually need `LayoutSeparatorType`. Maybe using the layout's direction would always be enough. That would definitely simplify some things.

source/blender/editors/include/UI_interface_c.hh Outdated

						
				@ -1501,6 +1501,12 @@ enum eButProgressType {

				  UI_BUT_PROGRESS_TYPE_RING = 1,

				};

				enum eLayoutSeparatorType {

Hans Goudey commented

2024-01-30 04:12:23 +01:00

How about this for some stronger type safety and prettier code?

enum class LayoutSeparatorType : int8_t{
  Auto,
  Space,
  Line,
};

Then you refer to it like LayoutSeparatorType::Space elsewhere, and it can be forward declared like enum class LayoutSeparatorType : int8_t;

I'd also try representing the "Auto" state with std::optional. But maybe that wouldn't be better

How about this for some stronger type safety and prettier code? ```cpp enum class LayoutSeparatorType : int8_t{ Auto, Space, Line, }; ``` Then you refer to it like `LayoutSeparatorType::Space` elsewhere, and it can be forward declared like `enum class LayoutSeparatorType : int8_t;` I'd also _try_ representing the "Auto" state with `std::optional`. But maybe that wouldn't be better

Harley marked this conversation as resolved

source/blender/editors/interface/interface_layout.cc Outdated

						
				@ -3458,1 +3457,3 @@

				  space *= factor;

				  bool is_vertical = (layout->w > 0);

				  int width = (is_vertical) ? int(UI_UNIT_X) : int(0.3f * UI_UNIT_X * factor);

Hans Goudey commented

2024-01-30 04:10:20 +01:00

Parentheses around is_vertical are unnecessary

Parentheses around `is_vertical` are unnecessary

Harley marked this conversation as resolved

source/blender/editors/interface/interface_layout.cc Outdated

						
				@ -3459,0 +3460,4 @@

				  int height = (is_vertical) ? int(0.35f * UI_UNIT_X * factor) : int(UI_UNIT_Y);

				  eButType but_type = UI_BTYPE_SEPR;

				  if (type == UI_LAYOUT_SEPARATOR_LINE) {

Hans Goudey commented

2024-01-30 04:10:03 +01:00

Can this be a switch statement?

Harley marked this conversation as resolved

source/blender/editors/interface/interface_layout.cc Outdated

						
				@ -3470,3 +3482,2 @@

				           0.0,

				           0.0,

				           0,

				           0.0, (is_vertical_bar) ? 1.0f : 0.0f,

Hans Goudey commented

2024-01-30 04:10:37 +01:00

Clang format

Harley marked this conversation as resolved

source/blender/editors/interface/interface_widgets.cc Outdated

						
				@ -3344,2 +3344,2 @@

				/** Separator for menus. */

				static void ui_draw_separator(const rcti *rect, const uiWidgetColors *wcol)

				/** Separator line. */

				static void ui_draw_separator(const rcti *rect, const uiWidgetColors *wcol, bool vertical)

Hans Goudey commented

2024-01-30 04:11:49 +01:00

Got to say it feels weird to pass the combination of rect and `vertical here. I seems like we only end up needing two points. Maybe it would be clearer to pass those here.

Got to say it feels weird to pass the combination of `rect` and `vertical here. I seems like we only end up needing two points. Maybe it would be clearer to pass those here.

Harley marked this conversation as resolved

Harley Acheson added 2 commits 2024-02-01 04:17:14 +01:00

Merge branch 'main' of projects.blender.org:blender/blender into VerticalSeparator 78d1e967e1

Changes requested by review 789adfaeb3

Zach Eastin commented

2024-02-01 17:08:33 +01:00

First-time contributor

First off, thank you so much for this! I can't tell you how much time I've tried creating hacky horizontal lines in my addons using repeating hyphens or asterisks.
Second, if done in a normal panel would it make a vertical line in a layout.row.separator(type="LINE") and a horizontal line in a layout.column.separator(type="OINE")? What about layout.grid?

First off, thank you so much for this! I can't tell you how much time I've tried creating hacky horizontal lines in my addons using repeating hyphens or asterisks. Second, if done in a normal panel would it make a vertical line in a `layout.row.separator(type="LINE")` and a horizontal line in a `layout.column.separator(type="OINE")`? What about `layout.grid`?

Harley Acheson commented

2024-02-01 17:17:16 +01:00

@Zeastin - What about layout.grid?

Are you able to give me a scenario to test? Maybe a sample script to run and describing the expected result? Or maybe describe a change to an existing script?

> @Zeastin - What about `layout.grid`? Are you able to give me a scenario to test? Maybe a sample script to run and describing the expected result? Or maybe describe a change to an existing script?

Zach Eastin commented

2024-02-01 17:24:35 +01:00

First-time contributor

Are you able to give me a scenario to test?

No, sorry I don't have anything in mind. I was mainly asking about the row/column thing and grid just popped in my mind and was curious.

> > Are you able to give me a scenario to test? > No, sorry I don't have anything in mind. I was mainly asking about the row/column thing and grid just popped in my mind and was curious.

Harley Acheson referenced this pull request

2024-02-08 17:38:29 +01:00

IDManagement: Update the Purge operator to display an interactive popup. #117304

Harley Acheson changed title from ~~UI: Vertical Bar Separator~~ to UI: Configurable Layout Separators

2024-02-08 17:51:25 +01:00

Harley Acheson commented

2024-02-08 18:17:41 +01:00

@HooglyBoogly - where we would use this by default (maybe Pablo can!) but having the option seems reasonable.

I don't think we'd need it often. But the following is an example where I would do so, with props dialogs to separate the title from the rest:

Maybe using the layout's direction would always be enough. That would definitely simplify some things.

I'm not sure that is possible, but maybe you are thinking of something that I have not considered correctly?

With a layout that has a vertical direction, like in dialogs and popups, we currently only have the option of a separator comprising only vertical space. But this PR would allow us to have both spacers and horizontal rules.

I think the only way to get rid of the need for LayoutSeparatorType would be replace all our current usages of layout.separator with a label when it is used to just used to make a vertical gap. Then we could change separator to always be a line for vertical layouts. But we use separator for vertical spaces all over the place and probably in many addons.

Or were you thinking of some other way or not needing LayoutSeparatorType ?

> @HooglyBoogly - where we would use this by default (maybe Pablo can!) but having the option seems reasonable. I don't think we'd need it often. But the following is an example where I would do so, with props dialogs to separate the title from the rest: ![image](/attachments/e12f6ab0-594a-4769-ae03-48b5641b1806) > Maybe using the layout's direction would always be enough. That would definitely simplify some things. I'm not sure that is possible, but maybe you are thinking of something that I have not considered correctly? With a layout that has a vertical direction, like in dialogs and popups, we currently only have the option of a separator comprising only vertical space. But this PR would allow us to have **both** spacers and horizontal rules. I _think_ the only way to get rid of the need for LayoutSeparatorType would be replace all our current usages of layout.separator with a label when it is used to just used to make a vertical gap. Then we could change separator to always be a line for vertical layouts. But we use separator for vertical spaces all over the place and probably in many addons. Or were you thinking of some other way or not needing LayoutSeparatorType ?

image.png

25 KiB

Harley Acheson commented

2024-02-08 18:21:12 +01:00

@pablovazquez

Can you look at the following image and consider the brightness of the horizontal rule on the right? It looks a bit too dim to me but I've been staring at it too long now and need a fresh opinion:

@pablovazquez Can you look at the following image and consider the brightness of the horizontal rule on the right? It looks a bit too dim to me but I've been staring at it too long now and need a fresh opinion: ![image](/attachments/e42d5a8b-3578-466a-a2fc-7bdef78307c5)

image.png

25 KiB

Hans Goudey approved these changes 2024-02-08 18:30:51 +01:00

Hans Goudey left a comment

Ah, my bad, I misunderstood what LayoutSeparatorType was doing here. Somehow I thought it was controlling the vertical/horizontal choice. Maybe I was asleep. Anyway, this change looks good to me.

Ah, my bad, I misunderstood what `LayoutSeparatorType` was doing here. Somehow I thought it was controlling the vertical/horizontal choice. Maybe I was asleep. Anyway, this change looks good to me.

source/blender/editors/interface/interface_layout.cc Outdated

						
				@ -3449,3 +3449,3 @@

				}

				void uiItemS_ex(uiLayout *layout, float factor)

				void uiItemS_ex(uiLayout *layout, float factor, LayoutSeparatorType type)