msglist [nfc]: Handle start markers within widget code, not model

[gnprice]

The view-model already exposes flags that express all the same information. By leaving these markers out of the list of items, we can save ourselves a bunch of stateful updates.

The main commit has a nice negative diffstat:

 lib/model/message_list.dart       | 49 ---------------------------------------------
 lib/widgets/message_list.dart     | 21 ++++++++++++++-----
 test/model/message_list_test.dart | 11 +---------
 3 files changed, 17 insertions(+), 64 deletions(-)

This comes up as part of my work toward #82: with this approach, we'll avoid needing to maintain marker items at the end of the list to depend on whether we've fetched the newest messages. Those markers would be more complex than the markers at the start, because they'd also include things like the typing-status line and the mark-as-read button.

@PIG208 I think this also makes for a small simplification in handling outbox-messages, in #1453.

[PIG208]

Thanks! It's neat and I have rebased my outbox changes on top of this. Overall looks good to me. Left some comments.

[PIG208]

This makes me wonder if dispose can ever get called before _initModel.

I guess the answer is no. When dispose is called, the State should be in its "ready" state.

  void dispose() {
    assert(_debugLifecycleState == _StateLifecycle.ready);
    assert(() {
      _debugLifecycleState = _StateLifecycle.defunct;
      return true;
    }());
    assert(debugMaybeDispatchDisposed(this));
  }

and from the dart doc of _StateLifecycle, "ready" is later than "created" and "initialized" in State objects' lifecycle:

/// Tracks the lifecycle of [State] objects when asserts are enabled.
enum _StateLifecycle {
  /// The [State] object has been created. [State.initState] is called at this
  /// time.
  created,

  /// The [State.initState] method has been called but the [State] object is
  /// not yet ready to build. [State.didChangeDependencies] is called at this time.
  initialized,

  /// The [State] object is ready to build and [State.dispose] has not yet been
  /// called.
  ready,

  /// The [State.dispose] method has been called and the [State] object is
  /// no longer able to build.
  defunct,
}

so State.didChangeDependencies (and then onNewStore) should have been called already.

On a side-note, we have been cautious about this at other places. While the framework might maintain this guarantee that onNewStore is called before dispose is called, it's probably easier to rest assure that ?.dispose() is always going to be safe. So I'm actually not sure if we want a null-assert here.

[gnprice]

Cool, thanks for the investigation.

Given those results, I think there would be some value in systematically having our dispose methods assume that didChangeDependencies (and so onNewStore where applicable) has been called. By visibly relying on that guarantee from the framework, it'd help teach the reader that that guarantee is there, and give confidence in relying on it in whatever new code they're writing.

That'd be an independent change from this one, though.

[PIG208]

Yeah, _EmojiPickerState seems like a place where a similar refactor may apply. The teaching aspect of this is refreshing think about.

[PIG208]

assert(fetched) being absent seems like a notable change here.

I suppose _updateEndMarkers has it because it nothing from items matter when fetched is false, because the widget code will just show the loading indicator and skip building the list view altogether.

I feel that it still applies here, right? _buildStartCap shouldn't be called at all if nothing has been fetched yet.

[gnprice]

Yeah, the bulk of the code on _MessageListState runs only when model.fetched is true. When model.fetched is false, the actual list doesn't get shown, so none of the "build" code runs beyond the first line of the build method.

In MessageListView (or _MessageSequence) where _updateEndMarkers lives/lived, the control flow between different methods is a lot more complicated (going up and down in the file), and so that's less clear.

I think this method's logic also makes sense even without the assumption that model.fetched is true. If we're fetching more, we say we're doing so; if we know we have the oldest messages, we say so; else say nothing. In _updateEndMarkers, there's the additional complication that the method is mutating items, so in order to think through the logic one wants to narrow down the possible states that items could be in.

[gnprice]

Thanks for the review! Replied above; merging.