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

Move Control::accept_event to Node::accept_event #96989

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

L2750558108
Copy link
Contributor

After #96867, the behavior of Control::accept_event and Viewport::set_input_as_handled will be the same. Control::accept_event could be a general way to handle input event, and replace get_viewport().set_input_as_handled() before in nodes except Control.

@L2750558108 L2750558108 requested review from a team as code owners September 14, 2024 09:48
@AThousandShips AThousandShips changed the title Move accept event method Move Control::accept_event to Node Sep 14, 2024
@L2750558108 L2750558108 changed the title Move Control::accept_event to Node Move Control::accept_event to Node::accept_event Sep 14, 2024
@AThousandShips AThousandShips added this to the 4.x milestone Sep 14, 2024
@AThousandShips AThousandShips changed the title Move Control::accept_event to Node::accept_event Move Control::accept_event to Node::accept_event Sep 14, 2024
@L2750558108 L2750558108 force-pushed the move-accept-event-method branch 3 times, most recently from 00ee445 to 0dab014 Compare September 14, 2024 10:13
@AThousandShips
Copy link
Member

You will need to add the following to the end of misc/extension_api_validation/4.3-stable.expected (with two empty lines after the last entry)

GH-96989
--------
Validate extension JSON: API was removed: classes/Control/methods/accept_event

Method moved to Node. Compatibility method registered.

@L2750558108 L2750558108 force-pushed the move-accept-event-method branch from 26d25f2 to 4fa5762 Compare September 14, 2024 13:16
@L2750558108 L2750558108 requested review from a team as code owners September 14, 2024 13:16
@AThousandShips
Copy link
Member

You need to pull your upstream changes and then apply your new changes, you just erased the previous changes you had made (remember to push with git push --force)

@L2750558108 L2750558108 force-pushed the move-accept-event-method branch from 4fa5762 to 0064559 Compare September 14, 2024 13:32
<method name="accept_event">
<return type="void" />
<description>
Stops the input from propagating further down the [SceneTree].
Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming it still is accurate, the new description is a bit too vague... or wrong? I noticed it's identical to Viewport's, but... hm. For example, I remember that the actual order is dependent on the current input step, it's not always "down".

Suggested change
Stops the input from propagating further down the [SceneTree].
Stops the input event from propagating further in this node's viewport (see [method get_viewport]). This is equivalent to [method Viewport.set_input_as_handled] on this node's viewport.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Emm... I copy it from the Viewport::set_input_as_handled, since its behavior is the same as it. Is this means that the description in Viewport also need change?

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't have to be done in this PR

@L2750558108 L2750558108 force-pushed the move-accept-event-method branch from 0064559 to 1ce9317 Compare September 15, 2024 07:33
@AThousandShips

This comment was marked as resolved.

@AThousandShips
Copy link
Member

You used a merge commit to update your branch, please use rebase in the future instead, see here

@raulsntos
Copy link
Member

Since the method moves from a derived type (Control) to a base type (Node), that doesn't break compat in C#.

I don't know if breaks compat for godot-cpp, but I feel like adding a compat method for a method that is removed doesn't really do anything because there's nowhere to add the compat hash in the extension_api.json.

So if the compat method is not needed for C++ I'd just remove it. If the compat method is needed, we'll likely need to figure out a way to add it to extension_api.json.

cc @dsnopek

@AThousandShips
Copy link
Member

AThousandShips commented Sep 16, 2024

It is needed and it is the way things have been done before, but there was several ones moved to the hashes in #96586 so might be worth doing that here too

Edit: those were moved for different reasons though so unsure if it is valid here, and realized the cases I thought were moved were in fact changed differently, so don't know if we have any cases like this in the codebase at all

@L2750558108 L2750558108 force-pushed the move-accept-event-method branch from 4963a50 to d6d2c4a Compare September 17, 2024 04:19
@Sauermann
Copy link
Contributor

I remember a similar case in #68070, but it was merged for 4.0, so no compatibility functions were necessary.

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