Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EditorUndoRedoManager: Use fixed history when a custom context is provided #100061

Conversation

allenwp
Copy link
Contributor

@allenwp allenwp commented Dec 5, 2024

This makes the custom context feature of the EditorUndoRedoManager behave consistently with the current documentation.

Fixes #100108. Also fixes #76851 by providing a custom context when creating the "Edit Shortcut" action.

@allenwp allenwp requested a review from a team as a code owner December 5, 2024 17:25
@allenwp allenwp force-pushed the force-fixed-history-for-custom-context branch from 905a619 to 259c556 Compare December 5, 2024 17:59
@@ -150,6 +150,7 @@ void EditorUndoRedoManager::create_action(const String &p_name, UndoRedo::MergeM
if (p_custom_context) {
// This assigns history to pending action.
get_history_for_object(p_custom_context);
force_fixed_history();
Copy link
Member

Choose a reason for hiding this comment

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

force_fixed_history() is supposed to be called manually. EditorUndoRedoManager will automatically deduce optimal history, if you force a different one it might lead to bugs. The method should be used if automatic history fails (there are some cases where it can't be determined), but you know that the forced one is correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without a change similar to this one, the custom_context parameter of create_action does not always function correctly (or maybe it never functions correctly). If you want, I can open a new issue describing this in more detail?

Copy link
Member

Choose a reason for hiding this comment

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

Feel free to open it, but it might be misunderstanding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've filed an issue to better describe the issue that this PR aims to resolve: #100108

@KoBeWi KoBeWi added this to the 4.4 milestone Dec 5, 2024
…rovided.

This makes the custom context feature of the `EditorUndoRedoManager` behave consistently with the current documentation.
Fixes godotengine#100108. Also fixes godotengine#76851 by providing a custom context when creating the "Edit Shortcut" action.
@allenwp allenwp force-pushed the force-fixed-history-for-custom-context branch from 259c556 to 85be143 Compare December 6, 2024 16:30
@allenwp allenwp closed this Dec 6, 2024
@allenwp allenwp deleted the force-fixed-history-for-custom-context branch December 6, 2024 20:28
@AThousandShips AThousandShips removed this from the 4.4 milestone Dec 6, 2024
@allenwp
Copy link
Contributor Author

allenwp commented Dec 6, 2024

Tried to rename my branch because this wasn't the correct solution. I'll link a new PR here in a moment that has a better fix.

@KoBeWi
Copy link
Member

KoBeWi commented Dec 6, 2024

You can push a new commit and change the title, branch name does not matter.

@allenwp
Copy link
Contributor Author

allenwp commented Dec 6, 2024

New PR with different approach: #100120

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants