UI: Improve node group input / output list interface

This commit replaces the two-column list for editing node group inputs
and outputs with a cleaner solution with two panels. The new layout
has several benefits:
 - It uses the vertical space in the node editor sidebar better.
 - It should be more familiar because of similarity with other UI lists.
 - It should look better with consistent alignment and icons.

Note that displaying the "Name" property outside of the list itself is
a bit inconsistent and could possibly be removed in the future.

Differential Revision: https://developer.blender.org/D9683

3 files changed, 130 insertions, 145 deletions

diff --git a/source/blender/editors/space_node/drawnode.c b/source/blender/editors/space_node/drawnode.c
index 1333c9ed215..4b5ba2af050 100644
--- a/source/blender/editors/space_node/drawnode.c
+++ b/source/blender/editors/space_node/drawnode.c
@@ -3665,37 +3665,35 @@ static void std_node_socket_interface_draw(bContext *UNUSED(C), uiLayout *layout
 {
   bNodeSocket *sock = ptr->data;
   int type = sock->typeinfo->type;
-  /*int subtype = sock->typeinfo->subtype;*/
+
+  uiLayout *col = uiLayoutColumn(layout, false);
 
   switch (type) {
     case SOCK_FLOAT: {
-      uiLayout *row;
-      uiItemR(layout, ptr, "default_value", DEFAULT_FLAGS, NULL, 0);
-      row = uiLayoutRow(layout, true);
-      uiItemR(row, ptr, "min_value", DEFAULT_FLAGS, IFACE_("Min"), 0);
-      uiItemR(row, ptr, "max_value", DEFAULT_FLAGS, IFACE_("Max"), 0);
+      uiItemR(col, ptr, "default_value", DEFAULT_FLAGS, IFACE_("Default"), ICON_NONE);
+      uiLayout *sub = uiLayoutColumn(col, true);
+      uiItemR(sub, ptr, "min_value", DEFAULT_FLAGS, IFACE_("Min"), ICON_NONE);
+      uiItemR(sub, ptr, "max_value", DEFAULT_FLAGS, IFACE_("Max"), ICON_NONE);
       break;
     }
     case SOCK_INT: {
-      uiLayout *row;
-      uiItemR(layout, ptr, "default_value", DEFAULT_FLAGS, NULL, 0);
-      row = uiLayoutRow(layout, true);
-      uiItemR(row, ptr, "min_value", DEFAULT_FLAGS, IFACE_("Min"), 0);
-      uiItemR(row, ptr, "max_value", DEFAULT_FLAGS, IFACE_("Max"), 0);
+      uiItemR(col, ptr, "default_value", DEFAULT_FLAGS, IFACE_("Default"), ICON_NONE);
+      uiLayout *sub = uiLayoutColumn(col, true);
+      uiItemR(sub, ptr, "min_value", DEFAULT_FLAGS, IFACE_("Min"), ICON_NONE);
+      uiItemR(sub, ptr, "max_value", DEFAULT_FLAGS, IFACE_("Max"), ICON_NONE);
       break;
     }
     case SOCK_VECTOR: {
-      uiLayout *row;
-      uiItemR(layout, ptr, "default_value", UI_ITEM_R_EXPAND, NULL, DEFAULT_FLAGS);
-      row = uiLayoutRow(layout, true);
-      uiItemR(row, ptr, "min_value", DEFAULT_FLAGS, IFACE_("Min"), 0);
-      uiItemR(row, ptr, "max_value", DEFAULT_FLAGS, IFACE_("Max"), 0);
+      uiItemR(col, ptr, "default_value", UI_ITEM_R_EXPAND, IFACE_("Default"), ICON_NONE);
+      uiLayout *sub = uiLayoutColumn(col, true);
+      uiItemR(sub, ptr, "min_value", DEFAULT_FLAGS, IFACE_("Min"), ICON_NONE);
+      uiItemR(sub, ptr, "max_value", DEFAULT_FLAGS, IFACE_("Max"), ICON_NONE);
       break;
     }
     case SOCK_BOOLEAN:
     case SOCK_RGBA:
     case SOCK_STRING: {
-      uiItemR(layout, ptr, "default_value", DEFAULT_FLAGS, NULL, 0);
+      uiItemR(col, ptr, "default_value", DEFAULT_FLAGS, IFACE_("Default"), 0);
       break;
     }
   }
diff --git a/source/blender/editors/space_node/node_buttons.c b/source/blender/editors/space_node/node_buttons.c
index 0aba45ceafc..c9a0c827a09 100644
--- a/source/blender/editors/space_node/node_buttons.c
+++ b/source/blender/editors/space_node/node_buttons.c
@@ -93,59 +93,35 @@ static bool node_tree_interface_poll(const bContext *C, PanelType *UNUSED(pt))
           (snode->edittree->inputs.first || snode->edittree->outputs.first));
 }
 
-static bool node_tree_find_active_socket(bNodeTree *ntree,
-                                         bNodeSocket **r_sock,
-                                         eNodeSocketInOut *r_in_out)
+static bNodeSocket *node_tree_find_active_socket(bNodeTree *ntree, const eNodeSocketInOut in_out)
 {
-  LISTBASE_FOREACH (bNodeSocket *, socket, &ntree->inputs) {
+  ListBase *sockets = (in_out == SOCK_IN) ? &ntree->inputs : &ntree->outputs;
+  LISTBASE_FOREACH (bNodeSocket *, socket, sockets) {
     if (socket->flag & SELECT) {
-      *r_sock = socket;
-      *r_in_out = SOCK_IN;
-      return true;
+      return socket;
     }
   }
-  LISTBASE_FOREACH (bNodeSocket *, socket, &ntree->outputs) {
-    if (socket->flag & SELECT) {
-      *r_sock = socket;
-      *r_in_out = SOCK_OUT;
-      return true;
-    }
-  }
-
-  *r_sock = NULL;
-  *r_in_out = 0;
-  return false;
+  return NULL;
 }
 
-static void node_tree_interface_panel(const bContext *C, Panel *panel)
+static void draw_socket_list(const bContext *C,
+                             uiLayout *layout,
+                             bNodeTree *ntree,
+                             const eNodeSocketInOut in_out)
 {
-  SpaceNode *snode = CTX_wm_space_node(C); /* NULL checked in poll function. */
-  bNodeTree *ntree = snode->edittree;      /* NULL checked in poll function. */
-  uiLayout *layout = panel->layout;
-
-  PointerRNA ptr;
-  RNA_id_pointer_create((ID *)ntree, &ptr);
-
-  bNodeSocket *socket;
-  eNodeSocketInOut in_out;
-  node_tree_find_active_socket(ntree, &socket, &in_out);
-  PointerRNA sockptr;
-  RNA_pointer_create((ID *)ntree, &RNA_NodeSocketInterface, socket, &sockptr);
-
-  uiLayout *row = uiLayoutRow(layout, false);
+  PointerRNA tree_ptr;
+  RNA_id_pointer_create((ID *)ntree, &tree_ptr);
 
-  uiLayout *split = uiLayoutRow(row, true);
-  uiLayout *col = uiLayoutColumn(split, true);
-  wmOperatorType *ot = WM_operatortype_find("NODE_OT_tree_socket_add", false);
-  uiItemL(col, IFACE_("Inputs:"), ICON_NONE);
-  uiTemplateList(col,
+  uiLayout *split = uiLayoutRow(layout, false);
+  uiLayout *list_col = uiLayoutColumn(split, true);
+  uiTemplateList(list_col,
                  (bContext *)C,
                  "NODE_UL_interface_sockets",
-                 "inputs",
-                 &ptr,
-                 "inputs",
-                 &ptr,
-                 "active_input",
+                 (in_out == SOCK_IN) ? "inputs" : "outputs",
+                 &tree_ptr,
+                 (in_out == SOCK_IN) ? "inputs" : "outputs",
+                 &tree_ptr,
+                 (in_out == SOCK_IN) ? "active_input" : "active_output",
                  NULL,
                  0,
                  0,
@@ -154,70 +130,90 @@ static void node_tree_interface_panel(const bContext *C, Panel *panel)
                  false,
                  false);
   PointerRNA opptr;
-  uiItemFullO_ptr(col, ot, "", ICON_PLUS, NULL, WM_OP_EXEC_DEFAULT, 0, &opptr);
-  RNA_enum_set(&opptr, "in_out", SOCK_IN);
+  uiLayout *ops_col = uiLayoutColumn(split, false);
+  uiLayout *add_remove_col = uiLayoutColumn(ops_col, true);
+  wmOperatorType *ot = WM_operatortype_find("NODE_OT_tree_socket_add", false);
+  uiItemFullO_ptr(add_remove_col, ot, "", ICON_ADD, NULL, WM_OP_EXEC_DEFAULT, 0, &opptr);
+  RNA_enum_set(&opptr, "in_out", in_out);
+  ot = WM_operatortype_find("NODE_OT_tree_socket_remove", false);
+  uiItemFullO_ptr(add_remove_col, ot, "", ICON_REMOVE, NULL, WM_OP_EXEC_DEFAULT, 0, &opptr);
+  RNA_enum_set(&opptr, "in_out", in_out);
 
-  col = uiLayoutColumn(split, true);
-  uiItemL(col, IFACE_("Outputs:"), ICON_NONE);
-  uiTemplateList(col,
-                 (bContext *)C,
-                 "NODE_UL_interface_sockets",
-                 "outputs",
-                 &ptr,
-                 "outputs",
-                 &ptr,
-                 "active_output",
-                 NULL,
-                 0,
-                 0,
-                 0,
-                 0,
-                 false,
-                 false);
-  uiItemFullO_ptr(col, ot, "", ICON_PLUS, NULL, WM_OP_EXEC_DEFAULT, 0, &opptr);
-  RNA_enum_set(&opptr, "in_out", SOCK_OUT);
+  uiItemS(ops_col);
 
+  uiLayout *up_down_col = uiLayoutColumn(ops_col, true);
   ot = WM_operatortype_find("NODE_OT_tree_socket_move", false);
-  col = uiLayoutColumn(row, true);
-  uiItemFullO_ptr(col, ot, "", ICON_TRIA_UP, NULL, WM_OP_EXEC_DEFAULT, 0, &opptr);
+  uiItemFullO_ptr(up_down_col, ot, "", ICON_TRIA_UP, NULL, WM_OP_EXEC_DEFAULT, 0, &opptr);
   RNA_enum_set(&opptr, "direction", 1);
-  uiItemFullO_ptr(col, ot, "", ICON_TRIA_DOWN, NULL, WM_OP_EXEC_DEFAULT, 0, &opptr);
+  RNA_enum_set(&opptr, "in_out", in_out);
+  uiItemFullO_ptr(up_down_col, ot, "", ICON_TRIA_DOWN, NULL, WM_OP_EXEC_DEFAULT, 0, &opptr);
   RNA_enum_set(&opptr, "direction", 2);
+  RNA_enum_set(&opptr, "in_out", in_out);
 
-  if (socket) {
-    row = uiLayoutRow(layout, true);
-    uiItemR(row, &sockptr, "name", 0, NULL, ICON_NONE);
-    uiItemO(row, "", ICON_X, "NODE_OT_tree_socket_remove");
+  bNodeSocket *socket = node_tree_find_active_socket(ntree, in_out);
+  if (socket != NULL) {
+    uiLayoutSetPropSep(layout, true);
+    uiLayoutSetPropDecorate(layout, false);
+    PointerRNA socket_ptr;
+    RNA_pointer_create((ID *)ntree, &RNA_NodeSocketInterface, socket, &socket_ptr);
+    uiItemR(layout, &socket_ptr, "name", 0, NULL, ICON_NONE);
 
     if (socket->typeinfo->interface_draw) {
-      uiItemS(layout);
-      socket->typeinfo->interface_draw((bContext *)C, layout, &sockptr);
+      socket->typeinfo->interface_draw((bContext *)C, layout, &socket_ptr);
     }
   }
 }
 
+static void node_tree_interface_inputs_panel(const bContext *C, Panel *panel)
+{
+  SpaceNode *snode = CTX_wm_space_node(C); /* NULL checked in poll function. */
+  bNodeTree *ntree = snode->edittree;      /* NULL checked in poll function. */
+
+  draw_socket_list(C, panel->layout, ntree, SOCK_IN);
+}
+
+static void node_tree_interface_outputs_panel(const bContext *C, Panel *panel)
+{
+  SpaceNode *snode = CTX_wm_space_node(C); /* NULL checked in poll function. */
+  bNodeTree *ntree = snode->edittree;      /* NULL checked in poll function. */
+
+  draw_socket_list(C, panel->layout, ntree, SOCK_OUT);
+}
+
 /* ******************* node buttons registration ************** */
 
 void node_buttons_register(ARegionType *art)
 {
-  PanelType *pt;
-
-  pt = MEM_callocN(sizeof(PanelType), "spacetype node panel node sockets");
-  strcpy(pt->idname, "NODE_PT_sockets");
-  strcpy(pt->category, N_("Node"));
-  strcpy(pt->label, N_("Sockets"));
-  strcpy(pt->translation_context, BLT_I18NCONTEXT_DEFAULT_BPYRNA);
-  pt->draw = node_sockets_panel;
-  pt->poll = node_sockets_poll;
-  pt->flag |= PANEL_TYPE_DEFAULT_CLOSED;
-  BLI_addtail(&art->paneltypes, pt);
-
-  pt = MEM_callocN(sizeof(PanelType), "spacetype node panel tree interface");
-  strcpy(pt->idname, "NODE_PT_node_tree_interface");
-  strcpy(pt->category, N_("Node"));
-  strcpy(pt->label, N_("Interface"));
-  strcpy(pt->translation_context, BLT_I18NCONTEXT_DEFAULT_BPYRNA);
-  pt->draw = node_tree_interface_panel;
-  pt->poll = node_tree_interface_poll;
-  BLI_addtail(&art->paneltypes, pt);
+  {
+    PanelType *pt = MEM_callocN(sizeof(PanelType), __func__);
+    strcpy(pt->idname, "NODE_PT_sockets");
+    strcpy(pt->category, N_("Node"));
+    strcpy(pt->label, N_("Sockets"));
+    strcpy(pt->translation_context, BLT_I18NCONTEXT_DEFAULT_BPYRNA);
+    pt->draw = node_sockets_panel;
+    pt->poll = node_sockets_poll;
+    pt->flag |= PANEL_TYPE_DEFAULT_CLOSED;
+    BLI_addtail(&art->paneltypes, pt);
+  }
+
+  {
+    PanelType *pt = MEM_callocN(sizeof(PanelType), __func__);
+    strcpy(pt->idname, "NODE_PT_node_tree_interface_inputs");
+    strcpy(pt->category, N_("Node"));
+    strcpy(pt->label, N_("Inputs"));
+    strcpy(pt->translation_context, BLT_I18NCONTEXT_DEFAULT_BPYRNA);
+    pt->draw = node_tree_interface_inputs_panel;
+    pt->poll = node_tree_interface_poll;
+    BLI_addtail(&art->paneltypes, pt);
+  }
+  {
+    PanelType *pt = MEM_callocN(sizeof(PanelType), __func__);
+    strcpy(pt->idname, "NODE_PT_node_tree_interface_outputs");
+    strcpy(pt->category, N_("Node"));
+    strcpy(pt->label, N_("Outputs"));
+    strcpy(pt->translation_context, BLT_I18NCONTEXT_DEFAULT_BPYRNA);
+    pt->draw = node_tree_interface_outputs_panel;
+    pt->poll = node_tree_interface_poll;
+    BLI_addtail(&art->paneltypes, pt);
+  }
 }
diff --git a/source/blender/editors/space_node/node_edit.c b/source/blender/editors/space_node/node_edit.c
index 9a1a5f9b8fc..30eee416b12 100644
--- a/source/blender/editors/space_node/node_edit.c
+++ b/source/blender/editors/space_node/node_edit.c
@@ -2227,21 +2227,15 @@ static int ntree_socket_add_exec(bContext *C, wmOperator *op)
 {
   SpaceNode *snode = CTX_wm_space_node(C);
   bNodeTree *ntree = snode->edittree;
-  int in_out = RNA_enum_get(op->ptr, "in_out");
 
   PointerRNA ntree_ptr;
   RNA_id_pointer_create((ID *)ntree, &ntree_ptr);
 
-  const char *default_name;
-  bNodeSocket *active_sock;
-  if (in_out == SOCK_IN) {
-    active_sock = ntree_get_active_interface_socket(&ntree->inputs);
-    default_name = "Input";
-  }
-  else {
-    active_sock = ntree_get_active_interface_socket(&ntree->outputs);
-    default_name = "Output";
-  }
+  const eNodeSocketInOut in_out = RNA_enum_get(op->ptr, "in_out");
+  ListBase *sockets = (in_out == SOCK_IN) ? &ntree->inputs : &ntree->outputs;
+
+  const char *default_name = (in_out == SOCK_IN) ? "Input" : "Output";
+  bNodeSocket *active_sock = ntree_get_active_interface_socket(sockets);
 
   bNodeSocket *sock;
   if (active_sock) {
@@ -2256,11 +2250,8 @@ static int ntree_socket_add_exec(bContext *C, wmOperator *op)
     sock = ntreeAddSocketInterface(ntree, in_out, "NodeSocketFloat", default_name);
   }
 
-  /* deactivate sockets (has to check both lists) */
-  LISTBASE_FOREACH (bNodeSocket *, socket_iter, &ntree->inputs) {
-    socket_iter->flag &= ~SELECT;
-  }
-  LISTBASE_FOREACH (bNodeSocket *, socket_iter, &ntree->outputs) {
+  /* Deactivate sockets. */
+  LISTBASE_FOREACH (bNodeSocket *, socket_iter, sockets) {
     socket_iter->flag &= ~SELECT;
   }
   /* make the new socket active */
@@ -2295,16 +2286,15 @@ void NODE_OT_tree_socket_add(wmOperatorType *ot)
 
 /********************** Remove interface socket operator *********************/
 
-static int ntree_socket_remove_exec(bContext *C, wmOperator *UNUSED(op))
+static int ntree_socket_remove_exec(bContext *C, wmOperator *op)
 {
   SpaceNode *snode = CTX_wm_space_node(C);
   bNodeTree *ntree = snode->edittree;
+  const eNodeSocketInOut in_out = RNA_enum_get(op->ptr, "in_out");
 
-  bNodeSocket *iosock = ntree_get_active_interface_socket(&ntree->inputs);
-  if (!iosock) {
-    iosock = ntree_get_active_interface_socket(&ntree->outputs);
-  }
-  if (!iosock) {
+  bNodeSocket *iosock = ntree_get_active_interface_socket(in_out == SOCK_IN ? &ntree->inputs :
+                                                                              &ntree->outputs);
+  if (iosock == NULL) {
     return OPERATOR_CANCELLED;
   }
 
@@ -2340,6 +2330,7 @@ void NODE_OT_tree_socket_remove(wmOperatorType *ot)
 
   /* flags */
   ot->flag = OPTYPE_REGISTER | OPTYPE_UNDO;
+  RNA_def_enum(ot->srna, "in_out", rna_enum_node_socket_in_out_items, SOCK_IN, "Socket Type", "");
 }
 
 /********************** Move interface socket operator *********************/
@@ -2356,36 +2347,35 @@ static int ntree_socket_move_exec(bContext *C, wmOperator *op)
   bNodeTree *ntree = snode->edittree;
   int direction = RNA_enum_get(op->ptr, "direction");
 
-  ListBase *lb = &ntree->inputs;
-  bNodeSocket *iosock = ntree_get_active_interface_socket(lb);
-  if (!iosock) {
-    lb = &ntree->outputs;
-    iosock = ntree_get_active_interface_socket(lb);
-  }
-  if (!iosock) {
+  const eNodeSocketInOut in_out = RNA_enum_get(op->ptr, "in_out");
+  ListBase *sockets = in_out == SOCK_IN ? &ntree->inputs : &ntree->outputs;
+
+  bNodeSocket *iosock = ntree_get_active_interface_socket(sockets);
+
+  if (iosock == NULL) {
     return OPERATOR_CANCELLED;
   }
 
   switch (direction) {
     case 1: { /* up */
       bNodeSocket *before = iosock->prev;
-      BLI_remlink(lb, iosock);
+      BLI_remlink(sockets, iosock);
       if (before) {
-        BLI_insertlinkbefore(lb, before, iosock);
+        BLI_insertlinkbefore(sockets, before, iosock);
       }
       else {
-        BLI_addhead(lb, iosock);
+        BLI_addhead(sockets, iosock);
       }
       break;
     }
     case 2: { /* down */
       bNodeSocket *after = iosock->next;
-      BLI_remlink(lb, iosock);
+      BLI_remlink(sockets, iosock);
       if (after) {
-        BLI_insertlinkafter(lb, after, iosock);
+        BLI_insertlinkafter(sockets, after, iosock);
       }
       else {
-        BLI_addtail(lb, iosock);
+        BLI_addtail(sockets, iosock);
       }
       break;
     }
@@ -2417,6 +2407,7 @@ void NODE_OT_tree_socket_move(wmOperatorType *ot)
   ot->flag = OPTYPE_REGISTER | OPTYPE_UNDO;
 
   RNA_def_enum(ot->srna, "direction", move_direction_items, 1, "Direction", "");
+  RNA_def_enum(ot->srna, "in_out", rna_enum_node_socket_in_out_items, SOCK_IN, "Socket Type", "");
 }
 
 /* ********************** Shader Script Update ******************/