karvulf / flutter-reorderable-grid-view

BSD 3-Clause "New" or "Revised" License
158 stars 23 forks source link

Modifying item list cause a rebuild of every item even those unchanged #86

Closed gmarizy closed 2 weeks ago

gmarizy commented 1 year ago

If I modify item list (calling ReorderableBuilder changing parameter children) the whole ReorderableBuilder is rebuild, with all its children, even those unchanged from previous build. Is there something I can do to prevent this ? I tried to use keys on every level (ReorderableBuilder, GridView, items) with no success. Or is this dependent on ReorderableBuilder ? In addition to reordering, I offer the possibility to add or remove items. I use this lib to reorder photos, meaning rebuild is a bit expensive and cause a glitch.

karvulf commented 1 year ago

Can you share the code with me? Sounds like the keys could change after the rebuilt @gmarizy

gmarizy commented 1 year ago

Better than sharing my code, I adapted the sample from readme to illustrate the problem. As you click 'REMOVE LAST' button, you can see in console that remaining _Items did rebuild.

import 'dart:math';

import 'package:flutter/material.dart';
import 'package:flutter_reorderable_grid_view/entities/order_update_entity.dart';
import 'package:flutter_reorderable_grid_view/widgets/widgets.dart';

class MyApp extends StatefulWidget {
  const MyApp({Key? key}) : super(key: key);

  @override
  _MyAppState createState() => _MyAppState();
}

class _MyAppState extends State<MyApp> {
  final _scrollController = ScrollController();
  final _gridViewKey = GlobalKey();
  final _reorderableBuilderKey = GlobalKey();
  final _fruits = ValueNotifier(["apple", "banana", "strawberry"]);

  @override
  Widget build(BuildContext context) => Scaffold(
      body: ValueListenableBuilder<List<String>>(
        valueListenable: _fruits,
        builder: (context, fruits, _) {
          final generatedChildren = List.generate(
            fruits.length,
                (index) => _Item(
              fruits: fruits,
              index: index,
              key: Key(fruits.elementAt(index)),
            ),
          );

          return ReorderableBuilder(
            key: _reorderableBuilderKey,
            children: generatedChildren,
            scrollController: _scrollController,
            onReorder: (List<OrderUpdateEntity> orderUpdateEntities) {
              for (final orderUpdateEntity in orderUpdateEntities) {
                final fruit = fruits.removeAt(orderUpdateEntity.oldIndex);
                fruits.insert(orderUpdateEntity.newIndex, fruit);
              }
            },
            builder: (children) {
              return GridView(
                key: _gridViewKey,
                controller: _scrollController,
                children: children,
                gridDelegate: const SliverGridDelegateWithFixedCrossAxisCount(
                  crossAxisCount: 4,
                  mainAxisSpacing: 4,
                  crossAxisSpacing: 8,
                ),
              );
            },
          );
        }
      ),
    bottomSheet: ElevatedButton(
      onPressed: () {
        _fruits.value = _fruits.value.sublist(0, max(0, _fruits.value.length - 1));
      },
      child: Text("REMOVE LAST"),
    ),
    );
}

class _Item extends StatelessWidget {
  const _Item({
    super.key,
    required List<String> fruits,
    required this.index
  }) : _fruits = fruits;

  final int index;
  final List<String> _fruits;

  @override
  Widget build(BuildContext context) {
    print("build index: $index");
    return Container(
      color: Colors.lightBlue,
      child: Text(
        _fruits.elementAt(index),
      ),
    );
  }
}
karvulf commented 1 year ago

Your remove last part looks weird, you could also call removeLast on the list and then call setState to update your widget @gmarizy

karvulf commented 1 year ago

And I don’t understand why you use the ValueListenableBuilder. How your code looks you don’t need it if you just use setState @gmarizy

gmarizy commented 1 year ago

This is with setState:

import 'dart:math';

import 'package:flutter/material.dart';
import 'package:flutter_reorderable_grid_view/widgets/widgets.dart';

final _gridViewKey = GlobalKey();
final _reorderableBuilderKey = GlobalKey();

class MyApp extends StatefulWidget {
  const MyApp({Key? key}) : super(key: key);

  @override
  _MyAppState createState() => _MyAppState();
}

class _MyAppState extends State<MyApp> {
  final _scrollController = ScrollController();
  List<String> _fruits = ["apple", "banana", "strawberry"];

  @override
  Widget build(BuildContext context) {
    final generatedChildren = List.generate(
      _fruits.length,
          (index) => _Item(
        fruits: _fruits,
        index: index,
        key: Key(_fruits.elementAt(index)),
      ),
    );
    return Scaffold(
    body: ReorderableBuilder(
            key: _reorderableBuilderKey,
            children: generatedChildren,
            scrollController: _scrollController,
            onReorder: (_) {},
            builder: (children) {
              return GridView(
                key: _gridViewKey,
                controller: _scrollController,
                children: children,
                gridDelegate: const SliverGridDelegateWithFixedCrossAxisCount(
                  crossAxisCount: 4,
                  mainAxisSpacing: 4,
                  crossAxisSpacing: 8,
                ),
              );
            },
    ),
    bottomSheet: ElevatedButton(
      onPressed: () {
        setState(() {
          _fruits = _fruits.sublist(0, max(0, _fruits.length - 1));
        });
      },
      child: Text("REMOVE LAST"),
    ),
  );
  }
}

class _Item extends StatelessWidget {
  const _Item({
    super.key,
    required List<String> fruits,
    required this.index
  }) : _fruits = fruits;

  final int index;
  final List<String> _fruits;

  @override
  Widget build(BuildContext context) {
    print("build index: $index");
    return Container(
      color: Colors.lightBlue,
      child: Text(
        _fruits.elementAt(index),
      ),
    );
  }
}

And this is with a ValueListenableBuilder:

import 'dart:math';

import 'package:flutter/material.dart';
import 'package:flutter_reorderable_grid_view/widgets/widgets.dart';

class MyApp extends StatelessWidget {
  MyApp({Key? key}) : super(key: key);

  final _scrollController = ScrollController();
  final _gridViewKey = GlobalKey();
  final _reorderableBuilderKey = GlobalKey();
  final _fruits = ValueNotifier(["apple", "banana", "strawberry"]);

  @override
  Widget build(BuildContext context) => Scaffold(
    body: ValueListenableBuilder<List<String>>(
        valueListenable: _fruits,
        builder: (context, fruits, _) {
          final generatedChildren = List.generate(
            fruits.length,
                (index) => _Item(
              fruits: fruits,
              index: index,
              key: Key(fruits.elementAt(index)),
            ),
          );

          return ReorderableBuilder(
            key: _reorderableBuilderKey,
            children: generatedChildren,
            scrollController: _scrollController,
            onReorder: (_) {},
            builder: (children) {
              return GridView(
                key: _gridViewKey,
                controller: _scrollController,
                children: children,
                gridDelegate: const SliverGridDelegateWithFixedCrossAxisCount(
                  crossAxisCount: 4,
                  mainAxisSpacing: 4,
                  crossAxisSpacing: 8,
                ),
              );
            },
          );
        }
    ),
    bottomSheet: ElevatedButton(
      onPressed: () {
        _fruits.value = _fruits.value.sublist(0, max(0, _fruits.value.length - 1));
      },
      child: Text("REMOVE LAST"),
    ),
  );
}

class _Item extends StatelessWidget {
  const _Item({
    super.key,
    required List<String> fruits,
    required this.index
  }) : _fruits = fruits;

  final int index;
  final List<String> _fruits;

  @override
  Widget build(BuildContext context) {
    print("build index: $index");
    return Container(
      color: Colors.lightBlue,
      child: Text(
        _fruits.elementAt(index),
      ),
    );
  }
}

I tested both snippets with v4 and v5. It doesn't depend on state management; updating _fruits list then calling a rebuild imply rebuilding everything, even _Items that are unchanged and don't need a full rebuild.

karvulf commented 2 weeks ago

Hello @gmarizy a late answer but I checked the issue and unfortunately I cannot prevent that these calls happen because of the internal structure of this package which I cannot easily change. You could try to use Reorderable.builder which would be much more performant if you have a lot of images to show. In this case only a specific amount of children will be loaded which you can define in your GridView.builder and could decrease the performance issue. Also check if you can cache the image so that it won't be fully reloaded. Otherwise there isn't much I can suggest to you. I hope it helps a bit. I will close this issue.

gmarizy commented 2 weeks ago

@karvulf Thank you for trying. I did invest in multi-level caching yes.

karvulf commented 2 weeks ago

Did it help? @gmarizy

gmarizy commented 2 weeks ago

@karvulf Yes, caching mitigated the problem.