Add UserStore; pull out getUser, userDisplayName, others

[gnprice]

I've been saying for a while that I plan to make a UserStore type akin to ChannelStore and MessageStore. Today merging #1148 and in particular 76425ed gave me a fresh reminder that such a thing would be useful. And I decided some nice refactoring would be a relaxing task, so I sat down and did it. 🙂

The immediate motivating improvement is the new userDisplayName method, which simplifies a lot of spots that were otherwise falling back explicitly to the "(unknown user)" translation.

There's also a bunch of new dartdoc, particularly on the subject of unknown users (cf #716), which there effectively wasn't room for when it would have had to live in the middle of the PerAccountStore definition. And at the end of the branch, the actual Map of users becomes private — the bulk of the former references to it now use getUser, as they're looking up a particular user by ID, while two others stand out (as they should) because they iterate through the new allUsers, an expensive operation.

Along the way, and basically as a side effect of looking at all the code touched in this PR, the branch also includes identifying some spots that need to change for #716 (handling unknown users), plus commenting (in the "autocomplete [nfc]: Make explicit …" commit below) on a couple of spots that look like they might need to change for that issue but in fact don't.

These changes are all NFC with the small exception noted in the first commit.

Selected commit messages

6f82fda user: Split a UserStore out from PerAccountStore

Like ChannelStore and others, this helps reduce the amount of
complexity that's concentrated centrally in PerAccountStore.

This change is all NFC except that if we get a RealmUserUpdateEvent
for an unknown user, we'll now call notifyListeners, and so might
cause some widgets to rebuild, when previously we wouldn't.
That's pretty low-stakes, so I'm not bothering to wire through
the data flow to avoid it.

c253f93 user [nfc]: Document users map, especially its incompleteness

c448083 user [nfc]: Factor out a userDisplayName

06be949 store [nfc]: Add a zulipFeatureLevel getter

This is more convenient than going through store.connection,
and should reduce the temptation to go through store.account.

2063975 user [nfc]: Add a selfUser getter

196966d user [nfc]: Introduce senderDisplayName

7103868 user [nfc]: Note unknown-user crashes where senderDisplayName can help

ff9c6a9 user [nfc]: Note places lacking live-update where senderDisplayName helps

f62379a autocomplete [nfc]: Make explicit why two user lookups have null-assertions

40c06f2 user [nfc]: Factor out a getUser method

3655582 user [nfc]: Factor out an allUsers iterable

4b42269 user [nfc]: Make the actual users Map private

This gives somewhat better encapsulation -- other code isn't expected
to add or remove items from this map.

[gnprice]

A series of follow-up changes that are enabled by this, but that I didn't get to before deciding I was done with this for the evening:

• Move hasPassedWaitingPeriod to the UserStore mixin.
• As a prerequisite for that, make a RealmSettingStore mixin along the same lines. (That way we can say mixin UserStore on RealmSettingStore so that a method defined on UserStore can access realmWaitingPeriodThreshold.)
• Then move hasPostingPermission to ChannelStore, after writing mixin ChannelStore on UserStore so that it can access hasPassedWaitingPeriod.

That would be a nice further improvement because those two methods are two of the very few remaining bits of specific model logic remaining on PerAccountStore. Moving them out from there would therefore get PerAccountStore very close to its pure desired form as something that just stitches together and orchestrates all the different parts of the Zulip data model, delegating all the details to other more-specialized classes defined in other files.

[PIG208]

Thanks for the PR! Left some comments.

[PIG208]

Really convenient!

[PIG208]

I can see that this assertion was already ineffective — data stores like TypingStatus, Unreads, and RecentDmConversationsView could all take different selfUserIds without detecting the discrepancy.

In contrast, the three surviving assertions check that realmUrl is consistent across the data stores. While we can do that for selfUserId too, I'm not sure if that's useful. A bug can slip through if we add a new realmUrl without adding a new assertion below, and the same issue applies to selfUserId assertions if we are to maintain them.

Perhaps a better way would be sharing the reference to realmUrl and selfUserId, instead of passing them multiple times, but the way PerAccountStore.fromInitialSnapshot is set up already makes it easy to check for mistakes, making this less of an issue.

[gnprice]

Yeah, this check is basically already internal to the implementation of the PerAccountStore constructors, so I think it's not essential to keep.

[gnprice]

Perhaps a better way would be sharing the reference to realmUrl and selfUserId, instead of passing them multiple times,

This is also something I think we'll want to do — what I'm imagining is making a type named perhaps BasePerAccountStore, with those and perhaps connection and/or account (which subsumes them), and then these other sub-stores can say on BasePerAccountStore to get access to those getters.

(Naturally that's for a future PR, though.)

[PIG208]

Need async here. Expressions like store.addUser(…) need to be awaited.

[gnprice]

Hmm, yeah.

That doesn't have any actual effect on behavior, because handleEvent doesn't have any awaits in it. But it will in the future, for the reasons described in baea6d7 which made that method async, so in principle these should get awaited.

Looks like there are a small number of other places in the tests that lack such awaits. I'll fix those too while at it.

[PIG208]

There are some outstanding references to store.users the Map.

$ git grep 'store\.users'
test/model/autocomplete_test.dart:  test('MentionAutocompleteView mutating store.users while in progress does not '
test/widgets/recent_dm_conversations_test.dart:        testWidgets('no error when user somehow missing from store.users', (tester) async {
test/widgets/recent_dm_conversations_test.dart:        testWidgets('no error when one user somehow missing from store.users', (tester) async {

[gnprice]

Thanks for the review! Pushed an update (including rebasing atop #1331 so that CI should work); PTAL.