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

Flutter Update: 2.10.0 - Fix: Looking up a deactivated widget's ancestor is unsafe fix #499

Merged
merged 2 commits into from Feb 4, 2022
Merged

Flutter Update: 2.10.0 - Fix: Looking up a deactivated widget's ancestor is unsafe fix #499

merged 2 commits into from Feb 4, 2022

Conversation

karvulf
Copy link
Contributor

@karvulf karvulf commented Feb 4, 2022

With the new stable version 2.10.0 of Flutter, there is a new problem.
When building this widget with some images, it works fine.
But when leaving/disposing this widget, there is crash report in the console that says Looking up a deactivated widget's ancestor is unsafe.
The solution for this problem was to initialize the controller inside initState to ensure, that everything was initialized correctly in the lifecycle of a stateful widget.
When the widget is disposed, there is no problem disposing these controllers.

@renancaraujo renancaraujo merged commit 8156907 into bluefireteam:master Feb 4, 2022
@karvulf karvulf deleted the bugfix/flutter-update-2.10 branch February 4, 2022 15:12
@m-wasilewski
Copy link

@renancaraujo could you update package on pub.dev?

@YDA93
Copy link

YDA93 commented Feb 4, 2022

Will this be published to pub.dev anytime soon?

@Shlaayl
Copy link

Shlaayl commented Feb 5, 2022

I also need a new fix

@Shlaayl
Copy link

Shlaayl commented Feb 6, 2022

Please upload new update for this package on pub.dev

@bounty1342
Copy link

bounty1342 commented Feb 6, 2022

It's not related to 2.10.0 and the fix is only partial I'm afraid.

This should work on all situations :

  void initControler() {
    _scaleAnimationController = AnimationController(vsync: this)
      ..addListener(handleScaleAnimation)
      ..addStatusListener(onAnimationStatus);
    _positionAnimationController = AnimationController(vsync: this)
      ..addListener(handlePositionAnimate);
    _rotationAnimationController = AnimationController(vsync: this)
      ..addListener(handleRotationAnimation);
  }

  void disposeControler() {
    _scaleAnimationController.removeListener(handleScaleAnimation);
    _scaleAnimationController.removeStatusListener(onAnimationStatus);
    _scaleAnimationController.dispose();
    _positionAnimationController.removeListener(handlePositionAnimate);
    _positionAnimationController.dispose();
    _rotationAnimationController.removeListener(handleRotationAnimation);
    _rotationAnimationController.dispose();
  }

  @override
  void initState() {
    super.initState();
    initControler();
    initDelegate();
    addAnimateOnScaleStateUpdate(animateOnScaleStateUpdate);

    cachedScaleBoundaries = widget.scaleBoundaries;
  }

  @override
  void didChangeDependencies() {
    super.didChangeDependencies();
    disposeControler();
    initControler();
  }

  @override
  void dispose() {
    disposeControler();
    super.dispose();
  }

edit : maybe the didChange is not mandatory. So it just add the missing _rotationAnimationController.

@Shlaayl
Copy link

Shlaayl commented Feb 7, 2022

@bounty1342 Thank you ,It works fine 👍

@wladyslawczyzewski
Copy link

@bounty1342 would you mind open a PR for a proper fix?

@parthvirani95
Copy link

parthvirani95 commented Feb 9, 2022

@bounty1342 in which file do we need to apply these changes? could you please suggest me? my code are attached here and I am receiving same error.

github

@karvulf
Copy link
Contributor Author

karvulf commented Feb 9, 2022

The suggested change of @bounty1342 is looking interesting.
I didn't move one controller to initState because it wasn't part of the exception. But it's correct that there is no reason to not do that.
But I don't understand why you recreate the controller inside didChangeDependencies. This could lead to weird behavior in my opinion.

If you don't want to get the exception and you are willing to use the master branch, then you could change your dependency in pubspec.yaml to:

  photo_view:
    git: https://github.com/bluefireteam/photo_view.git

@bounty1342
Copy link

@karvulf , yep there is no need for didChangeDependencies, I was thinking about didUpdateWidget...

We should override didUpdateWidget() and recreate the controller if the controller change, this way we could reuse the same widget with the new controller. (We need to create new controller as there is no way to update the initial scale for exemple)

What do you think ?

@karvulf
Copy link
Contributor Author

karvulf commented Feb 9, 2022

Well, I didn't look any deeper inside the code, but normally I wouldn't recreate the controller, but it depends on the usecase, I think.
@bounty1342

@karvulf
Copy link
Contributor Author

karvulf commented Feb 9, 2022

And if there was a problem with an intial value, then the bug should already exists because the creation of the controller didn't changed. It was just moved to initState.

@harry-dickson
Copy link

FYI, my encounter with the problem was that if you don't touch anything and navigate away from PhotoView, causing PhotoViewCoreState.dispose() to be called, it lazily instantiates the controllers in dispose() (they are late final) just to immediately dispose of them. And this causes pain and confusion to the confused dev (me)

@parthvirani95
Copy link

parthvirani95 commented Feb 13, 2022 via email

@S-Man42
Copy link

S-Man42 commented Mar 24, 2022

Please release this fix! :)

@matehat
Copy link

matehat commented Mar 28, 2022

Anything holding this from a release? @karvulf

@karvulf
Copy link
Contributor Author

karvulf commented Mar 28, 2022

No I don't think so, but I am not able to update this on pub.dev @matehat

@NathanMDT
Copy link

I am also voting for a release.

@JmyW
Copy link

JmyW commented Apr 11, 2022

vote too for an official release! thanks a lot!

@dipbhi
Copy link

dipbhi commented Apr 20, 2022

Is there any planned date to release this fix?

T-moz pushed a commit to T-moz/photo_view that referenced this pull request Apr 22, 2022
…tor is unsafe fix (bluefireteam#499)

* Looking up a deactivated widget's ancestor is unsafe fix

* fixed assignment

Co-authored-by: André <andre.boerger@alte-leipziger.de>
T-moz pushed a commit to T-moz/photo_view that referenced this pull request Apr 22, 2022
…tor is unsafe fix (bluefireteam#499)

* Looking up a deactivated widget's ancestor is unsafe fix

* fixed assignment

Co-authored-by: André <andre.boerger@alte-leipziger.de>
@arcbueno
Copy link

Is there any release date, please? We are really needing this

@meg4cyberc4t
Copy link

Guys, WTF? With all due respect, the solution to this problem has been waiting for more than two months. Maybe it's just that we're doing something wrong, which is why we get an error? Please give a comment.

@KeithBacalso
Copy link

any release date for this fix? its Flutter 3 now

@S-Man42
Copy link

S-Man42 commented May 16, 2022

Is the owner still active anywhere? Maybe it's time for a fork?

@murali-cse
Copy link

The suggested change of @bounty1342 is looking interesting. I didn't move one controller to initState because it wasn't part of the exception. But it's correct that there is no reason to not do that. But I don't understand why you recreate the controller inside didChangeDependencies. This could lead to weird behavior in my opinion.

If you don't want to get the exception and you are willing to use the master branch, then you could change your dependency in pubspec.yaml to:

  photo_view:
    git: https://github.com/bluefireteam/photo_view.git

I still have the issue.. is there any other solution to solve this issue?

NM4ik added a commit to NM4ik/hotel_app that referenced this pull request May 20, 2022
@renancaraujo
Copy link
Member

0.14.0 is out!

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.

None yet