local echo (2/n): Rearrange queueId and sendMessage within store
#1455
Conversation
This file contains 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
PIG208
changed the title
queueId and sendMessage within store (2/n)queueId and sendMessage within store
rajveermalviya
approved these changes
Apr 2, 2025
rajveermalviya
added
integration review
PIG208
changed the title
queueId and sendMessage within storequeueId and sendMessage within store
gnprice
reviewed
Apr 2, 2025
lib/model/store.dart
Outdated
| @@ -420,6 +429,7 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, UserStore, Channel | |||
| final GlobalStore _globalStore; | |||
| final ApiConnection connection; // TODO(#135): update zulipFeatureLevel with events | |||
|
|
|||
| String queueId; | |||
There was a problem hiding this comment.
Suggested change
| String queueId; | |
| final String queueId; |
Right? (Just like it was when on UpdateMachine.)
lib/model/store.dart
Outdated
Comment on lines
1078
to
1076
| final String queueId; | ||
| String get queueId => store.queueId; |
There was a problem hiding this comment.
This getter is fine for keeping the commit small, but let's have a followup commit clean it up by having its callers refer directly to the store. I think most of them won't even get any longer — they'll replace something like updateMachine.queueId with store.queueId.
There was a problem hiding this comment.
The change needed removing this ends up being quite small, so I added it to the same commit.
test/model/store_test.dart
Outdated
Comment on lines
218
to
219
| connection.prepare(json: | ||
| deepToJson(eg.initialSnapshot()) as Map<String, dynamic> |
|
Thanks! I have updated the PR. |
MessageStore stands out as a better home for sendMessage, which was on PerAccountStore before we extracted all the separate stores. This will make it more natural to support outbox/local echoing.
This is an NFC because UpdateMachine would have thrown (before this change) when the null-check is not on PerAccountStore. This queueId will later be used for local-echoing.
|
Thanks for the revision! Looks good; merging. |
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.
Related: