Page MenuHome

UI: Fix drawing of aligned icon buttons in menu rows
ClosedPublic

Authored by Nathan Craddock (natecraddock) on Jul 16 2020, 7:35 PM.
Tags
Tokens
"Love" token, awarded by 1D_Inc."Love" token, awarded by monio."Love" token, awarded by billreynish.

Details

Summary

This adds support for drawing icon buttons as a row in a popup menu. This is needed for drawing collection color tag icons in a row in the outliner context menu.

A few issues still exist, but I would like some initial review now that it mostly works.

  • The icons do not draw aligned with the text labels. This is easy to fix, but the hover highlight is still drawn incorrectly.
  • This works well when creating row icon buttons with an operator_enum, but manually creating the row elements doesn't draw an initial offset blank icon.


I have been testing on soc-2020-outliner with this small change to space_outliner.py

-            layout.operator_enum("outliner.collection_color_tag_set", "color")
+            row = layout.row(align=True)
+            row.operator_enum("outliner.collection_color_tag_set", "color", icon_only=True)

Diff Detail

Repository
rB Blender
Branch
fix-inline-icons (branched from master)
Build Status
Buildable 10160
Build 10160: arc lint + arc unit

Event Timeline

Nathan Craddock (natecraddock) requested review of this revision.Jul 16 2020, 7:35 PM
Nathan Craddock (natecraddock) created this revision.
Nathan Craddock (natecraddock) retitled this revision from UI: Fix drawing of aligned buttons in menu rows to UI: Fix drawing of aligned icon buttons in menu rows.Jul 16 2020, 7:43 PM
Nathan Craddock (natecraddock) edited the summary of this revision. (Show Details)
Nathan Craddock (natecraddock) added projects: User Interface, Restricted Project.

Nice common solution for color tagging.
Are color swatches predefined or user-defined with some defaults?

If predefined, I would like to propose to place them in Hue order, avoid Value/Saturation variations and reserve some defined color for cumulative collections which shares the same objects
(like purple, which is used, for example, for cases when texture is missing).


This will be helpful for possible further viewport shading Collections coloring mode, since cumulative objects may belong to multiple color tagged collections, so they can be detected this way using viewport shading, avoiding color uncertainity.
So regular objects will take collection's color, and cumulative objects will take that prefefined color in such viewport shading mode.
This reserved color should be out of the palette and predefinder palette should not contain color that are close to that color.

If swatches are user-defined, well, there will be such kind of a problem with cumulative objects and collections, so cumulative collections color should be user-defined as well, separately from the main palette swatches, but as part of the entire palette.

@Paul Kotelevets (1D_Inc) This patch is for the UI layout fix, not the use case of collection colors. If you want to discuss collection color more look at https://developer.blender.org/T77777 or https://devtalk.blender.org/t/gsoc-2020-outliner-discussion-and-suggestions/13178.

The colors are themeable in the user preferences with defaults as shown in the image. Right now the specific defaults are semi-random and not organized well. I'll get that fixed up before the final commit, but it's not urgent.

In D8317#203963, @Zachman wrote:

@Paul Kotelevets (1D_Inc) This patch is for the UI layout fix, not the use case of collection colors. If you want to discuss collection color more look at https://developer.blender.org/T77777 or https://devtalk.blender.org/t/gsoc-2020-outliner-discussion-and-suggestions/13178.

Got it. I thought that it will possibly influense the UI layout, so I mentioned it here. I will post it there.

The colors are themeable in the user preferences with defaults as shown in the image. Right now the specific defaults are semi-random and not organized well. I'll get that fixed up before the final commit, but it's not urgent.

Thank you for the answer!

Could you split the icon-only addition into a separate patch from the layout fixes? The former should be ready to go into master, but not entirely sure which changes belong to which.

source/blender/editors/interface/interface_layout.c
1437

Note for myself: Layout specific button placement, should be easy to break.

@Julian Eisel (Severin) I hope I understand what you asked. I moved all the icon_only for operator_enum code to D8880: UI: Add icon_only argument to operator_enum

The thinkcode here (for fixing the layout of inline menu icons is okay. I'm super unfamiliar with this area, and I just changed small things while single-stepping over the course of a week until it drew as desired. There might be a more efficient or better way of fixing it.

Remove unrelated changes

Julian Eisel (Severin) requested changes to this revision.Sep 14 2020, 9:53 PM

I looked into this for a while and noticed some issues. The following should fix them and removes some unnecessary changes (did this on top of soc-2020-outliner).
With that applied, I think this patch is ready.

diff --git a/source/blender/editors/interface/interface.c b/source/blender/editors/interface/interface.c
index 4a68d7f861c..18a0ef1d95b 100644
--- a/source/blender/editors/interface/interface.c
+++ b/source/blender/editors/interface/interface.c
@@ -365,6 +365,12 @@ void UI_block_translate(uiBlock *block, int x, int y)
   BLI_rctf_translate(&block->rect, x, y);
 }
 
+static bool ui_but_is_row_alignment_group(const uiBut *left, const uiBut *right)
+{
+  const bool is_same_align_group = (left->alignnr && (left->alignnr == right->alignnr));
+  return is_same_align_group && (left->rect.xmin < right->rect.xmin);
+}
+
 static void ui_block_bounds_calc_text(uiBlock *block, float offset)
 {
   const uiStyle *style = UI_style_get();
@@ -383,27 +389,32 @@ static void ui_block_bounds_calc_text(uiBlock *block, float offset)
       }
     }
 
-    /* Keep aligned buttons in the same column. */
-    if (bt->alignnr && bt->next) {
+    /* Skip all buttons that are in a horizontal alignment group.
+     * We don't want to split them appart (but still check the row's width and apply current
+     * offsets). */
+    if (bt->next && ui_but_is_row_alignment_group(bt, bt->next)) {
       int width = 0;
-      for (col_bt = bt; col_bt->rect.xmin < col_bt->next->rect.xmin; col_bt = col_bt->next) {
+      int alignnr = bt->alignnr;
+      for (col_bt = bt; col_bt && col_bt->alignnr == alignnr; col_bt = col_bt->next) {
         width += BLI_rctf_size_x(&col_bt->rect);
+        col_bt->rect.xmin += x1addval;
+        col_bt->rect.xmax += x1addval;
       }
       if (width > i) {
         i = width;
       }
-      bt = col_bt;
+      /* Give the following code the last button in the alignment group, there might have to be a
+       * split immediately after. */
+      bt = col_bt ? col_bt->prev : NULL;
     }
 
-    if (bt->next && bt->rect.xmin < bt->next->rect.xmin) {
+    if (bt && bt->next && bt->rect.xmin < bt->next->rect.xmin) {
       /* End of this column, and it's not the last one. */
       for (col_bt = init_col_bt; col_bt->prev != bt; col_bt = col_bt->next) {
-        if (!col_bt->alignnr) {
-          col_bt->rect.xmin = x1addval;
-          col_bt->rect.xmax = x1addval + i + block->bounds;
+        col_bt->rect.xmin = x1addval;
+        col_bt->rect.xmax = x1addval + i + block->bounds;
 
-          ui_but_update(col_bt); /* clips text again */
-        }
+        ui_but_update(col_bt); /* clips text again */
       }
 
       /* And we prepare next column. */
@@ -415,11 +426,20 @@ static void ui_block_bounds_calc_text(uiBlock *block, float offset)
 
   /* Last column. */
   for (col_bt = init_col_bt; col_bt; col_bt = col_bt->next) {
-    if (!col_bt->alignnr) {
-      col_bt->rect.xmin = x1addval;
-      col_bt->rect.xmax = max_ff(x1addval + i + block->bounds, offset + block->minbounds);
+    /* Recognize a horizontally arranged alignment group and skip its items. */
+    if (col_bt->next && ui_but_is_row_alignment_group(col_bt, col_bt->next)) {
+      int alignnr = col_bt->alignnr;
+      for (; col_bt && col_bt->alignnr == alignnr; col_bt = col_bt->next) {
+        /* pass */
+      }
+    }
+    if (!col_bt) {
+      break;
     }
 
+    col_bt->rect.xmin = x1addval;
+    col_bt->rect.xmax = max_ff(x1addval + i + block->bounds, offset + block->minbounds);
+
     ui_but_update(col_bt); /* clips text again */
   }
 }
diff --git a/source/blender/editors/interface/interface_layout.c b/source/blender/editors/interface/interface_layout.c
index f19596c7ba7..293063d6c5c 100644
--- a/source/blender/editors/interface/interface_layout.c
+++ b/source/blender/editors/interface/interface_layout.c
@@ -1432,12 +1432,25 @@ void uiItemsFullEnumO_items(uiLayout *layout,
   if (radial) {
     target = uiLayoutRadial(layout);
   }
-  else if (layout->item.type == ITEM_LAYOUT_ROW && flag & UI_ITEM_R_ICON_ONLY) {
+  else if ((uiLayoutGetLocalDir(layout) == UI_LAYOUT_HORIZONTAL) && (flag & UI_ITEM_R_ICON_ONLY)) {
     target = layout;
+    UI_block_layout_set_current(block, target);
 
     /* Add a blank button to the beginning of the row. */
-    uiDefIconBut(
-        block, UI_BTYPE_LABEL, 0, ICON_BLANK1, 0, 0, UI_UNIT_X, UI_UNIT_Y, NULL, 0, 0, 0, 0, NULL);
+    uiDefIconBut(block,
+                 UI_BTYPE_LABEL,
+                 0,
+                 ICON_BLANK1,
+                 0,
+                 0,
+                 1.25f * UI_UNIT_X,
+                 UI_UNIT_Y,
+                 NULL,
+                 0,
+                 0,
+                 0,
+                 0,
+                 NULL);
   }
   else {
     split = uiLayoutSplit(layout, 0.0f, false);
@@ -1501,7 +1514,7 @@ void uiItemsFullEnumO_items(uiLayout *layout,
       if (item->name) {
         uiBut *but;
 
-        if (item != item_array && !radial) {
+        if (item != item_array && !radial && split) {
           target = uiLayoutColumn(split, layout->align);
 
           /* inconsistent, but menus with labels do not look good flipped */
@@ -3741,18 +3754,18 @@ static void ui_litem_estimate_column(uiLayout *litem, bool is_box)
   }
 }
 
-static void ui_litem_layout_column(uiLayout *litem, bool is_box, bool is_menu)
+static void ui_litem_layout_column(uiLayout *litem, bool is_box)
 {
-  int itemw, itemh, x, y;
+  int itemh, x, y;
 
   x = litem->x;
   y = litem->y;
 
   LISTBASE_FOREACH (uiItem *, item, &litem->items) {
-    ui_item_size(item, &itemw, &itemh);
+    ui_item_size(item, NULL, &itemh);
 
     y -= itemh;
-    ui_item_position(item, x, y, is_menu ? itemw : litem->w, itemh);
+    ui_item_position(item, x, y, litem->w, itemh);
 
     if (item->next && (!is_box || item != litem->items.first)) {
       y -= litem->space;
@@ -3913,11 +3926,8 @@ static void ui_litem_layout_root(uiLayout *litem)
   else if (litem->root->type == UI_LAYOUT_PIEMENU) {
     ui_litem_layout_root_radial(litem);
   }
-  else if (litem->root->type == UI_LAYOUT_MENU) {
-    ui_litem_layout_column(litem, false, true);
-  }
   else {
-    ui_litem_layout_column(litem, false, false);
+    ui_litem_layout_column(litem, false);
   }
 }
 
@@ -3961,7 +3971,7 @@ static void ui_litem_layout_box(uiLayout *litem)
     litem->h -= 2 * boxspace;
   }
 
-  ui_litem_layout_column(litem, true, false);
+  ui_litem_layout_column(litem, true);
 
   litem->x -= boxspace;
   litem->y -= boxspace;
@@ -5222,7 +5232,7 @@ static void ui_item_layout(uiItem *item)
 
     switch (litem->item.type) {
       case ITEM_LAYOUT_COLUMN:
-        ui_litem_layout_column(litem, false, false);
+        ui_litem_layout_column(litem, false);
         break;
       case ITEM_LAYOUT_COLUMN_FLOW:
         ui_litem_layout_column_flow(litem);
This revision now requires changes to proceed.Sep 14 2020, 9:53 PM

I had a comment here from last saturday that I forgot to submit. Oops.

Once this and D8880: UI: Add icon_only argument to operator_enum are committed I will clean up D8622: Collections: Add color tagging and use in outliner to remove all unrelated changes.

This revision is now accepted and ready to land.Sep 15 2020, 1:00 PM