Refactor store: RealmStore; proxy mixins; move more methods to UserStore etc #1736
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…pass down params This is a bit simpler in itself, and also closer to what we'll want with the help of a substore.
This is the default when someone actually creates a custom profile field, so the more realistic default for test data.
The other substores could always handle this themselves individually, like Unreads does today with ChannelStore. But this is one substore that lots of others are going to want references to, so we might as well make that a bit easier.
This will help us move the underlying logic to live on a substore, which in turn will let us save these advance lookups and the annoying zulipFeatureLevel conditional.
This way, the method's implementation can look up zulipFeatureLevel and realmEmptyTopicDisplayName for itself, rather than have them looked up in advance by the caller and passed in. Then as a further consequence, the method only looks up realmEmptyTopicDisplayName if and when it determines it actually needs that information. That makes the lookup compatible with the check we have in the main realmEmptyTopicDisplayName getter to assert that it's only consulted when it should in fact be needed, on old servers. A hack in the message store, and a similar hack on the message list, therefore become unnecessary.
Before this change, if you hover in your IDE over an identifier whose declaration happens to be the first one after one of these section dividers, the doc shown would begin with a long string of slashes. After all, the divider line did start with "///". The choice of "|" as the substitute was fairly arbitrary. I tried a space first, and felt it interrupted the divider too much and made it look like just another line of comments. Done with a bit of Perl and Git: $ perl -i -0pe 's,^\s*//\K(?=//),|,gm' $(git grep -l //// '*.dart')
To identify which fields are realm settings in the API, so that zulip#668 applies to them, I looked at the current API docs: https://zulip.com/api/get-events#realm-update_dict It turns out a couple of items that had been realm settings are now something else; one that was a server setting (and even lacked "realm" in the name: maxFileUploadSizeMib) is now a realm setting updated by events. So the comments on the implementation fields need updating. I'll do that in a separate commit, though, so that this one is more purely moving code around.
The accurate version of these is now covered by a todo-comment up on RealmStore.
Instead, leave it as a method on EmojiStoreImpl and on the overall PerAccountStore. Each of these FooStore types, such as EmojiStore, is implemented by both a FooStoreImpl class and the overall PerAccountStore. Nearly all of these types' methods behave exactly the same on both those types; the one implementation just forwards to the other. But this method behaves slightly differently between them: the PerAccountStore implementation will call notifyListeners, while the EmojiStoreImpl implementation doesn't (because it can't, not being the ChangeNotifier itself). That mismatch, when nearly all the other similar methods have an exact match, risks confusion. Mitigate that by removing the method from the EmojiStore interface, so that the two implementations become unrelated methods. (The one other such mismatch is `reconcileMessages`. We'll deal with that one too, in a later commit.)
This doesn't move any tests, because this method has no tests. (Which at this point isn't a problem: for server versions we actually support, this method's logic becomes trivial.)
Like HasRealmStore for data about the realm, this will help make it convenient for other substores to refer to data about users.
This way, this substore becomes a valid home for methods that need to refer to data about both channels and users.
Ideally this would have been done when sendMessage itself was moved, in 942aa87 (zulip#1455), oops.
For most of these methods, we check the store isn't disposed within the substore's implementation; for a few, we check in the proxy implementation. Simplify the proxies by always checking in the substore's implementation.
Much like we did with setServerEmojiData in a recent commit, and for the same reasons: after setServerEmojiData, this was the only one of our numerous methods on the various FooStore interfaces where the two different implementations of the method actually had different behavior. That's potentially misleading, so make the two implementations be unrelated methods instead.
|
Very helpful, thanks much!! LGTM; merging. |
|
CI is failing, and interestingly, the same problem occurred to me now when I fetched from upstream and then ran |
|
Thanks — seems like definitely an upstream issue, then. Let's discuss in #mobile-team. |
|
The problem seems to be solved. Here's the discussion link: #mobile-team > Flutter upgrade - downloding Dark SDK fails. |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR makes a series of refactors to how PerAccountStore and its substores are organized, which I hope will help keep things clean to understand as the app's model has continued to gain complexity to handle the features we've been adding.
Specifically:
Selected commit messages
(Most of the most important commits are covered above, so omitted here.)
b30df8b realm [nfc]: Add Duration getters for durations
86ce025 dartdoc: Stop section-divider comments from getting absorbed into
docs
Before this change, if you hover in your IDE over an identifier whose
declaration happens to be the first one after one of these section
dividers, the doc shown would begin with a long string of slashes.
After all, the divider line did start with "///".
The choice of "|" as the substitute was fairly arbitrary. I tried a
space first, and felt it interrupted the divider too much and made it
look like just another line of comments.
Done with a bit of Perl and Git:
2b045d2 realm [nfc]: Organize realm settings, from current API doc
To identify which fields are realm settings in the API, so that #668
applies to them, I looked at the current API docs:
https://zulip.com/api/get-events#realm-update_dict
It turns out a couple of items that had been realm settings are now
something else; one that was a server setting (and even lacked "realm"
in the name: maxFileUploadSizeMib) is now a realm setting updated by
events. So the comments on the implementation fields need updating.
I'll do that in a separate commit, though, so that this one is
more purely moving code around.
db8809b emoji [nfc]: Drop setServerEmojiData from main interface
Instead, leave it as a method on EmojiStoreImpl and on the overall
PerAccountStore.
Each of these FooStore types, such as EmojiStore, is implemented by
both a FooStoreImpl class and the overall PerAccountStore. Nearly all
of these types' methods behave exactly the same on both those types;
the one implementation just forwards to the other.
But this method behaves slightly differently between them: the
PerAccountStore implementation will call notifyListeners, while the
EmojiStoreImpl implementation doesn't (because it can't, not being the
ChangeNotifier itself).
That mismatch, when nearly all the other similar methods have an
exact match, risks confusion. Mitigate that by removing the method
from the EmojiStore interface, so that the two implementations become
unrelated methods.
(The one other such mismatch is
reconcileMessages. We'll dealwith that one too, in a later commit.)
f83c9c1 message test [nfc]: Move sendMessage smoke test to follow sendMessage
Ideally this would have been done when sendMessage itself was moved,
in 942aa87 (#1455), oops.
9499dbe message [nfc]: Consolidate "disposed" checks onto substore methods
For most of these methods, we check the store isn't disposed
within the substore's implementation; for a few, we check in
the proxy implementation. Simplify the proxies by always checking
in the substore's implementation.