UI: Cleanup Dialog to Manage Orphaned Data #106653
No reviewers
Labels
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
6 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#106653
Loading…
Reference in New Issue
No description provided.
Delete Branch "Harley/blender:ManageData"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Creates a new "Cleanup" dialog that allows purging unused data blocks,
as well as a window used for managing unused data blocks.
Whether or not we keep, change, or improve our practice of removing unused data-blocks, we need tools that allow users to more easily select what to keep and delete.
The File/Cleanup menu confusingly contains six items that all call the same operator in different ways. They each remove unused data but there is one for local only, linked only, and one where you have to assume does both. And for all three there is an alternative that adds the word "Recursive" without much explanation. The tooltip description for all six is identical: "Clear all orphaned data-blocks without any users from the file". Even this text is confusing as "orphans" have no parents, while these are just without owner.
Once you decide which of the six items to select, you are then presented with an ugly confirmation dialog:
This patch replaces all six menu items with one item. And also adds a new item:
"Purge Unused Data..." gives you a single dialog where you can delete any type of unused data, in a manner that I think is more informative:
"Manage Unused Data..." brings up a window with Outliner in "Orphan Data" mode so that you can have more fine-grained control:
e9fd0fc516
to05a9dbf8ef
WIP: Managing Orphaned Datato UI: Cleanup Dialog to Manage Orphaned DataHello Harley, I like this mock-up very much. I have developed add-on for more precise purging that is somewhat popular. I thought I'd share this with you so that maybe you can get some ideas from add-on inside this patch.
https://blendermarket.com/products/deep-clean
This is the add-on, if you're interested I'll send you free copy.
Especially with addons like BlenderKit that import lot of images and node groups and pack them into your scene feature like this is a must. Also makes the feature more accessible and less risky, because if you use Blender's purge you're always risking losing something important you accidentally left without fake user, like Mesh data or Action.
I've done purge duplicates, that only purge orphaned data that have .00 in the name, very useful and single-click solution to removing duplicated object data blocks after you've played with them.
Purge by Name is little more niche, but I use it very often too for images especially. Like I want to purge multiple images from one PBR texture set, but don't want to purge rest of the orphaned images I might go back to later, I purge images with "BrickWall" in the name.
Also one thing I've noticed in the images is that purging recursive option is absent in your patch (might be wrong). I think that should be kept. I wanted to implement that but currently its impossible to do "data.remove" in python recursively.
Wish you luck with this patch
There are times in my workflow where I put 1 or 2 of the specific options in my Quick Favorites or assign one or 2 to keyboard shortcuts.
I would prefer for the Clean Up menu to be left as is and add this feature as something new.
Good point. If we did this it wouldn't change or remove the existing operators. And we'd move these existing separate menu items somewhere else, like to Outliner. That way these items would still show up in searches so you could add them to your Quick Favorites. Everyone else could just the new pretty thing.
This is great stuff.
@mont29, I don't remember if you approved of this design in the past, so might want to have a look.
@ -2255,0 +2268,4 @@
static char orphans_purge_do_local = true;
static char orphans_purge_do_linked = false;
static char orphans_purge_do_recursive = false;
UI_block_funcN_set
andUI_but_funcN_set
exist to avoid global variables like these. You can pass them an allocated structure with these 3 booleans, and it should be freed for when the block or button is freed.@ -2255,0 +2353,4 @@
uiItemS(layout);
char text[1024];
DynStr *dyn_str = BLI_dynstr_new();
Not really important since it's already working, but I think simple
std::string
would be preferred overDynStr
for simple cases like this where performance is not an issue.To be clear, I'm not expecting this to be addressed.
@ -2255,0 +2458,4 @@
/* identifiers */
ot->idname = "OUTLINER_OT_orphans_cleanup";
ot->name = "Remove Unused Data...";
ot->description = "Remove Unused data in the file";
Unused -> unused for the description
Is recursive purging missing or being removed? Its more useful for me I think. Also, if there is recursive number of data blocks displayed in brackets is misleading, since 1 Material 1 Mesh can in reality be 1 Material 1 Mesh 4 Images 2 Textures 4 Brushes because that is the data that WILL become orphaned once their owners are purged and deleted with them.
Recursive is called Indirectly Unused, and would list those images and textures.
I like "Recursive" and "Purge" better as names. But regardless this is great design
Maybe keeping "Purge" makes sense. It does communicate the purpose a bit more, in implying that this data is probably unwanted.
"Recursive", not sure how well understood that term is by the average user, I'm too biased as a programmer to make a good guess.
I might be biased as well. But since we use recursive in api it would be nice if that was name in UI as well. Will make things more discoverable in api.
One more nitpick: In screens "Cancel" is colored and confirmation is black. Shouldn't this be vice versa? AFAIK only place we had those confirmation windows before was on saving, and there "Save" is colored, and cancel is black
That could certainly change. But usually my thinking is that the default button (the action that happens if you just press "Enter") should not have negative consequence if possible. So for things that could do harm if you don't read and just press "Enter" it should be the Cancel button.
@ -2255,0 +2400,4 @@
BLI_dynstr_clear(dyn_str);
orphan_desc(C, dyn_str, false, false, true);
BLI_snprintf(text, 1024, "Include Indirectly Unused (Recursive)");
Now getting into nitpicky territory...
This is using title case while "Local data" and "Linked data" are not. "unused" is implied for "Local data" and "Linked data", so why use it here? Clarifications like "(Recursive)" are not something we normally do in the UI, that seems like it should be in the tooltip.
I suggest "Include indirect data".
@blender-bot build
The feature from a user perspective looks great.
But the code can use some more work to make it a bit more aligned with general UI designs afaik, see comment below.
Could also use some review from people more informed about our UI codebase...
@ -2255,0 +2328,4 @@
else {
is_first = false;
}
desc.append<>(std::to_string(num_tagged[i]) + " ");
No reason to use
append
on strings afaik, just use+=
operator?That kind of formatting could also benefit from using
fmt::format
from ourextern/fmtlib
library.@ -2255,0 +2445,4 @@
{
OrphansPurgeData *purge_data = MEM_new<OrphansPurgeData>("orphans_purge_data");
purge_data->op = op;
UI_popup_block_invoke(C, wm_block_create_orphans_cleanup, purge_data, MEM_freeN);
Do not invoke modal UI items from an
exec
function. This code should be in theinvoke
callback.The
exec
callback should be basically whatwm_block_orphans_purge
is doing currently.Further more, any reasons not to use some operator-aware popup system, like
WM_operator_props_dialog_popup
or so?That way you can also define the settings (which types of data to purge) in the operator itself - or even better, can just re-use (and extend) existing
OUTLINER_OT_orphans_purge
, instead of creating a new one that does the same thing?Thanks! done.
Just unfamiliarity with that, so will definitely will look into that. I'm also hoping to move toward dialogs that have a clear "cancel". I don't mind if some dialogs with complex behavior are more "bespoke".
@ -2255,0 +2460,4 @@
ot->exec = outliner_orphans_cleanup_exec;
/* flags */
ot->flag = OPTYPE_REGISTER;
Missing
OPTYPE_UNDO
I think? Otherwise, a comment explaining why it's not needed would be good to have.@ -2255,0 +2465,4 @@
static int outliner_orphans_manage_exec(bContext *C, wmOperator * /*op*/)
{
const rcti window_rect = {
Would be better to adjust these default values to the available screen space? On some modern screens, 500x500 is a fairly small window...
@ -2255,0 +2486,4 @@
SpaceOutliner *soutline = (SpaceOutliner *)CTX_wm_area(C)->spacedata.first;
soutline->outlinevis = SO_ID_ORPHANS;
}
return OPERATOR_FINISHED;
Should return
OPERATOR_CANCELLED
ifWM_window_open
fails...Another point I just realized: If you are renaming "orphan" to "unused", you should also tackle the other areas of the UI (starting with the name of the Outliner view itself).
@ -2255,0 +2315,4 @@
UI_but_drawflag_disable(but, UI_BUT_TEXT_LEFT);
}
static std::string orphan_desc(bContext *C, bool local, bool linked, bool recursive)
const bContext *
,const bool
@ -2255,0 +2346,4 @@
static uiBlock *wm_block_create_orphans_cleanup(bContext *C, ARegion *region, void *arg)
{
OrphansPurgeData *purge_data = (OrphansPurgeData *)arg;
Use
static_cast
here instead of C style cast@ -2255,0 +2446,4 @@
static int outliner_orphans_cleanup_exec(bContext *C, wmOperator *op)
{
OrphansPurgeData *purge_data = MEM_new<OrphansPurgeData>("orphans_purge_data");
Use
__func__
instead of manually choosing a string@blender-bot build
@blender-bot build
Please revert that commit, this PR was not approved, and half of my feedback was not addressed (not creating a new operator when existing one should be usable, not using 'standard' operator popup code instead of defining own popup panel,...)