Skip to content

Conversation

@Raftatul
Copy link
Contributor

@Raftatul Raftatul commented Oct 10, 2025

Fixes: godotengine/godot-proposals#13343

Screen.Recording.2025-10-10.025134.mp4

@AThousandShips AThousandShips added this to the 4.x milestone Oct 10, 2025
@AThousandShips AThousandShips changed the title Copy/paste group/category properties Add support for copy/paste of group/category properties Oct 10, 2025
@Raftatul Raftatul force-pushed the Copy/PasteGroup/CategoryProperties branch from 6c550c1 to e4d829f Compare October 11, 2025 01:35
@Raftatul Raftatul marked this pull request as ready for review October 11, 2025 22:03
@Raftatul Raftatul requested review from a team as code owners October 11, 2025 22:03
@YeldhamDev
Copy link
Member

YeldhamDev commented Oct 12, 2025

Pasting when there was nothing copied creates an action named "Set category/group <null>".

Also, the fact that the paste action in the popup uses "category" or "group" in the name is odd since "Paste Group Values" will actually paste a category if that's what you have copied. It should just be "Paste Copied Values" instead.

@Raftatul
Copy link
Contributor Author

Pasting when there was nothing copied creates an action named "Set category/group ".

Also, the fact that the paste action in the popup uses "category" or "group" in the name is odd since "Paste Group Values" will actually paste a category if that's what you have copied. It should just be "Paste Copied Values" instead.

I think copying properties from a category and pasting them into a group within that category should only paste the group's properties. This allows you to paste multiple groups within a single category without having to copy each group individually.

But if the properties of a category are pasted into a group that is in another category, nothing should happen.

@Raftatul Raftatul force-pushed the Copy/PasteGroup/CategoryProperties branch 2 times, most recently from 1024aac to b54c883 Compare October 12, 2025 06:58
@YeldhamDev
Copy link
Member

Feature seems to be working well. Those commits need to be squashed, however.

@Raftatul Raftatul force-pushed the Copy/PasteGroup/CategoryProperties branch from b54c883 to 79b3fa4 Compare October 12, 2025 16:41
Copy link
Member

@YeldhamDev YeldhamDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feature-wise, everything seems to be working fine.

@Raftatul Raftatul force-pushed the Copy/PasteGroup/CategoryProperties branch from 1934d9b to 23cc324 Compare October 12, 2025 20:27
@Raftatul Raftatul force-pushed the Copy/PasteGroup/CategoryProperties branch 2 times, most recently from 8f29df4 to 4ccf52b Compare October 13, 2025 00:44
@Raftatul
Copy link
Contributor Author

I found unexpected behavior when pasting on multiple nodes.

Screen.Recording.2025-10-13.130423.mp4

@Raftatul
Copy link
Contributor Author

I found unexpected behavior when pasting on multiple nodes.
Screen.Recording.2025-10-13.130423.mp4

Could this come from the way I'm doing the undo/redo ?
image

@KoBeWi
Copy link
Member

KoBeWi commented Oct 13, 2025

The undo/redo usage looks correct, but for some reason it creates "Set category" as global action and makes individual action for each property. Something is wrong.

@Raftatul
Copy link
Contributor Author

Raftatul commented Oct 13, 2025

Yeah, the thing is, this code

LocalVector<String> properties;
const String action_name = vformat(TTR("Set group %s on node %s"), group_name, object->get("name"));

_collect_properties(properties);

EditorUndoRedoManager *undo_redo = EditorUndoRedoManager::get_singleton();
undo_redo->create_action(action_name);

for (const String &property_name : properties) {
	undo_redo->add_do_property(object, property_name, clipboard[property_name]);
	undo_redo->add_undo_property(object, property_name, object->get(property_name));
}

undo_redo->commit_action();

Is only called for the object that is selected in the inspector (so the last one selected?). So the values pasted onto the other object are handled by something else.

@KoBeWi
Copy link
Member

KoBeWi commented Oct 14, 2025

Ok I see what's the problem, but fixing it will involve modifying MultiNodeEdit.
MultiNodeEdit has undo/redo built-in in its _set() method. If you call add_do_property() on it, the undo action will cause another undo action. MultiNodeEdit itself is not recognized as part of the scene, hence there is history mismatch on to of that.

This is a bit messy, so I think you can just put a big FIXME comment and I will do a follow-up PR once this is merged.

EDIT:
Actually I came up with how to implement it independently. I will open a fix later today.

@KoBeWi
Copy link
Member

KoBeWi commented Oct 14, 2025

I realized it's wrong either way. You can't undo a property change on all nodes at once, because MultINodeEdit can't retrieve current value from all nodes, it will just use the first node. So if you want proper undo/redo, you will need to manually set properties on each node.

@Raftatul
Copy link
Contributor Author

I realized it's wrong either way. You can't undo a property change on all nodes at once, because MultINodeEdit can't retrieve current value from all nodes, it will just use the first node. So if you want proper undo/redo, you will need to manually set properties on each node.

What approach should I take then ?

@KoBeWi
Copy link
Member

KoBeWi commented Oct 16, 2025

Check if object is MultiNodeEdit, and if so, loop over its nodes and set properties individually.

You might need to do the same for EditorDebuggerRemoteObjects.

@Raftatul Raftatul force-pushed the Copy/PasteGroup/CategoryProperties branch 2 times, most recently from 0b76af7 to b2428fb Compare October 16, 2025 20:33
@Raftatul Raftatul requested a review from KoBeWi October 16, 2025 22:07
@Raftatul Raftatul changed the title Add support for copy/paste of group/category properties Add support for copy/paste of section/category properties Nov 4, 2025
Added Copy/Paste Properties For Categories

Fixed SubGroup Not Showing PopupMenu

Disables popmenu on unchecked groups

Format code

Changed _collect_properties for EditorInspectorCategory and EditorInspectorSection

Fixed coding style

Fixes copy/paste for EditorInspectorArray

Fixes check for EditorInspectorArray

Fixes empty paste null reference + Allow to copy/paste group using hierarchy

Apply suggestions from code review

Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>
Co-authored-by: Tomasz Chabora <kobewi4e@gmail.com>

Prevents modification of copied properties

Adds copy/paste icons to theme

Apply suggestions + Cache icons for editor section

Fixes category property copy/paste issues

Handle MultiNodeEdit paste for group/category

Handle EditorDebuggerRemoteObjects paste for category

Only allow copy/paste on the same category/section name
@Raftatul Raftatul force-pushed the Copy/PasteGroup/CategoryProperties branch from 1e26692 to 17f942f Compare November 8, 2025 04:50
@Raftatul Raftatul requested review from KoBeWi and adamscott November 8, 2025 04:50
@Raftatul
Copy link
Contributor Author

Raftatul commented Nov 8, 2025

I think the feature is operational. However, it would be better to disable the paste buttons (like graying them out) if the clipboard doesn’t contain “@pastebin_category_name” or “@pastebin_section_name”, or if the category or section name doesn’t match.

}
}

if (category_name != pastebin_category_name) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is technically unnecessary 🤔
Pasting into a different category will just assign matching properties, which might be useful sometimes.

@@ -1410,7 +1350,11 @@ void EditorProperty::menu_option(int p_option) {
InspectorDock::get_inspector_singleton()->set_property_clipboard(object->get(property));
} break;
case MENU_PASTE_VALUE: {
emit_changed(property, InspectorDock::get_inspector_singleton()->get_property_clipboard());
const Dictionary clipboard = InspectorDock::get_inspector_singleton()->get_property_clipboard();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Properties are copied as their actual value, not a Dictionary. You need to first check type of property clipboard.

if (clipboard.has("@pastebin_category_name") || clipboard.has("@pastebin_section_name")) {
break;
}
emit_changed(property, clipboard);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use the original value, not the forced Dictionary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Copy/Paste Group/Category Properties

6 participants