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

DisconnectHandler is never called #3604

Closed
pictos opened this issue Dec 1, 2021 · 16 comments
Closed

DisconnectHandler is never called #3604

pictos opened this issue Dec 1, 2021 · 16 comments
Assignees
Labels
legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor platform/android 🤖 s/needs-attention Issue has more information and needs another look s/verified Verified / Reproducible Issue ready for Engineering Triage t/bug Something isn't working
Projects
Milestone

Comments

@pictos
Copy link
Contributor

pictos commented Dec 1, 2021

Description

I created a Custom Control that inherits from ElementHandler and at the moment that the control is closed the DisconnectHandler method isn't called, even if I set the VirtualView.Handler to null.

Steps to Reproduce

  1. Add breakpoints here
  2. Start the sample app
  3. Scroll to the bottom and press the button
  4. See the popup
  5. Click outside the popup to close it
  6. The DisconnectHandler isn't called

Version with bug

Preview 10 (current)

Last version that worked well

Unknown/Other

Affected platforms

Android

Affected platform versions

Android 11

Did you find any workaround?

No

Relevant log output

No response

@pictos pictos added the t/bug Something isn't working label Dec 1, 2021
@jsuarezruiz jsuarezruiz added this to New in Triage via automation Dec 1, 2021
@J-Swift
Copy link
Contributor

J-Swift commented Feb 25, 2022

Just ran into this when writing a custom handler that extends PageHandler on iOS. I assume I'm leaking event subscriptions because of it, but I haven't tested.

@kristinx0211 kristinx0211 added the s/needs-repro Attach a solution or code which reproduces the issue label Mar 8, 2022
@ghost
Copy link

ghost commented Mar 8, 2022

Hi @pictos. We have added the "s/needs-repro" label to this issue, which indicates that we require steps and sample code to reproduce the issue before we can take further action. Please try to create a minimal sample project/solution or code samples which reproduce the issue, ideally as a GitHub repo that we can clone. See more details about creating repros here: https://github.com/dotnet/maui/blob/main/.github/repro.md

This issue will be closed automatically in 7 days if we do not hear back from you by then - please feel free to re-open it if you come back to this issue after that time.

@pictos
Copy link
Contributor Author

pictos commented Mar 10, 2022

Hey @kristinx0211, here's the link for the light repro: https://github.com/pictos/MauiBugRepro/tree/DisconnectHandlerNotCalled

@ghost ghost added s/needs-attention Issue has more information and needs another look and removed s/needs-repro Attach a solution or code which reproduces the issue labels Mar 10, 2022
@VincentBu
Copy link

Hi @pictos , Could you give more details about repro steps for MauiBugRepro?

@VincentBu VincentBu added the s/needs-info Issue needs more info from the author label Mar 17, 2022
@ghost
Copy link

ghost commented Mar 17, 2022

Hi @pictos. We have added the "s/needs-info" label to this issue, which indicates that we have an open question for you before we can take further action. This issue will be closed automatically in 7 days if we do not hear back from you by then - please feel free to re-open it if you come back to this issue after that time.

@pictos
Copy link
Contributor Author

pictos commented Mar 17, 2022

@VincentBu , sure.

  1. Checkout this branch and run the sample app;
  2. Add a breakpoint here to make sure that the sample is using the CustomEntryHandler
  3. If you run on iOS add a breakpoint here if you're on Android add a breaking point here
  4. Click on the Button with the Text Navigate to Next Page
  5. The breakpoint on CustomEntryHandler ctor should be triggered, just press Continue
  6. Navigate back to the previous page
  7. The breakpoints on the DisconnectHandler will not be triggered. But they should, no?

@ghost ghost removed the s/needs-info Issue needs more info from the author label Mar 17, 2022
@VincentBu
Copy link

@pictos you are right. DisconnectHandler isn't triggered on Android emulator. @kristinx0211 , Could you repro this on IOS emulator?

@VincentBu VincentBu added the s/verified Verified / Reproducible Issue ready for Engineering Triage label Mar 18, 2022
@kristinx0211
Copy link

repro on iPhone13 IOS 15.2. BR can't be hit when navigate back, repro sample:
MauiApp2.zip

@Redth Redth added the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label Mar 22, 2022
@Redth Redth added this to the 6.0.300-rc.2 milestone Mar 22, 2022
@PureWeen
Copy link
Member

@pictos Do you still need DisconnectHandler here? Can you use the new Loaded/Unloaded events to know when to unsubscribe or call DisconnectHandler yourself?

@pictos
Copy link
Contributor Author

pictos commented Apr 20, 2022

@PureWeen would be good if the .NET MAUI does that for me (lazy developer mode on), but it's fine if I've to call the DisconnectHandler myself.

@PureWeen
Copy link
Member

PureWeen commented May 19, 2022

I'm going to close this issue for now.

The current behavior for MAUI is to not automatically call DisconnectHandler as we can't predict when users might want resources cleaned up.

I suspect we'll need to eventually add some helpers or possibly lifecycle properties so users can tie Disconnect to being attached/detached from a window

Triage automation moved this from New to Done May 19, 2022
@J-Swift
Copy link
Contributor

J-Swift commented May 19, 2022

Thats really surprising : counterintuitive. Isnt that the whole point of connect/disconnect, that I want my resources cleaned up at that point? I dont put anything in those blocks if I dont want that. Especially because ConnectHandler is where you attach touch handlers etc as far as Im aware, I think its a recipe for surprising behavior and memory leaking.

@pictos
Copy link
Contributor Author

pictos commented May 20, 2022

@PureWeen will you create the issue about the lifecycle properties?

@MichaelRumpler
Copy link
Contributor

The current behavior for MAUI is to not automatically call DisconnectHandler as we can't predict when users might want resources cleaned up.

I want to clean up when the platform view is not needed anymore. Where would I put that code if not in DisconnectHandler? What was the intention of DisconnectHandler if not for that purpose?

@PureWeen
Copy link
Member

@MichaelRumpler DisconnectHandler is used to cleanup the resource but we can't make assumptions about when the user will want that to happen. Us assuming when to disconnect a handler works for one set of problems but then messes up another set.

@PureWeen
Copy link
Member

PureWeen commented May 20, 2022

@pictos yea this is definitely going to become a thing :-)

An issue and some design docs around current recommendations will definitely be in order.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor platform/android 🤖 s/needs-attention Issue has more information and needs another look s/verified Verified / Reproducible Issue ready for Engineering Triage t/bug Something isn't working
Projects
No open projects
Development

No branches or pull requests

8 participants