marcossevilla / lazy_indexed_stack

A Flutter package that exposes an IndexedStack that can be lazily loaded.
https://pub.dev/packages/flutter_lazy_indexed_stack
MIT License
28 stars 3 forks source link

Crash if length of children changes #23

Open milesegan opened 1 year ago

milesegan commented 1 year ago

Description The LazyIndexedStack can crash if the length of children changes.

Steps To Reproduce

  1. Render the widget.
  2. Change the length of children and then change the index to point to one of the new children.
  3. The widget will crash trying to index past the end of the _activatedChildren list.

https://github.com/marcossevilla/lazy_indexed_stack/blob/0d156173f7fa19a330178721ee2b51833a48f79d/lib/src/flutter_lazy_indexed_stack.dart#L37

Maybe it would be safer to use a set like this:

  late final _activatedChildren = <int>{widget.index};

Thanks for the library by the way!

marcossevilla commented 1 year ago

hi @milesegan! I will look into this but I was curious what was the use case that generated this issue, like why did you change the length of the children and then pointed to one that wasn't on the list? This would help me understand better how the API should solve your issue without breaking changes 👍

milesegan commented 1 year ago

@marcossevilla In my case I have a responsive app that has a different number of items depending on whether it's on a small or a large screen. So when the user resizes the app from small to large the number of items in the stack increases. Maybe this is an unusual situation?

In any case I think you could make this change to the implementation without changing anything in the API of LazyIndexedStack.

marcossevilla commented 1 week ago

@milesegan sorry for the late reply, I can try reassigning the children on didUpdateWidget if they change but this would reset the stack active children. would that be the behavior you're expecting?

milesegan commented 1 week ago

@marcossevilla Thanks for the reply. I've been using a patched version of the stack storing them in a set instead of a list like this:

  late final _activatedChildren = <int>{widget.index};

This has been working well for me for a while now.

marcossevilla commented 1 week ago

@milesegan alright, you feel that should be the way it's handled in the package? I don't really see advantages aside from your use case.

milesegan commented 1 week ago

Well I think the main issue with the current implementation is that it's quite easy to crash the app.