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

Add deprecation notice to a duplicate method of class Window #83014

Merged
merged 1 commit into from
Jan 15, 2024

Conversation

Chubercik
Copy link
Contributor

@Chubercik Chubercik commented Oct 8, 2023

This pull request adds a deprecation warning to the Window::move_to_foreground() method, as internally it contains the same code as the Window::grab_focus() method.

In a future version of Godot (4.3? 4.4? 5.x?) said method ought to be removed, however, to maintain compatibility a deprecation warning should suffice for now.

Note: If this is not the right way to purge duplicates from the codebase I am more than willing to re-do it once I've been shown the proper way.

@Chubercik Chubercik requested review from a team as code owners October 8, 2023 21:19
@Sauermann
Copy link
Contributor

Please squash the commits into a single one as described in the PR-workflow
https://docs.godotengine.org/en/stable/contributing/workflow/pr_workflow.html#the-interactive-rebase

@Chubercik Chubercik force-pushed the window_method_duplicate_fix branch from 1bd5baf to b6ae4ba Compare October 9, 2023 06:35
@Chubercik Chubercik requested a review from Sauermann October 9, 2023 08:03
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.

Code looks correct.
Question is, if the linked issue is enough in order to justify this deprecation.

@akien-mga
Copy link
Member

Given that there is a DisplayServer.window_move_to_foreground, it's expected that there should be a Window.move_to_foreground counterpart. It's similar in most situations where there's a Node counterpart to a Server API.

grab_focus was likely added as a counterpart to the Control API (which doesn't share an inheritance with Window aside from Node, but some of their API is indeed similar).

So yeah I'm not sure what to do here. CC @bruvzg

@Chubercik
Copy link
Contributor Author

If we end up deeming both of these methods useful, how about making one of them an alias for the other?

@akien-mga
Copy link
Member

If we end up deeming both of these methods useful, how about making one of them an alias for the other?

Yes that's a good idea. And this can be pointed out in both their description, linking to the other method using the [method move_to_foreground] syntax.

Additionally deprecating could still be an option though, but I'd like to have @bruvzg's input on this.

scene/main/window.cpp Outdated Show resolved Hide resolved
@Chubercik Chubercik force-pushed the window_method_duplicate_fix branch from b6ae4ba to 06534f8 Compare January 12, 2024 18:13
@akien-mga akien-mga modified the milestones: 4.x, 4.3 Jan 13, 2024
@akien-mga akien-mga merged commit fb5ad1a into godotengine:master Jan 15, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@Chubercik Chubercik deleted the window_method_duplicate_fix branch January 15, 2024 17:34
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.

Duplicated Code in Window::move_to_foreground() and Window::grab_focus()
4 participants