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

Allow contextual plugins to persist temporarily #81523

Merged
merged 1 commit into from
Oct 6, 2023

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Sep 10, 2023

Fixes #72271

This makes the theme editor semi-persistent. If you edit a theme from the FileSystem Dock or directly in the inspector (via Edit), the editor will not be closed when something else takes over its editing context. A close button was added to close the editor manually.
The editor can still close automatically if its editing context vanishes (EditorProperty).

godot.windows.editor.dev.x86_64_vO8cL5OzT1.mp4

@KoBeWi KoBeWi force-pushed the epic_self_roast branch 2 times, most recently from 02be397 to 4223df4 Compare September 10, 2023 20:52
@YuriSizov
Copy link
Contributor

That's a neat trick, and I think we could do this. But the case of editing a theme attached to a control is still important, I believe, and this trick doesn't cover that case. So I wouldn't say it fully closes the issue.

There's also a bug:

editor\editor_node.cpp:8087 - Condition "plugins_list.has(p_plugin)" is true.

godot.windows.editor.dev.x86_64_2023-09-11_14-25-14.mp4

@KoBeWi
Copy link
Member Author

KoBeWi commented Sep 11, 2023

The bug is fixed.

But the case of editing a theme attached to a control is still important, I believe, and this trick doesn't cover that case.

The Theme Editor is closed, because the EditorProperty that was editing the theme is gone. I could make it persist, but then deselecting the node would keep the theme edited. Not sure if that's desirable.

@YuriSizov
Copy link
Contributor

Could we add the old hack to can_auto_hide which checked if the edited object is also a stylebox, a font, or a texture that the edited theme holds?

@KoBeWi
Copy link
Member Author

KoBeWi commented Sep 11, 2023

Done, but I had to do a questionable change. Inspector sets new object before clearing, so that plugins can know it before being cleared.

@YuriSizov
Copy link
Contributor

Going back in history is a font of all sorts of bugs 🙁 Closing the theme normally seems to work fine. But once you close it under the conditions demonstrated in the clip, you can no longer open the theme edit:

godot.windows.editor.dev.x86_64_2023-09-14_20-42-26.mp4

Regarding the unnatural thing that you had to do, it's probably fine. But to be absolutely safe you could add get_next_edited_object() or something like that? That's all you need from the hack, to be able to get a reference through the inspector dock to the upcoming edited object.

@YuriSizov
Copy link
Contributor

@KoBeWi Any news on fixing the bug (and maybe doing the pre-edit step in a less nasty manner)?

@KoBeWi
Copy link
Member Author

KoBeWi commented Sep 25, 2023

I will finish this one day.

@YuriSizov YuriSizov marked this pull request as draft September 29, 2023 12:03
@YuriSizov
Copy link
Contributor

I'll put it into draft for now, feel free to undraft once it's ready

@KoBeWi KoBeWi marked this pull request as ready for review September 29, 2023 16:21
@KoBeWi
Copy link
Member Author

KoBeWi commented Sep 29, 2023

Ok I fixed the history bug. I just forgot to unref Theme when closing the editor.
Also added get_next_edited_object() like you suggested.

@YuriSizov
Copy link
Contributor

As described on RC, there is currently an issue because behavior is different between the main inspector and sub inspectors. For subinspectors get_next_edited_object() doesn't work, because it gets cleared before we get to the theme editor, so it has nothing to work with. For subinspectors using the old get_edited_object() method is sufficient. But it works the other way around for the main inspector.

The solution can be as simple as moving the clearing step to the last possible moment:

diff --git a/editor/editor_inspector.cpp b/editor/editor_inspector.cpp
index 5985bdf525..233fcd10ce 100644
--- a/editor/editor_inspector.cpp
+++ b/editor/editor_inspector.cpp
@@ -3408,7 +3408,6 @@ void EditorInspector::edit(Object *p_object) {
 	per_array_page.clear();
 
 	object = p_object;
-	next_object = nullptr;
 
 	if (object) {
 		update_scroll_request = 0; //reset
@@ -3418,6 +3417,10 @@ void EditorInspector::edit(Object *p_object) {
 		object->connect("property_list_changed", callable_mp(this, &EditorInspector::_changed_callback));
 		update_tree();
 	}
+
+	// Keep it available until the end so it works with both main and sub inspectors.
+	next_object = nullptr;
+
 	emit_signal(SNAME("edited_object_changed"));
 }

However, you can still manage to permanently disable the theme editor with this PR. Open a theme resource, then edit its subresource. Use the inspector history to navigate back to the theme and close the editor. You will no longer be able to open it by any means.

godot.windows.editor.dev.x86_64_2023-10-06_15-23-39.mp4

This does NOT happen if you open a control that has a theme as a subresource. Doing similar steps still restores the theme editor.

@KoBeWi
Copy link
Member Author

KoBeWi commented Oct 6, 2023

Fixed. Theme editor was clearing the inspector to remove itself, but the code for hiding editors did not handle self-owning plugins.

Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

I tried a few more things and couldn't kill it anymore. I declare this a success!

@akien-mga akien-mga merged commit 49e2bd9 into godotengine:master Oct 6, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

Theme editor immediately closing after opening any theme item or simply clicking on viewport
4 participants