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

Check FLAG_POPUP to close an AcceptDialog when parent is focused #79293

Merged

Conversation

ItsNL
Copy link
Contributor

@ItsNL ItsNL commented Jul 10, 2023

Fix #79150

The close_on_escape property was used to be able to close an AcceptDialog by pressing the Esc key, but it was also checked to close when the parent was focused, meaning it had lost the focus.

With this pull request instead of checking if close_on_escape is true to close automatically when the parent is focused (For example clicking outside of the dialog), it will check if popup_window is true.

Now dialogue_close_on_escape only affects if pressing the Esc key will close the dialog.

Here is a video showing the change:

close_on_unfocused.test.-.fix.79150.-.godot.mp4

Note: When using popup_window you will need that exclusive is false so that it works. exclusive as true blocks all input that is outside the dialog.
Note 2: When only using dialog_close_on_escape clicking outside will make the dialog's focus be lost and so using Esc won't work until the dialog has the focus again.

@ItsNL ItsNL requested review from a team as code owners July 10, 2023 15:26
@AThousandShips AThousandShips added this to the 4.x milestone Jul 10, 2023
@Sauermann
Copy link
Contributor

I was rather thinking along the lines of

diff --git a/scene/gui/dialogs.cpp b/scene/gui/dialogs.cpp
index 0860bf3968..5865e06ec9 100644
--- a/scene/gui/dialogs.cpp
+++ b/scene/gui/dialogs.cpp
@@ -45,7 +45,7 @@ void AcceptDialog::_input_from_window(const Ref<InputEvent> &p_event) {
 }
 
 void AcceptDialog::_parent_focused() {
-       if (close_on_escape && !is_exclusive()) {
+       if (!is_exclusive()) {
                _cancel_pressed();
        }
 }

I am not sure, if there is a use-case supporting close_on_unfocused.

@ItsNL
Copy link
Contributor Author

ItsNL commented Jul 10, 2023

Your option uses exclusive as your way to decide if the dialog will close or not when clicking outside. But, when enabling exclusive you can't interact with the rest of the interface.
So, a use-case of close_on_unfocused would be having it off, but being able to have the entire UI accessible without having to close the dialog first or closing it automatically. With the option of still being able to close it using Esc (if wanted).
In conclusion, with that option, a user has to decide whether they allow clicking outside of the dialog to close it or not allow interacting with any other UI at all.

Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

It won't work with native windows when switching focus to the other app, so the property name can be misleading. For native windows, the same behavior is handled by FLAG_POPUP, so it probably should check it instead of adding a new property.

@ItsNL
Copy link
Contributor Author

ItsNL commented Jul 14, 2023

I have the changes ready using FLAG_POPUP and it works fine. Should I create another Pull Request and close this one? Because now it has nothing to do with this Pull Request.

@KoBeWi
Copy link
Member

KoBeWi commented Jul 14, 2023

You can change commit message and title. No need to make new PR.

@ItsNL ItsNL force-pushed the add-accept-dialog-close-on-unfocused branch from 17bc182 to 267e4bf Compare July 14, 2023 17:10
@ItsNL
Copy link
Contributor Author

ItsNL commented Jul 14, 2023

With the new way of using FLAG_POPUP, what should I do with its description?
Right now it says:

If [code]true[/code], the [Window] will be considered a popup. Popups are sub-windows that don't show as separate windows in system's window manager's window list and will send close request when anything is clicked outside of them (unless [member exclusive] is enabled).
[b]Note:[/b] This property only works with native windows.

I don't think I have to change it because the behavior described doesn't involve the change.

@ItsNL ItsNL changed the title Add close_on_unfocused property to AcceptDialog Check FLAG_POPUP to close an AcceptDialog when parent is focused Jul 14, 2023
@Sauermann
Copy link
Contributor

I can confirm, that this solves the linked issue for me.

After a few tests, I believe, that the note can be removed:

[b]Note:[/b] This property only works with native windows.

The description fits with the behavior of non-native windows.

@ItsNL ItsNL force-pushed the add-accept-dialog-close-on-unfocused branch from 267e4bf to a77d8b3 Compare July 19, 2023 14:06
@ItsNL
Copy link
Contributor Author

ItsNL commented Jul 19, 2023

Ok. I've deleted the note now.

Copy link
Contributor

@Sauermann Sauermann left a comment

Choose a reason for hiding this comment

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

Have tested it successfully with my MRP in the linked issue.

@YuriSizov YuriSizov modified the milestones: 4.x, 4.2 Jul 25, 2023
@akien-mga akien-mga changed the title Check FLAG_POPUP to close an AcceptDialog when parent is focused Check FLAG_POPUP to close an AcceptDialog when parent is focused Aug 2, 2023
@akien-mga akien-mga merged commit 2a9aaae into godotengine:master Aug 2, 2023
@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.

AcceptDialog.close_on_escape changes how mouse clicks are interpreted
7 participants