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

A ScrollController was used after being disposed. #350

Open
scalz opened this issue Dec 14, 2024 · 2 comments
Open

A ScrollController was used after being disposed. #350

scalz opened this issue Dec 14, 2024 · 2 comments
Labels
bug Something isn't working in triage

Comments

@scalz
Copy link

scalz commented Dec 14, 2024

Bug report

Describe the bug
Hello. Thank you for sharing your amazing work.

When providing a ScrollController to a SliverWoltModalSheetPage, when resizing window (tested on desktop/Windows), this error happens: "A ScrollController was used after being disposed."

Because I guess WoltModalSheetAnimatedSwitcher disposes _scrollControllers.

Am I doing something wrong, or is it a bug?

I'm using a workaround (I modified WoltModalSheetAnimatedSwitcher), in dispose(), by disposing only if scrollcontroller is "internal", and if external, removing its position listener (recreated by _subscribeToCurrentPageScrollPositionChanges).
For the moment it only works if I use a topbar (but this does the job for the moment as I'm using a topbar).
I'll try to dig in package to better understand how everything works, but you probably have a better idea how to fix it than me :)

Thank you in advance for your help.

Steps to reproduce

Steps to reproduce the behavior:

  1. Reproducible example based on Basic Example from package
import 'package:flutter/material.dart';
import 'package:wolt_modal_sheet/wolt_modal_sheet.dart';

void main() {
  runApp(
    const WoltModalSheetApp(),
  );
}

class WoltModalSheetApp extends StatelessWidget {
  const WoltModalSheetApp({super.key});

  @override
  Widget build(BuildContext context) {
    return const MaterialApp(
      home: WoltModalSheetHomePage(),
    );
  }
}

class WoltModalSheetHomePage extends StatelessWidget {
  const WoltModalSheetHomePage({super.key});

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: const Text('Wolt Modal Bottom Sheet Sample'),
      ),
      floatingActionButton: FloatingActionButton.extended(
        onPressed: () async {
          final scrollController = ScrollController();
          await WoltModalSheet.show(
            context: context,
            pageListBuilder: (bottomSheetContext) => [
              SliverWoltModalSheetPage(
                scrollController: scrollController,
                mainContentSliversBuilder: (context) => [
                  SliverList.builder(
                    itemBuilder: (context, index) {
                      return ListTile(
                        title: Text('Index is $index'),
                        onTap: Navigator.of(bottomSheetContext).pop,
                      );
                    },
                  ),
                ],
              )
            ],
          );
          scrollController.dispose();
        },
        label: const Text('Trigger Wolt Sheet'),
      ),
    );
  }
}
  1. Run as Desktop app
  2. Resize window to switch layouts between bottomsheet and dialog, and you should get an error.

Expected behavior

When providing an external ScrollController to a SliverWoltModalSheetPage, we should be responsible of disposing it.


Additional context

I stumbled on this error while I wanted to programmatically scroll to end of the list (for example when receiving an api event, adding an element + scrolling to end).


@scalz scalz added bug Something isn't working in triage labels Dec 14, 2024
@ulusoyca
Copy link
Collaborator

Hi,

Thank you for reporting this bug, and sorry for being unresponsive due to holiday slow-down time. Would you have time to debug with me?

I suspect that the bug could be caused by this line:

  void _resetScrollControllers() {
    _scrollControllers.clear();
    _scrollControllers = [
      for (int i = 0; i < _pagesCount; i++)
        (_page.scrollController ??
            ScrollController(initialScrollOffset: _scrollPositions[i].value)),
    ];
  }

this should have been:

(widget.pages[i].scrollController ??
            ScrollController(initialScrollOffset: _scrollPositions[i].value)),

WDYT?

@scalz
Copy link
Author

scalz commented Dec 30, 2024

Hi.

No problem :)

I just tried your idea but it's not enough, controller is still getting disposed, in dispose() of WoltModalSheetAnimatedSwitcher.

The workaround I used for debugging:

Added a bool to _scrollControllers to keep track of which ones should be autodisposed by WoltModalSheetAnimatedSwitcher dispose().

List<({ScrollController c, bool autoDispose})> _scrollControllers = [];

ScrollController get _currentPageScrollController =>
    _scrollControllers[_pageIndex].c;
      
void _resetScrollControllers() {
  _scrollControllers.clear();
  _scrollControllers = [
    for (int i = 0; i < _pagesCount; i++)
      (c: (widget.pages[i].scrollController ??
          ScrollController(initialScrollOffset: _scrollPositions[i].value)),
      autoDispose: widget.pages[i].scrollController == null),
  ];
}

Created a method _currentPageScrollPositionChanges for adding and removing position Listener.
Dispose only scrollControllers which have autoDispose: true, and taking care of removing position Listener. Like this, custom scrollControllers don't get disposed.

  void _currentPageScrollPositionChanges() {
    if (_currentPageScrollController.hasClients) {
      _currentPageScrollPosition.value =
          _currentPageScrollController.position.pixels;
    }
  }

  void _subscribeToCurrentPageScrollPositionChanges() {
    for (final scrollController in _scrollControllers) {
      scrollController.c.addListener(_currentPageScrollPositionChanges);
    }
  }

  @override
  void dispose() {
    _animationController?.dispose();
    for (final element in _scrollControllers) {
      if (element.autoDispose) {
        element.c.dispose();
      }
      else element.c.removeListener(_currentPageScrollPositionChanges);
    }
    for (final element in _scrollPositions) {
      element.dispose();
    }
    super.dispose();
  }      

It works if I enable isTopBarLayerAlwaysVisible, which is fine for my case.
But if isTopBarLayerAlwaysVisible is disabled, then it triggers another error that I have not tried to fix yet.. snake bites its tail I guess.

ScrollController attached to multiple scroll views.
'package:flutter/src/widgets/scroll_controller.dart':
Failed assertion: line 172 pos 12: '_positions.length == 1'

The relevant error-causing widget was: 
  MaterialApp MaterialApp:file:///C:/projects/flutter/repro_wolt_scrollcontroller_issue/lib/main.dart:15:18
When the exception was thrown, this was the stack: 
#2      ScrollController.position (package:flutter/src/widgets/scroll_controller.dart:172:12)
#3      _TopBarFlowDelegate.currentScrollOffset (package:wolt_modal_sheet/src/content/components/main_content/wolt_modal_sheet_top_bar_flow.dart:81:54)
#4      _TopBarFlowDelegate.paintChildren (package:wolt_modal_sheet/src/content/components/main_content/wolt_modal_sheet_top_bar_flow.dart:98:11)```

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working in triage
Projects
None yet
Development

No branches or pull requests

2 participants