UI: Fix inconsistent menu background padding #111826

Leon Schittek · 2023-09-02T13:17:15+02:00

Leon Schittek commented

2023-09-02 13:17:15 +02:00

This is something from 2.5, has already been partially disabled for a
long time.

This patch was originally authored by Yevgeny Makarov (@jenkm) as
https://archive.blender.org/developer/D13394 , now porting this as a PR.

Comparison


Main
Pull Request

This is something from 2.5, has already been partially disabled for a long time. This patch was originally authored by Yevgeny Makarov (@jenkm) as https://archive.blender.org/developer/D13394 , now porting this as a PR. #### Comparison ||| |---|---| |**Main**|![main_padding.png](/attachments/a2971805-ac26-4e6f-8542-a83dcf463347)| |**Pull Request**|![pr_padding.png](/attachments/4c774365-0d68-439c-a00d-2d4e2aaed94f)|

pr_padding.png

80 KiB

main_padding.png

80 KiB

Leon Schittek added the

Module

User Interface

label 2023-09-02 13:17:15 +02:00

Leon Schittek added 1 commit 2023-09-02 13:17:31 +02:00

e0d28961ba UI: Fix inconsistent menu backround padding

Pablo Vazquez added this to the User Interface project 2023-09-02 13:41:43 +02:00

Pablo Vazquez approved these changes 2023-09-02 13:43:27 +02:00

Pablo Vazquez left a comment

I remember this one! Wondered what happened to it.

Thanks @jenkm and @lone_noel !

I remember this one! Wondered what happened to it. Thanks @jenkm and @lone_noel !

Harley Acheson commented

2023-09-02 23:55:29 +02:00

@lone_noel

Sorry that I haven't actually tested this yet, but taking a quick look this looks like it will have unintended consequences with popup context menus. Right now there is a test for (block_flag & UI_BLOCK_POPUP) that will be true for context menus so it will avoid the next code blocks. With yours fairly certain a context menu will get its top or bottom corners squared off.

@lone_noel Sorry that I haven't actually tested this yet, but taking a quick look this looks like it will have unintended consequences with popup context menus. Right now there is a test for (block_flag & UI_BLOCK_POPUP) that will be true for context menus so it will avoid the next code blocks. With yours fairly certain a context menu will get its top or bottom corners squared off.

Leon Schittek added 1 commit 2023-09-03 08:46:19 +02:00

f7ddbfc4ac Fix top corners on popups not being rounded.

Leon Schittek commented

2023-09-03 08:59:53 +02:00

@Harley you were right! The top corners were indeed squared off. I had only tested it with nested menus, not popups. That is fixed now!

PR before (sharp corners at top)	updated PR (round corners)	main (round corners)

@Harley you were right! The top corners were indeed squared off. I had only tested it with nested menus, not popups. That is fixed now! |PR before (sharp corners at top)|updated PR (round corners)|main (round corners)| |---|---|---| |![pr_popup_wrong.png](/attachments/c9e4be55-616e-47cb-b56b-55076678729e)|![pr_popup_fixed.png](/attachments/1dbb0de8-bfd0-426a-8ecd-cfc92de1aa58)|![main_popup.png](/attachments/262b1b5a-99c4-40c4-8a92-242ab634b93d)|

main_popup.png

42 KiB

pr_popup_wrong.png

42 KiB

pr_popup_fixed.png

42 KiB

❤️ 1

Harley Acheson commented

2023-09-07 00:51:12 +02:00

This works well. But it does have some odd overlap with your changes in #111554. And I find the function name widget_menu_back_roundbox_set be not a good fit since it doesn't set anything.

Ideally if this makes changes to widget_menu_back to solve this issue it would do so in a way that compatible with #111554. That way you can remove the changes from that PR and it will be easier to review since it would then contain only changes to interface_region_popup.cc.

Would something like this work for both?

diff --git a/source/blender/editors/interface/interface_widgets.cc b/source/blender/editors/interface/interface_widgets.cc
index 9543dd51a4f..7223c881569 100644
--- a/source/blender/editors/interface/interface_widgets.cc
+++ b/source/blender/editors/interface/interface_widgets.cc
@@ -2819,26 +2819,30 @@ static void widget_softshadow(const rcti *rect, int roundboxalign, const float r
 
 static void widget_menu_back(
     uiWidgetColors *wcol, rcti *rect, const int block_flag, const int direction, const float zoom)
 {
   uiWidgetBase wtb;
-  int roundboxalign = UI_CNR_ALL;
+  int roundboxalign = UI_CNR_NONE;
 
   widget_init(&wtb);
 
-  /* menu is 2nd level or deeper */
   if (block_flag & UI_BLOCK_POPUP) {
-    // rect->ymin -= 4.0;
-    // rect->ymax += 4.0;
+    roundboxalign = UI_CNR_ALL;
   }
-  else if (direction == UI_DIR_DOWN) {
-    roundboxalign = (UI_CNR_BOTTOM_RIGHT | UI_CNR_BOTTOM_LEFT);
-    rect->ymin -= 0.1f * U.widget_unit;
-  }
-  else if (direction == UI_DIR_UP) {
-    roundboxalign = UI_CNR_TOP_LEFT | UI_CNR_TOP_RIGHT;
-    rect->ymax += 0.1f * U.widget_unit;
+  else {
+    if (direction & UI_DIR_DOWN) {
+      roundboxalign |= (UI_CNR_BOTTOM_RIGHT | UI_CNR_BOTTOM_LEFT);
+    }
+    if (direction & UI_DIR_UP) {
+      roundboxalign |= (UI_CNR_TOP_LEFT | UI_CNR_TOP_RIGHT);
+    }
+    if (direction & UI_DIR_LEFT) {
+      roundboxalign |= (UI_CNR_TOP_RIGHT | UI_CNR_BOTTOM_RIGHT);
+    }
+    if (direction & UI_DIR_RIGHT) {
+      roundboxalign |= (UI_CNR_TOP_LEFT | UI_CNR_BOTTOM_LEFT);
+    }
   }
 
   GPU_blend(GPU_BLEND_ALPHA);
   const float radius = widget_radius_from_zoom(zoom, wcol);
   widget_softshadow(rect, roundboxalign, radius);

This works well. But it does have some odd overlap with your changes in #111554. And I find the function name `widget_menu_back_roundbox_set` be not a good fit since it doesn't set anything. Ideally if this makes changes to `widget_menu_back` to solve this issue it would do so in a way that compatible with #111554. That way you can remove the changes from that PR and it will be easier to review since it would then contain only changes to `interface_region_popup.cc`. Would something like this work for both? ``` diff --git a/source/blender/editors/interface/interface_widgets.cc b/source/blender/editors/interface/interface_widgets.cc index 9543dd51a4f..7223c881569 100644 --- a/source/blender/editors/interface/interface_widgets.cc +++ b/source/blender/editors/interface/interface_widgets.cc @@ -2819,26 +2819,30 @@ static void widget_softshadow(const rcti *rect, int roundboxalign, const float r static void widget_menu_back( uiWidgetColors *wcol, rcti *rect, const int block_flag, const int direction, const float zoom) { uiWidgetBase wtb; - int roundboxalign = UI_CNR_ALL; + int roundboxalign = UI_CNR_NONE; widget_init(&wtb); - /* menu is 2nd level or deeper */ if (block_flag & UI_BLOCK_POPUP) { - // rect->ymin -= 4.0; - // rect->ymax += 4.0; + roundboxalign = UI_CNR_ALL; } - else if (direction == UI_DIR_DOWN) { - roundboxalign = (UI_CNR_BOTTOM_RIGHT | UI_CNR_BOTTOM_LEFT); - rect->ymin -= 0.1f * U.widget_unit; - } - else if (direction == UI_DIR_UP) { - roundboxalign = UI_CNR_TOP_LEFT | UI_CNR_TOP_RIGHT; - rect->ymax += 0.1f * U.widget_unit; + else { + if (direction & UI_DIR_DOWN) { + roundboxalign |= (UI_CNR_BOTTOM_RIGHT | UI_CNR_BOTTOM_LEFT); + } + if (direction & UI_DIR_UP) { + roundboxalign |= (UI_CNR_TOP_LEFT | UI_CNR_TOP_RIGHT); + } + if (direction & UI_DIR_LEFT) { + roundboxalign |= (UI_CNR_TOP_RIGHT | UI_CNR_BOTTOM_RIGHT); + } + if (direction & UI_DIR_RIGHT) { + roundboxalign |= (UI_CNR_TOP_LEFT | UI_CNR_BOTTOM_LEFT); + } } GPU_blend(GPU_BLEND_ALPHA); const float radius = widget_radius_from_zoom(zoom, wcol); widget_softshadow(rect, roundboxalign, radius); ```

Leon Schittek commented

2023-09-07 07:29:50 +02:00

And I find the function name widget_menu_back_roundbox_set be not a good fit since it doesn't set anything.

I don't disagree that it's a bit odd, but I chose the name to be consistent with widget_roundbox_set which does the same thing for normal widgets. But if simplified like you mentioned it might not be necessary to make this it's own function.

Ideally if this makes changes to widget_menu_back to solve this issue it would do so in a way that compatible with #111554. That way you can remove the changes from that PR and it will be easier to review since it would then contain only changes to interface_region_popup.cc.

Sounds good.

Would something like this work for both?

I think it should. I remember the padding being the only reason this didn't work in #111554 but I'll test to make sure.

Thanks for all the feedback! I should be able to update this and the related PRs by tomorrow!

> And I find the function name `widget_menu_back_roundbox_set` be not a good fit since it doesn't set anything. I don't disagree that it's a bit odd, but I chose the name to be consistent with `widget_roundbox_set` which does the same thing for normal widgets. But if simplified like you mentioned it might not be necessary to make this it's own function. > Ideally if this makes changes to `widget_menu_back` to solve this issue it would do so in a way that compatible with #111554. That way you can remove the changes from that PR and it will be easier to review since it would then contain only changes to `interface_region_popup.cc`. Sounds good. > Would something like this work for both? I think it should. I remember the padding being the only reason this didn't work in #111554 but I'll test to make sure. Thanks for all the feedback! I should be able to update this and the related PRs by tomorrow!

Pablo Vazquez changed title from ~~UI: Fix inconsistent menu backround padding~~ to UI: Fix inconsistent menu background padding

2023-09-07 16:17:34 +02:00

Leon Schittek added 2 commits 2023-09-08 13:16:10 +02:00

5cfd63cb0a Merge branch 'main' into menu-padding

1681ebffc2 Partially revert previous changes so this fix is minimal.

Leon Schittek commented

2023-09-08 13:20:18 +02:00

Would something like this work for both?

@Harley After testing it unfortunetely doesn't quite work, since nested menus will now only have only the outer corners rounded.
(The comment /* menu is 2nd level or deeper */ seems to be wrong, since nested menus are not considered popups, since they are emitted from a button...)

This can be fixed, but it goes into a similar territory as #111554 creating even more overlap.

So i've updated this patch to be as minimal of a change as can be. We can either merge this as the small fix it is or close this PR and I'll just include the removal of the padding in #111554, since it touches this area anyway.

I also found that breaking out the other changes in interface_region_popup.cc from #111554 into this one doesn't work too well, since they really only make sense in the context of supporting more complex corner rounding.

> Would something like this work for both? @Harley After testing it unfortunetely doesn't quite work, since nested menus will now only have only the outer corners rounded. (The comment `/* menu is 2nd level or deeper */` seems to be wrong, since nested menus are not considered popups, since they are emitted from a button...) This can be fixed, but it goes into a similar territory as #111554 creating even more overlap. So i've updated this patch to be as minimal of a change as can be. We can either merge this as the small fix it is or close this PR and I'll just include the removal of the padding in #111554, since it touches this area anyway. I also found that breaking out the other changes in interface_region_popup.cc from #111554 into this one doesn't work too well, since they really only make sense in the context of supporting more complex corner rounding.

Harley Acheson approved these changes 2023-09-08 17:41:15 +02:00

Harley Acheson left a comment

Make sense.

The other complications can be dealt with in #111554

Make sense. The other complications can be dealt with in #111554

Harley Acheson referenced this issue from a commit

2023-09-08 17:43:41 +02:00

UI: Fix inconsistent menu background padding

Harley Acheson merged commit cc1c38f74d into main

2023-09-08 17:43:41 +02:00

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

UI: Fix inconsistent menu background padding #111826

Comparison