[Bug]: PersistentTabView.dispose unconditionally disposes the controller

[lukehutch]

Version

5.2.3

Flutter Doctor Output

Doctor summary (to see all details, run flutter doctor -v):
[✓] Flutter (Channel beta, 3.22.0-0.3.pre, on Fedora Linux 39 (Workstation Edition) 6.7.7-200.fc39.x86_64, locale en_US.utf8)
[✓] Android toolchain - develop for Android devices (Android SDK version 34.0.0)
[✓] Chrome - develop for the web
[✓] Linux toolchain - develop for Linux desktop
[✓] Android Studio (version 2023.3)
[✓] VS Code (version 1.89.0)
[✓] VS Code (version 1.88.0-insider)
[✓] Connected device (3 available)
[✓] Network resources

What platforms are you seeing the problem on?

No response

What happened?

PersistentTabView does this in initState:

    _controller = widget.controller ??
        PersistentTabController(
          initialIndex: widget.navigationShell?.currentIndex ?? 0,
        );

but then does this in dispose:

    _controller.dispose();

This can cause the controller to be disposed twice. If the parent provides the controller, then the parent should dispose the controller. The correct code in PersistentTabView.dispose is:

    if (widget.controller == null) {
      _controller.dispose();
    }

Steps to reproduce

--

Code to reproduce the problem

--

Relevant log output

No response

Screenshots

No response

[jb3rndt]

Hi, thank you for bringing this up. I agree that the controller that is created in a parent widget should be its sole responsibility. But I'm sure everyone right now relies on the PersistentTabView to dispose the controller in any case. So I suggest to either a) release this change with a major version an require everyone with a manually created controller to migrate by writing their own dispose method (does not sound very developer friendly) b) add an additional flag that signals whether the controller should be automatically disposed (if it is a manually created controller) or not, which defaults to true

What do you think?

[lukehutch]

Well you definitely can't leave it how it is, the convention in Flutter is that if a caller provides an object, the caller must dispose that object.

I suggest deprecating the current parameter, and creating a second parameter with a slightly different name that the docs clearly indicate that the controller must be disposed by the creator of the controller, if it is provided.

Although honestly, I'm pretty sure that people know, or should know, that if they create a controller, it is their responsibility to dispose it. So maybe you can just change the behavior, and indicate that in the change log, hoping that people notice that.

Also, this particular UI widget is usually only created once in a program, so the controller being leaked is not usually going to be a problem, because the navigation bar itself is never disposed.

I know that people don't rely on the navigation bar being disposed, because my program actually tried to rebuild it, which resulted in all sorts of errors (the ! operator being used on null values), because it wasn't designed to be torn down and recreated.

So I say just make the change, probably nobody's program will suffer a leak.

[jb3rndt]

You're right, there probably wont be many leaked controllers. Thank you for taking care of this.

[RB-93]

Please update on this. When there is going to be a new version. This issue is creating more issues and affected user experience drastically.

[jb3rndt]

It is released with version 5.3.0