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

Incomplete expansion state when expanding too many nodes in AnimatedTreeView #78

Open
TENX-S opened this issue Jan 15, 2024 · 4 comments
Labels
bug Something isn't working

Comments

@TENX-S
Copy link

TENX-S commented Jan 15, 2024

reproduction.mp4

I tweaked examples/minimal.dart a little bit to get minimal reproducible code:

import 'package:flutter/material.dart';
import 'package:flutter_fancy_tree_view/flutter_fancy_tree_view.dart';
import 'package:uuid/uuid.dart';

class Node {
  Node({
    required this.title,
    Iterable<Node>? children,
  }) : children = <Node>[...?children];

  final String title;
  final List<Node> children;
}

class MinimalTreeView extends StatefulWidget {
  const MinimalTreeView({super.key});

  @override
  State<MinimalTreeView> createState() => _MinimalTreeViewState();
}

class _MinimalTreeViewState extends State<MinimalTreeView> {
  late final TreeController<Node> treeController;

  @override
  void initState() {
    super.initState();

    treeController = TreeController<Node>(
      roots: getRoots(),
      childrenProvider: (Node node) => node.children,
    );
  }

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

  @override
  Widget build(BuildContext context) {
    return Column(
      children: [
        Builder(builder: (context) {
          return ElevatedButton(
            onPressed: () {
              (treeController.isTreeExpanded
                  ? treeController.collapseAll
                  : treeController.expandAll)();
              setState(() {});
            },
            child: Text(
                treeController.isTreeExpanded ? 'collapse all' : 'expand all'),
          );
        }),
        Expanded(
          child: AnimatedTreeView<Node>(
            treeController: treeController,
            shrinkWrap: true,
            duration: const Duration(milliseconds: 100),
            curve: Curves.easeInOutQuint,
            physics: const ClampingScrollPhysics(),
            maxNodesToShowWhenAnimating: 200,
            nodeBuilder: (BuildContext context, TreeEntry<Node> entry) {
              return TreeIndentation(
                entry: entry,
                child: Row(
                  children: [
                    ExpandIcon(
                      key: GlobalObjectKey(entry.node),
                      isExpanded: entry.isExpanded,
                      onPressed: (_) =>
                          treeController.toggleExpansion(entry.node),
                    ),
                    Flexible(
                      child: Text(entry.node.title),
                    ),
                  ],
                ),
              );
            },
          ),
        ),
      ],
    );
  }
}

final roots = [];

const _uuid = Uuid();

List<Node> getRoots() {
  var title = _uuid.v4;
  return List.generate(
      50,
      (index) => Node(
            title: title(),
            children: [
              Node(
                title: title(),
                children: List.generate(4, (index) => Node(title: title())),
              ),
              Node(
                title: title(),
                children: List.generate(4, (index) => Node(title: title())),
              ),
            ],
          ));
}

TreeView does not have this problem even if you generate 5000 nodes in getRoots() (but expand all nodes will freeze the app)

@baumths baumths added the bug Something isn't working label Jan 21, 2024
@baumths
Copy link
Owner

baumths commented Jan 21, 2024

Hey @TENX-S

I'm currently completely out of free time to work on this project, so I won't be able to look into this for quite a while.

If you're willing to submit a PR I'll do my best to find time to review it.

@baumths
Copy link
Owner

baumths commented Jan 21, 2024

I have marked both your issues as bugs for now until I have time to look further into it.

@baumths
Copy link
Owner

baumths commented Apr 11, 2024

I just took a look at this and could reproduce locally. Upon further investigation I couldn't find out why this is happening, might me an issue with how scrollables handle states in Flutter, but I'm not sure.

Unfortunatelly there's no quick fix for this from what I can tell. I might have to rewrite the SliverAnimatedTree logic to use a single AnimationController for the whole tree instead of one per animating node.

@baumths
Copy link
Owner

baumths commented May 4, 2024

Hey, sorry it took me so long to look further into this.

I've rewritten (simplified a lot) the animation implementation. The new implementation is way more performant (with some trade-offs) and relies on SliverAnimatedList to run the animations.

You can try it out on the dev/animation-improvements branch by depending on it via git refs:

dependencies:
  flutter_fancy_tree_view:
    git:
      url: git@github.com:baumths/flutter_tree_view.git
      ref: dev/animation-improvements

Trade-offs

The new implementation has some quirks, e.g.:

  • (Breaking Change) I had to deprecate the maxNodesToShowWhenAnimating feature because it breaks the node's index offset.
  • (Bug) When a node is expanded while it is running the collapse animation, the nodes are shown twice (duplicated).

Benefits of the new implementation:

  • Greatly improved performance
  • Fixes this issue, after calling expandAll(), all nodes are shown correctly
  • Fixes the issue where calling collapseCascading() would only animate the first level of the subtree and make all lower levels disappear instantly

Moving forward

I need some feedback about the new implementation before merging as I'm not sure if introducing such a breaking change should bump the major or minor package number. I don't think the maxNodesToShowWhenAnimating feature is used all that much and I don't have a way to collect data about it to decide.

As the new AnimatedTreeView implementation is way less chaotic, it could very well be merged into TreeView with a toggleable animation behavior (probably using the new AnimationStyle api) which would deprecate the use of AnimatedTreeView.

I'm going to let this sit as is for now while I work on another project and decide later. Any suggestions are very appreciated.

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

No branches or pull requests

2 participants