Skip to content

Remodel GestureController lifetimes #2355

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

leolost2605
Copy link
Member

@leolost2605 leolost2605 commented Apr 11, 2025

We are leaking WindowClones since #2327. This is because the GestureController holds a strong reference on the target which means in our case that the windowclone holds a strong ref on the controller which holds a strong ref on the window clone => ref cycle. That is for example also why we set it to null in the panelwindow once it unmanages

gesture_controller = null; // make it release its reference on us

While we could easily work around this I admit for how we currently use it the ownership model of gesturecontrollers is not really intuitive.

There are multiple solutions to this problem:

  • We could just do the same as in panel window but we don't exactly know the lifetime of a windowclone (it can move to other ws, unmanage, etc.) and destroy doesn't work either because that relies on actually destroying it instead of e.g. just removing it which would be very easy to miss (that would be the workaround).
  • Make the gesture controller just hold an unowned ref to target. Very error prone because you definitely don't expect that. (Even less intuitive)
  • Be inspired by Clutter, GTK, etc. (think EventControllers, Constraints, Actions) and have a RootTarget where you can add controllers which will automatically attach them to that target and bind their lifetime to it. (Hopefully more intuitive :))

I went with the last one because IMO it's the least error prone and thus safest :)

Also don't hold a ref on the actor in the ScrollBackend because it's unneeded and would cause another ref cycle.

@leolost2605 leolost2605 requested a review from a team April 11, 2025 13:39
@leolost2605 leolost2605 requested a review from lenemter April 17, 2025 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants