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

Rework EditorPlugin editing logic #71770

Merged
merged 1 commit into from
Jan 22, 2023

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Jan 20, 2023

Kind of revival of #45085

The aim of this PR is to make editing Nodes and Resources more reliable. EditorPlugins will now receive edit(something) when necessary and edit(nullptr) when they are supposed finish editing. Also the limitation of only one active plugin is lifted.

The PR introduces concept of "editing owner". When a resource is edited, the system that triggered the editing becomes the "owner". It can be SceneTreeDock (by selecting node), Inspector (by opening a resource file) or EditorPropertyResource (by unfolding a sub-resource). The idea is that each owner can have its own active editor (or editors), which allows to fix certain issues where you can't edit two things at once. I also added logic for determining whether an "owner" is currently editing something or is no longer editing it, so plugins can be explicitly notified that they should stop editing.

Fixes #13849
Fixes #32002
Fixes #43635
Fixes #70914

Opening as draft, because the PR is unfinished. I confirmed that it fixes the issues, but it might have unwanted side-effects and the code is a mess right now.

@realkotob
Copy link
Contributor

Thank you for this 👍🏽

Lifting these limitations I believe also helps open up enhancements to multi-window support of the bottom panels which I want to work on.

@KoBeWi KoBeWi force-pushed the better_editing_or_something branch 6 times, most recently from bb859ee to f784598 Compare January 21, 2023 22:19
@KoBeWi KoBeWi marked this pull request as ready for review January 21, 2023 22:21
@KoBeWi KoBeWi requested review from a team as code owners January 21, 2023 22:21
@KoBeWi
Copy link
Member Author

KoBeWi commented Jan 21, 2023

Ok this is more or less finished. I removed some methods regarding top editors and folding properties, as they are now handled differently. I will give this PR some testing, but the biggest problem that may happen (aside from random oversights) is that not all plugins handle editing nullptr correctly.

@reduz
Copy link
Member

reduz commented Jan 21, 2023

I really like this, super clean.

@KoBeWi KoBeWi force-pushed the better_editing_or_something branch from f784598 to 4ae168e Compare January 21, 2023 23:21
@KoBeWi KoBeWi requested a review from a team as a code owner January 21, 2023 23:21
@YuriSizov
Copy link
Contributor

Would that fix #70914 too, or do we still need to fix relevant plugins?

@KoBeWi
Copy link
Member Author

KoBeWi commented Jan 21, 2023

Yes.

@akien-mga akien-mga merged commit bda8730 into godotengine:master Jan 22, 2023
@akien-mga
Copy link
Member

Thanks!

@KoBeWi KoBeWi deleted the better_editing_or_something branch January 22, 2023 11:39
@geowarin
Copy link
Contributor

geowarin commented Jan 22, 2023

I can very reliably get a segfault here where p_object is null with the scatter plugin.

Wouldn't a null check be necessary here or this is not supposed to happen?

@KoBeWi
Copy link
Member Author

KoBeWi commented Jan 22, 2023

See #71859

@geowarin
Copy link
Contributor

Yes I've just seen it, you're too fast ;)

@vonagam
Copy link
Contributor

vonagam commented Jan 23, 2023

ThemeEditor::edit does not handle null, leads to crashes. (Strange because ThemeEditorPlugin::edit does have a call with null reference, but I guess it never was called before).

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