[RFC]: NeonListView performance

[Leptopoda]

As talked about on matrix I found that the layout phase of NeonListView takes to long. This is probably the case because the main content isn't built lazily.

Just asking about how we would approach a refactor of this (and related) widget/s. IMO we should only expose a lazy way for building this Widget. Consumers of neon shouldn't be able to go the wrong route.

Also I'd love to remove the withFloatingActionButton as we could just search the the WidgetTree ourselves (thinking of Scaffold.of(context).hasFloatingActionButton).

1. How should the API of the widget be?

Currently all implementations expect a list of all items and the builder function to return the context and an item while the flutter style would be to get an itemCount and a builderfunction that provides the context and an index (see NullableIndexedWidgetBuilder).

Both are possible and the current code is tailored towards the first one.

2. Move the Children into their own Widget?

Currently the children are all build in the builder provided to the NeonListView. Making them their own Widget would allow us to also make a PrototypeItem that we could provide to the list. Most (if not all) implementations of the List could profit of only needing to layout one child.

3. Provide a second NeonListView.ordered factory

Most implementations of the NeonListView are wraped by SortBoxBuilder (makes sense as lists are sortable). This would just be a convenience method and the old implementation would still be possible.

We could also just make a separate convenience Widget like NeonSortedListView that does this.

4. Improve SortBoxBuilder nullability.

When the list provided to the SortBoxBuilder is null we will still call the builder just without a list value. I think we should just return a container if the input is null and not build the children.

Currently this would lead to not rendering the entire NeonListView wich might be used to display an error in this case so this is only desirable when we have a merged version of both (see above).

5. Avoid nested SortBoxBuilder

The notes_view currently nests two SortBoxBuilder(s). One for the favorites and one for nonFavorites. This means we couldn't profit form such a mentioned NeonSortedListView. In theory the nesting is only done to sort based on the favorite state and then the notesSortPropertyOptions.

Recursive SortBox anyone :eyes: #276 (I already have an idea of how we could expose a nice api for the sortbox to support this but don't know if it would work yet)

---

Those are just ideas and I'd love some feedback on them. Obviously this would need separate PRs probably from the bottom up. We could also try to split up the NeonListView a bit into a NeonRefreshableScreen that basically holds everything around the scrolling view. This would also make it easier for clients that don't need a list as a screen (CookBook uses a grid for it's main screen and I'd love being able to still use the abstraction for it).

---

How a simple PR review with some performance testing can escalate in my head :upside_down_face:

[provokateurin]

Also I'd love to remove the withFloatingActionButton as we could just search the the WidgetTree ourselves (thinking of Scaffold.of(context).hasFloatingActionButton).

I didn't know that existed, let's use it.

1. We can go with the more flutter style, I don't really care. If it's easier for devs to adapt to, then that's the better way.
2. Can you explain this a little more? I don't know if I understood your idea correctly.
3. Good idea, let's go with the factory.
4. Also good improvement
5. I agree, but that sounds hard, so let's save it for later.

We could also try to split up the NeonListView a bit into a NeonRefreshableScreen that basically holds everything around the scrolling view. This would also make it easier for clients that don't need a list as a screen (CookBook uses a grid for it's main screen and I'd love being able to still use the abstraction for it).

Sure thing, it helps a lot that you know the cookbook use-cases :)

[Leptopoda]

5. I agree, but that sounds hard, so let's save it for later.

It's not that hard if I can pull of what I'm thinking of and might be needed when we tackle 4. In a nested SortBoxBuilder if the upper one is null it'd lead to the second one not even being rendered (that's the qhole point of 4.) But If I just don't have any favorite news articles I could have null on the first SortBox but still want the second one to be called. Therefore a nested one would be needed.

Yes this is a bad example as I'd assume in this case the first sort box wouldn't even get a null value and get an empty list instead but I hope you get my concern. But we could also leave that as the last thing no problem.

2. Currently we do:

final items = <Widget>[]
List(
  children:[
    for(item in items)...[
      buildChild(item),  
    ],
  ],
);

Widget buildChild(Widget child){
  // Do some Stuff
}

So during layout the framework will need to call the buildChild method items.length times. When only one item changes we again need to do all the work from the beginning.

Having the // Do some Stuff (the child/item) in it's own Widget would mean that the framework could detect when we call it with the same value and only need to rebuild/layout the changed child/item. This is the same as just wrapping it with a builder.

final items = <DATA>[]
List(
  children:[
    for(data in items)...[
      Child(data),  
    ],
  ],
);

class Child extends StatelessWidget{
  // Do some Stuff here
}

If Child is called during a rebuild but none of it's input values have changed (and the constraints are still the same) we expect the widget to be the same and it wont be rebuilt (might have to do something with the immutability of widgets).

As lazily laying out the ListView (The main point of the restructure) also provides a real builder function for it's children the expected performance gain of just extracting the child isn't that high. But: The framework would still need to calculate the size of every visible Widget to know how many will fit on the screen. If we know that they all are the same size (what they probably are as they're the same ListTily thing) we can provide the framework a so called prototype item. This means it only needs the calculate the size of one widget (The prototype) and use it's size for every subsequent one.

Does it make sense now?

---

Please treat this as a small overview and whoever reads this don't quote me on the specifics (small nuances might be expressed wrong. It's about the overall picture). If anyone is interested I'd recommend reading a bit about slivers as that's the backbone of flutters scrolling experience.

[Leptopoda]

5. I agree, but that sounds hard, so let's save it for later.

class SortBox<T extends Enum, R> {
  SortBox(
    this._properties,
    this._secondaryBoxes,
  );

  final Map<T, ComparableGetter<R>> _properties;
  final Map<int, Map<T, Box<T>>> _secondaryBoxes;

  List<R> sort(final List<R> input, final Box<T> box) {
    if (input.length <= 1) {
      return input;
    }

    final sorted = input..sort((final item1, final item2) => _compare(item1, item2, box));

    return sorted;
  }

  int _compare(
    final R item1,
    final R item2,
    final Box<T>? box, [
    final int itteration = 0,
  ]) {
    // Stop sorting when no adinitonal property is specified.
    if (box == null) {
      return 0;
    }

    final comparableGetter = _properties[box.property]!;

    final comparable1 = comparableGetter(item1);
    final comparable2 = comparableGetter(item2);

    final order =
        box.order == SortBoxOrder.ascending ? comparable1.compareTo(comparable2) : comparable2.compareTo(comparable1);

    final nextBox = _secondaryBoxes[itteration]?[box.property];

    if (order == 0) {
      return _compare(item1, item2, nextBox, itteration + 1);
    }

    return order;
  }
}

this works quite well. It would probably make more sense to use an Iterable<Map<T, Box<T>>> instead of the Map: Also I don't know if it makes sense but we could also use a Set (or are multiple occurrences expected? haven't thought about this one yet). We could also stick to the map and just switch it around Map<T, Map<int, Box<T>>> this would make the interface better to understand: "If property T has been chosen use the following Boxes in the specified order to continue"

Finally It makes sense as a sanity check to limit the number of iterations to the item count of T (the enum) as that's the maximum amount of properties to sort on (so just to prevent an "oversort").

The above does pass all tests but I haven't added more to test deeper sorting. Just the idea I had in my head with the recursive comparison I demonstrated in the last PR.

---

Also just because this works does not mean the interface makes sense :sweat_smile: Possibilities are there and we should clearly think about the interface but it's not that hard. Just three lines to adjust.

[Leptopoda]

Can we add this to 1.0 @provokateurin ?

At least the branch I have locally would be breaking.

[provokateurin]

It's not directly related, but I would like this as a follow up feature (could also be implemented now already): Show some text and an icon when the list is completely empty. I find it a but confusing if there is nothing at all and this is also done a lot on the web interface.

[Leptopoda]

Sure good idea.

Tbh I was thinking to completely deprecate the NeonListView. Iirc the only advantage it brings is having permanent header items and scrolling header ones. Both are trivial to implement with Slivers.

The top item like a searchbar or the files navigator could also be permanent but hide on scroll (similar to what the official client does).

The only potential I can see is to have the mentioned empty state handled or have a convenience NeonSortedLietaview that also includes the sortbox stuff.

[provokateurin]

I agree, having the convenience for sort box would be nice although we could probably just make this as a sliver, right?

[provokateurin]

So this can be closed because the remaining stuff is in https://github.com/nextcloud/neon/issues/811?