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

Remove useless Viewport::gui.key_input_accepted #96867

Merged

Conversation

L2750558108
Copy link
Contributor

Viewport::gui.key_input_accepted seems could be replaced by set_input_as_handled and is_input_handled, since all detections gui.key_event_accepted end up being set_input_as_handled

@L2750558108 L2750558108 requested a review from a team as a code owner September 11, 2024 16:25
@AThousandShips AThousandShips added this to the 4.x milestone Sep 11, 2024
@AThousandShips AThousandShips requested a review from a team September 11, 2024 16:31
@L2750558108 L2750558108 force-pushed the remove-gui-key-event-accepted-shit branch from f67895e to 4788f54 Compare September 11, 2024 19:52
@L2750558108 L2750558108 reopened this Sep 11, 2024
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.

I believe, that this simplification is beneficial and that the behavioral changes are very minor, so that nearly no user will not notice these changes.
Please have a look at my other comments, which seem to be easily resolvable.
Also make sure, to squash the commits together as mentioned in the docs: https://docs.godotengine.org/en/latest/contributing/workflow/pr_workflow.html#the-interactive-rebase

@L2750558108 L2750558108 force-pushed the remove-gui-key-event-accepted-shit branch 2 times, most recently from 8a24c62 to 8714414 Compare September 13, 2024 12:47
@L2750558108 L2750558108 force-pushed the remove-gui-key-event-accepted-shit branch from 8714414 to ccc6e5d Compare September 13, 2024 12:52
@L2750558108
Copy link
Contributor Author

L2750558108 commented Sep 13, 2024

have a look at my other comments, which seem to be easily resolvable.

Already fixed. Just careless mistakes.

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.

I did a code review and these changes make sense to me. I have not tested the PR though.

These changes are beneficial, because they consolidate the different behavior of Viewport.set_input_as_handled() and Control.accept_event(), which could be considered a bug.

While this is a change in behavior, I believe, that it is a minor change, that will not interfere with nearly all users.

Since it touches a core class, this PR would benefit from merging early in the dev cycle to get enough testing.

@Sauermann Sauermann modified the milestones: 4.x, 4.4 Sep 13, 2024
@L2750558108 L2750558108 requested review from a team as code owners September 14, 2024 02:16
@L2750558108
Copy link
Contributor Author

L2750558108 commented Sep 14, 2024

While the Control::accept_event is become the same as Viweport::set_input_as_handled, I want to do some trimmer in this PR by the way. Move Control::accept_event to Node::accept_event. Nodes can receive input events more briefly like accept_event in before Control nodes. This will not lead to compatibility issues.

@L2750558108 L2750558108 force-pushed the remove-gui-key-event-accepted-shit branch 2 times, most recently from 441b788 to 8e34520 Compare September 14, 2024 04:11
@L2750558108 L2750558108 force-pushed the remove-gui-key-event-accepted-shit branch from 8e34520 to 3249cd6 Compare September 14, 2024 08:36
@L2750558108 L2750558108 force-pushed the remove-gui-key-event-accepted-shit branch from 3249cd6 to 918b638 Compare September 14, 2024 08:44
@AThousandShips
Copy link
Member

Moving a method up in the hierarchy can be a bit complicated and might need compatibility code, we'll see when CI has finished

@AThousandShips AThousandShips changed the title Remove Useless Viewport::gui.key_input_accepted Move Control::accept_event to Node Sep 14, 2024
@AThousandShips
Copy link
Member

You will need to register compatibility methods, see here for instructions

@L2750558108 L2750558108 force-pushed the remove-gui-key-event-accepted-shit branch from 918b638 to 6da3069 Compare September 14, 2024 09:29
@L2750558108
Copy link
Contributor Author

L2750558108 commented Sep 14, 2024

Remove Useless Viewport::gui.key_input_accepted

OK. And why the PR's title is changed? Move this method is not the main thing this PR do.

@Sauermann
Copy link
Contributor

My recommendation would be to keep the two changes in two different pull requests.
The removal of Viewport::gui.key_input_accepted is enough of a change for a single pull request.
Since the behavior change could cause irritations with users, debugging or reverting it that way is much cleaner.
Also: the smaller a PR, the easier it is to review.

The change of moving accept_event from Control to Node would need to be discussed and evaluated separately - a proposal for that change could potentially be a good starting point, in order to evaluate if there is sufficient support in the community for that change.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

You also need to add to the API validation file

@AThousandShips
Copy link
Member

AThousandShips commented Sep 14, 2024

The move is the method is by far the most impactful change of this, it shouldn't be a footnote

@L2750558108 L2750558108 force-pushed the remove-gui-key-event-accepted-shit branch 6 times, most recently from b686073 to ccc6e5d Compare September 14, 2024 09:43
@AThousandShips AThousandShips changed the title Move Control::accept_event to Node Remove Useless Viewport::gui.key_input_accepted Sep 14, 2024
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.

The PR is again in its previous state, that I already had reviewed yesterday.

@akien-mga akien-mga changed the title Remove Useless Viewport::gui.key_input_accepted Remove useless Viewport::gui.key_input_accepted Sep 14, 2024
@akien-mga akien-mga merged commit 391849d into godotengine:master Sep 16, 2024
40 of 41 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@L2750558108
Copy link
Contributor Author

Yay!

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.

5 participants