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

Unexpose DisplayServer::window_set_window_event_callback #84638

Closed
wants to merge 1 commit into from

Conversation

Sauermann
Copy link
Contributor

@Sauermann Sauermann commented Nov 8, 2023

Overwriting this function led to problematic behavior, that is impossible to reimplement from GDScript.
It is possible to listen to the events, by overwriting Window._notification, so an alternative to this exposed function is available.

This PR is based on this discussion in RC and on an overview of potential DisplayServer function interferences by bruvzg.

resolve #81830

If additional DisplayServer functions should be unexposed, let me known and i will adjust this PR.

Overwriting this function led to problematic behavior, that is impossible
to reimplement from GDScript.
It is possible to listen to the events, by overwriting
`Window._notification`, so an alternative to this exposed function is
available.
@Sauermann Sauermann added this to the 4.2 milestone Nov 8, 2023
@Sauermann Sauermann marked this pull request as ready for review November 8, 2023 22:47
@Sauermann Sauermann requested review from a team as code owners November 8, 2023 22:47
Copy link
Contributor

@Riteo Riteo left a comment

Choose a reason for hiding this comment

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

I 100% support this. It looks like this was added in 8e6960a without further explanation (CC @reduz, hope I'm not bothering) and I'm not sure either what's the usecase for this too.

Just one thing before I approve, do we allow removing API like this instead of making stub deprecated methods?

@akien-mga
Copy link
Member

So I asked @reduz about this:

Akien @reduz Can you comment on why you exposed window_set_*_callback methods in DisplayServer? See #84638
reduz @Akien I have no idea why I exposed it, my suggestion is that all functions in DisplayServer for window management are discouraged to use in the docs

Since we identified a number of methods which interfere with Window and should only be used by someone who really knows what they're doing, I think it might indeed be best to put a clear disclaimer in the docs for each of those, but keep them exposed so we don't break compatibility.

#81830 shows that some users were actually using this one at least.

@akien-mga
Copy link
Member

Documentation only alternative, for discussion: #84669

My understanding is that these methods could be used to create and manage windows (but not Window nodes!) directly on the DisplayServer. Like RenderingServer can be used to create canvas items (and not CanvasItem nodes), etc.

So using these methods on an existing Window node's ID is risky, but they can be used for something more low level.

@Sauermann
Copy link
Contributor Author

#81830 shows that some users were actually using this one at least.

The user of that issue was happy with the workaround (using _notification), so I wouldn't count them as "actually using" this functionality.

@akien-mga
Copy link
Member

Superseded by #84669.

@akien-mga akien-mga closed this Nov 10, 2023
@AThousandShips AThousandShips removed this from the 4.2 milestone Nov 10, 2023
@Sauermann Sauermann deleted the fix-event-callback branch November 12, 2023 20:08
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.

Mouse Events weird when using DisplayServer.window_set_window_event_callback
5 participants