UI: Add title to tree view context menus #120694

Open
Pratik Borhade wants to merge 6 commits from PratikPB2123/blender:bcol-ctxmenu-header into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
Member

Add new string member in AbstractView class to store title/header of
tree view. Later use this to set title (pup->title) of context menu.

Main PR
image image
Add new string member in `AbstractView` class to store title/header of tree view. Later use this to set title (`pup->title`) of context menu. | Main | PR | | -- | -- | | ![image](/attachments/cdf1df8e-12b2-4039-8ccf-c5c96b0cc1f8) | ![image](/attachments/7382fc84-e8a6-4808-9545-a3e5d1a00eaa) |
Pratik Borhade added 1 commit 2024-04-16 13:34:21 +02:00
c2e9ec9b1c UI: bone collection context menu improvements
Add header in context menu. The extra separator is only useful when
"developer extras" is enabled to separate "edit source" operator.
Pratik Borhade requested review from Pablo Vazquez 2024-04-16 13:34:40 +02:00
Pratik Borhade requested review from Sybren A. Stüvel 2024-04-16 13:34:40 +02:00
Author
Member

Hi, noticed one more thing. If we right click visibility/solo toggle then context menu shows these bone collection operations which seems wrong to me. Want to me to handle it in this PR? (just one condition of UI_BTYPE_VIEW_ITEM will fix this)

Hi, noticed one more thing. If we right click visibility/solo toggle then context menu shows [these bone collection operations](/attachments/01b3707d-3d7c-41e6-a186-202d5e374aa9) which seems wrong to me. Want to me to handle it in this PR? (just one condition of `UI_BTYPE_VIEW_ITEM` will fix this)
Pratik Borhade added the
Module
User Interface
label 2024-04-16 13:38:36 +02:00
Member

Add header in context menu.
image

When titles don't have icons, they shouldn't be indented, e.g.:
context menu


The extra separator is only useful when "developer extras" is enabled to separate "edit source" operator.

I think this applies to other menus not just Bone Collections, so perhaps better as a separate patch?

> Add header in context menu. > ![image](/attachments/81862914-269c-422a-a583-39b27bdafd1c) When titles don't have icons, they shouldn't be indented, e.g.: ![context menu](/attachments/6f1f61e1-0931-4532-be64-725cd7e225ea) ---- > The extra separator is only useful when "developer extras" is enabled to separate "edit source" operator. I think this applies to other menus not just Bone Collections, so perhaps better as a separate patch?
Author
Member

When titles don't have icons, they shouldn't be indented

Hi, for that I think we need to set pup->title. I'm not sure how we can obtain bone collection panel name from the button. Is it possible in any way? 🤔

> When titles don't have icons, they shouldn't be indented Hi, for that I think we need to set `pup->title`. I'm not sure how we can obtain bone collection panel name from the button. Is it possible in any way? 🤔
Member

for that I think we need to set pup->title. I'm not sure how we can obtain bone collection panel name from the button. Is it possible in any way? 🤔

I'm not sure, @Harley might know.

> for that I think we need to set `pup->title`. I'm not sure how we can obtain bone collection panel name from the button. Is it possible in any way? 🤔 I'm not sure, @Harley might know.
Pratik Borhade added 2 commits 2024-04-25 13:18:11 +02:00
Pratik Borhade changed title from UI: bone collection context menu improvements to UI: Bone collection context menu header 2024-04-25 13:18:40 +02:00
Author
Member

@pablovazquez hi, updated to PR and description. This should work now.

@pablovazquez hi, updated to PR and description. This should work now.
Member

Looks good! However, looking at the code and it seems the only part that makes this a "Bone Collection" change is adding the label there.

Wouldn't it be better to just add the label all other tree views (Light Linking, Grease Pencil, Asset Browser/Shelf Catalogs, Spreadsheet) and make this patch a more general "Add title label to tree view context menus"? Note: In that case the reviewer should be Julian as he's the author of Tree Views.

Some are not yet populated, they only show "Edit Source" if Developer Extras is enabled, or not show at all since they're empty. But it doesn't hurt to have the labels already. Also better for future tree views when developers copy-paste this part of the code they remember to add it :)

Catalogs:

Before After
before after

Grease Pencil
Light Linking

diff --git forkSrcPrefix/source/blender/editors/space_file/asset_catalog_tree_view.cc forkDstPrefix/source/blender/editors/space_file/asset_catalog_tree_view.cc
index 5f2b5f03be6c7e3e732d2926a1a15a063e0805c1..942af1a8f1c35882fcffc7be19a0204006ac0a7e 100644
--- forkSrcPrefix/source/blender/editors/space_file/asset_catalog_tree_view.cc
+++ forkDstPrefix/source/blender/editors/space_file/asset_catalog_tree_view.cc
@@ -767,6 +767,7 @@ void file_create_asset_catalog_tree_view_in_layout(asset_system::AssetLibrary *a
       "asset catalog tree view",
       std::make_unique<ed::asset_browser::AssetCatalogTreeView>(
           asset_library, params, *space_file));
+  tree_view->label = "Catalog";
 
   ui::TreeViewBuilder::build_tree_view(*tree_view, *layout);
 }
diff --git forkSrcPrefix/source/blender/editors/interface/templates/interface_template_grease_pencil_layer_tree.cc forkDstPrefix/source/blender/editors/interface/templates/interface_template_grease_pencil_layer_tree.cc
index 316e237dea29663247931c4063319c185e8ae3ee..2cd2a58581a39e41df1246d42d8b491c68556ab2 100644
--- forkSrcPrefix/source/blender/editors/interface/templates/interface_template_grease_pencil_layer_tree.cc
+++ forkDstPrefix/source/blender/editors/interface/templates/interface_template_grease_pencil_layer_tree.cc
@@ -417,6 +417,7 @@ void uiTemplateGreasePencilLayerTree(uiLayout *layout, bContext *C)
       *block,
       "Grease Pencil Layer Tree View",
       std::make_unique<blender::ui::greasepencil::LayerTreeView>(grease_pencil));
+  tree_view->label = "Grease Pencil Layer";
   tree_view->set_min_rows(3);
 
   ui::TreeViewBuilder::build_tree_view(*tree_view, *layout);
diff --git forkSrcPrefix/source/blender/editors/space_spreadsheet/spreadsheet_dataset_draw.cc forkDstPrefix/source/blender/editors/space_spreadsheet/spreadsheet_dataset_draw.cc
index 3dce9fde89b936dce3020f0db042d7feedc5d2c6..8a0421ac5f394e6676a30828f644bc3dbac17f32 100644
--- forkSrcPrefix/source/blender/editors/space_spreadsheet/spreadsheet_dataset_draw.cc
+++ forkDstPrefix/source/blender/editors/space_spreadsheet/spreadsheet_dataset_draw.cc
@@ -318,6 +318,7 @@ void spreadsheet_data_set_panel_draw(const bContext *C, Panel *panel)
       "Data Set Tree View",
       std::make_unique<GeometryDataSetTreeView>(
           spreadsheet_get_display_geometry_set(sspreadsheet, object), *C));
+  tree_view->label = "Spreadsheet";
 
   ui::TreeViewBuilder::build_tree_view(*tree_view, *layout);
 }
diff --git forkSrcPrefix/source/blender/editors/asset/intern/asset_shelf_catalog_selector.cc forkDstPrefix/source/blender/editors/asset/intern/asset_shelf_catalog_selector.cc
index da9e3b189583a8965120abcfdcb230d69e8f97af..97625d56e2c54e47703488efe45df60cbf61d04c 100644
--- forkSrcPrefix/source/blender/editors/asset/intern/asset_shelf_catalog_selector.cc
+++ forkDstPrefix/source/blender/editors/asset/intern/asset_shelf_catalog_selector.cc
@@ -205,6 +205,7 @@ static void catalog_selector_panel_draw(const bContext *C, Panel *panel)
       *block,
       "asset catalog tree view",
       std::make_unique<AssetCatalogSelectorTree>(*library, *shelf));
+  tree_view->label = "Catalog";
 
   ui::TreeViewBuilder::build_tree_view(*tree_view, *layout);
 }
diff --git forkSrcPrefix/source/blender/editors/interface/templates/interface_template_light_linking.cc forkDstPrefix/source/blender/editors/interface/templates/interface_template_light_linking.cc
index 77bda54c236f95d21b5df0d44c29c14df8648d51..bb69a5d2376a1cb63fcccdf48a894f45af6669f7 100644
--- forkSrcPrefix/source/blender/editors/interface/templates/interface_template_light_linking.cc
+++ forkDstPrefix/source/blender/editors/interface/templates/interface_template_light_linking.cc
@@ -399,6 +399,7 @@ void uiTemplateLightLinkingCollection(uiLayout *layout,
       *block,
       "Light Linking Collection Tree View",
       std::make_unique<blender::ui::light_linking::CollectionView>(*context_layout, *collection));
+  tree_view->label = "Light Linking";
   tree_view->set_min_rows(3);
 
   blender::ui::TreeViewBuilder::build_tree_view(*tree_view, *layout);
Looks good! However, looking at the code and it seems the only part that makes this a "Bone Collection" change is adding the label there. Wouldn't it be better to just add the label all other tree views (Light Linking, Grease Pencil, Asset Browser/Shelf Catalogs, Spreadsheet) and make this patch a more general "Add title label to tree view context menus"? Note: In that case the reviewer should be Julian as he's the author of Tree Views. Some are not yet populated, they only show "Edit Source" if Developer Extras is enabled, or not show at all since they're empty. But it doesn't hurt to have the labels already. Also better for future tree views when developers copy-paste this part of the code they remember to add it :) Catalogs: |Before|After| |----|----| |![before](/attachments/5b2d79a7-80c9-44f2-91e0-d45307c15c5c)|![after](/attachments/c02d4a3a-44ad-4e28-b312-9251760c1586)| ![Grease Pencil](/attachments/6c26ff8a-4592-4e96-8edc-7fd40073e1e8) ![Light Linking](/attachments/ab87ed68-33cc-446a-a15d-b6d866c4e1c0) ```diff diff --git forkSrcPrefix/source/blender/editors/space_file/asset_catalog_tree_view.cc forkDstPrefix/source/blender/editors/space_file/asset_catalog_tree_view.cc index 5f2b5f03be6c7e3e732d2926a1a15a063e0805c1..942af1a8f1c35882fcffc7be19a0204006ac0a7e 100644 --- forkSrcPrefix/source/blender/editors/space_file/asset_catalog_tree_view.cc +++ forkDstPrefix/source/blender/editors/space_file/asset_catalog_tree_view.cc @@ -767,6 +767,7 @@ void file_create_asset_catalog_tree_view_in_layout(asset_system::AssetLibrary *a "asset catalog tree view", std::make_unique<ed::asset_browser::AssetCatalogTreeView>( asset_library, params, *space_file)); + tree_view->label = "Catalog"; ui::TreeViewBuilder::build_tree_view(*tree_view, *layout); } diff --git forkSrcPrefix/source/blender/editors/interface/templates/interface_template_grease_pencil_layer_tree.cc forkDstPrefix/source/blender/editors/interface/templates/interface_template_grease_pencil_layer_tree.cc index 316e237dea29663247931c4063319c185e8ae3ee..2cd2a58581a39e41df1246d42d8b491c68556ab2 100644 --- forkSrcPrefix/source/blender/editors/interface/templates/interface_template_grease_pencil_layer_tree.cc +++ forkDstPrefix/source/blender/editors/interface/templates/interface_template_grease_pencil_layer_tree.cc @@ -417,6 +417,7 @@ void uiTemplateGreasePencilLayerTree(uiLayout *layout, bContext *C) *block, "Grease Pencil Layer Tree View", std::make_unique<blender::ui::greasepencil::LayerTreeView>(grease_pencil)); + tree_view->label = "Grease Pencil Layer"; tree_view->set_min_rows(3); ui::TreeViewBuilder::build_tree_view(*tree_view, *layout); diff --git forkSrcPrefix/source/blender/editors/space_spreadsheet/spreadsheet_dataset_draw.cc forkDstPrefix/source/blender/editors/space_spreadsheet/spreadsheet_dataset_draw.cc index 3dce9fde89b936dce3020f0db042d7feedc5d2c6..8a0421ac5f394e6676a30828f644bc3dbac17f32 100644 --- forkSrcPrefix/source/blender/editors/space_spreadsheet/spreadsheet_dataset_draw.cc +++ forkDstPrefix/source/blender/editors/space_spreadsheet/spreadsheet_dataset_draw.cc @@ -318,6 +318,7 @@ void spreadsheet_data_set_panel_draw(const bContext *C, Panel *panel) "Data Set Tree View", std::make_unique<GeometryDataSetTreeView>( spreadsheet_get_display_geometry_set(sspreadsheet, object), *C)); + tree_view->label = "Spreadsheet"; ui::TreeViewBuilder::build_tree_view(*tree_view, *layout); } diff --git forkSrcPrefix/source/blender/editors/asset/intern/asset_shelf_catalog_selector.cc forkDstPrefix/source/blender/editors/asset/intern/asset_shelf_catalog_selector.cc index da9e3b189583a8965120abcfdcb230d69e8f97af..97625d56e2c54e47703488efe45df60cbf61d04c 100644 --- forkSrcPrefix/source/blender/editors/asset/intern/asset_shelf_catalog_selector.cc +++ forkDstPrefix/source/blender/editors/asset/intern/asset_shelf_catalog_selector.cc @@ -205,6 +205,7 @@ static void catalog_selector_panel_draw(const bContext *C, Panel *panel) *block, "asset catalog tree view", std::make_unique<AssetCatalogSelectorTree>(*library, *shelf)); + tree_view->label = "Catalog"; ui::TreeViewBuilder::build_tree_view(*tree_view, *layout); } diff --git forkSrcPrefix/source/blender/editors/interface/templates/interface_template_light_linking.cc forkDstPrefix/source/blender/editors/interface/templates/interface_template_light_linking.cc index 77bda54c236f95d21b5df0d44c29c14df8648d51..bb69a5d2376a1cb63fcccdf48a894f45af6669f7 100644 --- forkSrcPrefix/source/blender/editors/interface/templates/interface_template_light_linking.cc +++ forkDstPrefix/source/blender/editors/interface/templates/interface_template_light_linking.cc @@ -399,6 +399,7 @@ void uiTemplateLightLinkingCollection(uiLayout *layout, *block, "Light Linking Collection Tree View", std::make_unique<blender::ui::light_linking::CollectionView>(*context_layout, *collection)); + tree_view->label = "Light Linking"; tree_view->set_min_rows(3); blender::ui::TreeViewBuilder::build_tree_view(*tree_view, *layout); ```
Pratik Borhade changed title from UI: Bone collection context menu header to UI: Add title to tree view context menus 2024-04-26 13:08:23 +02:00
Pratik Borhade added 1 commit 2024-04-26 13:10:24 +02:00
Author
Member

@pablovazquez thanks, done :)

@pablovazquez thanks, done :)
Pratik Borhade requested review from Julian Eisel 2024-04-26 13:11:41 +02:00
Pablo Vazquez approved these changes 2024-04-26 14:36:35 +02:00
Julian Eisel requested changes 2024-04-26 18:19:01 +02:00
@ -206,3 +206,3 @@
"asset catalog tree view",
std::make_unique<AssetCatalogSelectorTree>(*library, *shelf));
tree_view->label = "Catalog";
Member

These strings need translation.

These strings need translation.
@ -66,3 +66,3 @@
public:
virtual ~AbstractView() = default;
std::string label;
Member

I'd avoid public members in classes like this generally, plus I'd avoid mixing public and non-public members scattered throughout the class. Just add this to the other private members at the top of the struct definition, and add a public getter/setter. label is also a bit misleading maybe. I'd just call it "context menu title", for as long as it's just that at least.

I'd avoid public members in classes like this generally, plus I'd avoid mixing public and non-public members scattered throughout the class. Just add this to the other private members at the top of the struct definition, and add a public getter/setter. `label` is also a bit misleading maybe. I'd just call it "context menu title", for as long as it's just that at least.
@ -6422,2 +6422,4 @@
return but.str.substr(0, str_len);
}
if (but.type == UI_BTYPE_VIEW_ITEM) {
Member

This isn't semantically correct, the function is called UI_but_string_get_label() but here we don't return the button label, but the label we want to use for the popup. In other cases we may also want to query the label and don't expect to get this string in case of view item buttons (we already do actually).

Suggestion: Add a UI_but_context_menu_title_from_button() which can handle the view-item case and returns UI_but_string_get_label() for other cases. This can be called by ui_popup_context_menu_for_button().

This isn't semantically correct, the function is called `UI_but_string_get_label()` but here we don't return the button label, but the label we want to use for the popup. In other cases we may also want to query the label and don't expect to get this string in case of view item buttons (we already do actually). Suggestion: Add a `UI_but_context_menu_title_from_button()` which can handle the view-item case and returns `UI_but_string_get_label()` for other cases. This can be called by `ui_popup_context_menu_for_button()`.
Pratik Borhade added 2 commits 2024-05-03 06:31:46 +02:00
This pull request can be merged automatically.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u bcol-ctxmenu-header:PratikPB2123-bcol-ctxmenu-header
git checkout PratikPB2123-bcol-ctxmenu-header
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#120694
No description provided.