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

Make save confirmation a child of last exclusive window of EditorNode before popup #102070

Merged

Conversation

ryevdokimov
Copy link
Contributor

Requires: #102017

This is necessary to prevent multiple exclusive windows on the same parent error.

@KoBeWi
Copy link
Member

KoBeWi commented Jan 27, 2025

That looks super hacky. What if there is another dialog with the same problem?
There is set_unparent_when_invisible(), I wonder if it can be used. If not, your current solution is probably the simplest one.

@ryevdokimov
Copy link
Contributor Author

ryevdokimov commented Jan 27, 2025

That looks super hacky.

The problem lies in these windows being exclusive, so to me it makes sense to reorder the windows/parents depending on the circumstances.

Reading out the code into plain English with reference to the documentation.

If the editor settings dialog is open, you want the save dialog to always appear on top of it and prevent input to it specifically. Otherwise, it should do the same to the base gui.

What if there is another dialog with the same problem?

Explicitly defining which window is on-top of and blocking which parent seems to me the most straight-forward solution to the problem. I think trying to find ways to do this automatically without the window having direct knowledge of which UI it is trying to block will end up being much hackier looking by comparison.

There is set_unparent_when_invisible(), I wonder if it can be used.

Probably, but you're going to have to reparent it to something depending on the circumstances anyways, so I think the result will end up looking the same but with more code, and not as clear what the intent is. That isn't to say that unparenting the window when it is invisible isn't beneficial, but it isn't clear to me what that the benefit is (saving resources?)

editor/editor_node.h Outdated Show resolved Hide resolved
@KoBeWi
Copy link
Member

KoBeWi commented Jan 27, 2025

I think trying to find ways to do this automatically without the window having direct knowledge of which UI it is trying to block will end up being much hackier looking by comparison.

ProgressDialog already does this, the currently focused window is not difficult to find. Although manually setting the parent is safer for now I suppose.

@ryevdokimov ryevdokimov force-pushed the reparent-save-confirmation branch 3 times, most recently from 44d10fa to f91e1a0 Compare January 27, 2025 17:51
@ryevdokimov ryevdokimov requested a review from KoBeWi January 27, 2025 18:42
@ryevdokimov ryevdokimov force-pushed the reparent-save-confirmation branch from f91e1a0 to fe5ec14 Compare January 27, 2025 20:44
@ryevdokimov
Copy link
Contributor Author

Made some modification based on conversations in Rocket Chat. The solution should be more automatic as mentioned because there already are already multiple scenarios that need to be addressed in the linked PR. This will need reapproval.

@ryevdokimov ryevdokimov changed the title Make save confirmation a child of editor settings dialog if popped up while it is visible Make save confirmation a child of last exclusive window of EditorNode before popup Jan 28, 2025
@ryevdokimov ryevdokimov force-pushed the reparent-save-confirmation branch from fe5ec14 to c7fd0bb Compare January 28, 2025 07:29
@ryevdokimov
Copy link
Contributor Author

Just updating the name of the commit to match the changes and title.

@Repiteo Repiteo merged commit 9ee1873 into godotengine:master Jan 30, 2025
19 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Jan 30, 2025

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.

4 participants