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

fix: 🐛 correct removal of newSuggestions listener #193

Conversation

jonasbadstuebner
Copy link
Contributor

Description

When disposing the Widget this to provide failed, thus the listener did not get removed properly.
This change fixes the call to removeListeners.

Checklist

  • The title of my PR starts with a Conventional Commit prefix (fix:, feat:, docs: etc).
  • I have followed the Contributor Guide when preparing my PR.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation in docs and added dartdoc comments with ///.
  • I have updated/added relevant examples in examples or docs.

Breaking Change?

  • Yes, this PR is a breaking change.
  • No, this PR is not a breaking change.

Related Issues

#187

@aditya-css
Copy link
Collaborator

Thanks for the PR @jonasbadstuebner. Can you please tell me more about the scenario where you faced the error?

What I have understood is that the extension provide is not able to give instance of ChatViewInheritedWidget when the dispose of SuggestionList method is called (which should not have happened). Let me know if my understanding of the issue is incorrect.

@jonasbadstuebner
Copy link
Contributor Author

With #191 being merged, this is not a "real" problem anymore, the dispose function just does not do what it should be doing. Since the context is not mounted anymore at this point, newSuggestions will always be null, thus the listener won't be removed. In practice, this might not be noticable, but it is better to remove it cleanly.

final newSuggestions = provide?.chatController.newSuggestions;
newSuggestions?.removeListener(animateSuggestionList);

I noticed the mistake because the example app threw the error: Looking up a deactivated widget's ancestor is unsafe.

You can reproduce this on 232214c by wrapping SuggestionList in a Container, causing the State to be disposed on the following line:

so it becomes

return Container(
  child: SuggestionList(
    suggestions: value,
  ),

and hot reloading the app.

This will produce the following error:

error log
══╡ EXCEPTION CAUGHT BY WIDGETS LIBRARY ╞═══════════════════════════════════════════════════════════
The following assertion was thrown while finalizing the widget tree:
Looking up a deactivated widget's ancestor is unsafe.
At this point the state of the widget's element tree is no longer stable.
To safely refer to a widget's ancestor in its dispose() method, save a reference to the ancestor by
calling dependOnInheritedWidgetOfExactType() in the widget's didChangeDependencies() method.

When the exception was thrown, this was the stack:
#0      Element._debugCheckStateIsActiveForAncestorLookup.<anonymous closure> (package:flutter/src/widgets/framework.dart:4743:9)
#1      Element._debugCheckStateIsActiveForAncestorLookup (package:flutter/src/widgets/framework.dart:4757:6)
#2      Element.dependOnInheritedWidgetOfExactType (package:flutter/src/widgets/framework.dart:4776:12)
#3      ChatViewInheritedWidget.of (package:chatview/src/widgets/chat_view_inherited_widget.dart:22:15)
#4      StatefulWidgetExtension.provide (package:chatview/src/extensions/extensions.dart:132:67)
#5      _SuggestionListState.dispose (package:chatview/src/widgets/suggestions/suggestion_list.dart:96:28)
#6      StatefulElement.unmount (package:flutter/src/widgets/framework.dart:5696:11)
#7      _InactiveElements._unmount (package:flutter/src/widgets/framework.dart:2077:13)
#8      ListIterable.forEach (dart:_internal/iterable.dart:49:13)
#9      _InactiveElements._unmountAll (package:flutter/src/widgets/framework.dart:2086:25)
#10     BuildOwner.lockState (package:flutter/src/widgets/framework.dart:2765:15)
#11     BuildOwner.finalizeTree (package:flutter/src/widgets/framework.dart:3175:7)
#12     WidgetsBinding.drawFrame (package:flutter/src/widgets/binding.dart:1143:19)
#13     RendererBinding._handlePersistentFrameCallback (package:flutter/src/rendering/binding.dart:443:5)
#14     SchedulerBinding._invokeFrameCallback (package:flutter/src/scheduler/binding.dart:1392:15)
#15     SchedulerBinding.handleDrawFrame (package:flutter/src/scheduler/binding.dart:1313:9)
#16     SchedulerBinding.scheduleWarmUpFrame.<anonymous closure> (package:flutter/src/scheduler/binding.dart:1035:9)
#17     PlatformDispatcher.scheduleWarmUpFrame.<anonymous closure> (dart:ui/platform_dispatcher.dart:837:16)
#21     _RawReceivePort._handleMessage (dart:isolate-patch/isolate_patch.dart:184:12)
(elided 3 frames from class _Timer and dart:async-patch)
════════════════════════════════════════════════════════════════════════════════════════════════════

As I said, this is not a problem anymore on main, but the listener is currently also not removed, so it is not working as intended.

@aditya-css
Copy link
Collaborator

Yes, that's a good catch @jonasbadstuebner. dependOnInheritedWidgetOfExactType shouldn't be called from dispose method. Can you please revert your changes and just call the remove listener in deactivate method?

@jonasbadstuebner jonasbadstuebner force-pushed the fix-removal-of-newsuggestions-listener branch from dcc3ad9 to f08803b Compare June 18, 2024 20:36
@jonasbadstuebner
Copy link
Contributor Author

I added the addListener call to activate too, because:

the framework will call activate to give the State object a chance to reacquire any resources that it released in deactivate.

Also about activate:

The framework does not call this method the first time a State object is inserted into the tree. Instead, the framework calls initState in that situation.

@aditya-css aditya-css merged commit b005e55 into SimformSolutionsPvtLtd:main Jun 19, 2024
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