Mark as resolved/unresolved prep 1 #1242
Conversation
(String, int, bool, int) is pretty hard to make sense of.
Instead of passing its fields separately. (We'd like to start using lastUnreadId from the data too.)
This will make it available in a long-press handler in the app bar for a topic narrow. That long-press handler brings up the topic action sheet, which will need a message ID to pass in the API request for resolve/unresolve topic. (Not sure the API request should really *need* a message ID, but it requires it; shrug.)
We put it here in lib/api/route/messages.dart, next to another constant that's about topics, kMaxTopicLength.
Since it has a `propagate_mode` param that means the same as PropagateMode in an update-message event -- this is after all the update-message request binding -- move that enum's definition here, from lib/api/model, and adjust imports.
chrisbobbe
added
the
integration review
| String stripResolvePrefixIfPresent(String topic) => | ||
| topic.startsWith(kResolvedTopicPrefix) | ||
| ? topic.substring(kResolvedTopicPrefix.length) | ||
| : topic; |
There was a problem hiding this comment.
How does this end up getting used later in your branch?
Its behavior differs from unresolve_name in the shared code:
https://github.com/zulip/zulip/blob/1fac99733/web/shared/src/resolved_topic.ts#L22-L29
which I'm guessing (haven't checked) is what gets applied when the user unresolves a topic in web or the legacy mobile app.
| /// When built from server data, the action sheet ignores changes in that data; | ||
| /// we intentionally don't live-update the buttons on events. | ||
| /// If a button's label, action, or position changes suddenly, | ||
| /// it can be confusing and make the on-tap behavior unexpected. | ||
| /// Better to let the user decide to tap |
There was a problem hiding this comment.
Hmm, yeah.
Looking for PerAccountStoreWidget.of calls in this file, I think there's one place we don't currently follow this: the hasSelfVote in ReactionButtons.
That one shouldn't cause this problem in practice, though, because it changes by the user's own action. (Or by the message getting deleted, but then it doesn't matter whether the user tries to add a reaction or remove one.)
| /// Better to let the user decide to tap | ||
| /// based on information that's comfortably in their working memory, | ||
| /// even if we sometimes have to explain (where we handle the tap) | ||
| /// that that information has changed and they need to decide again. |
There was a problem hiding this comment.
A key point here is that these races are always possible anyway — the change may have happened on the server but not reached us yet.
| @@ -1,5 +1,6 @@ | |||
| import 'package:json_annotation/json_annotation.dart'; | |||
|
|
|||
| import '../route/messages.dart'; | |||
There was a problem hiding this comment.
Hmm, I don't like having lib/api/model/ import from lib/api/route/. If it's needed for the model, that feels like a sign it's part of the model and not part of how particular routes interact with the model.
I think these can just stay in this file.
There was a problem hiding this comment.
(When this file gets to feel unwieldy, I think the thing to do is to split pieces out by subject area: like a lib/ap/model/message.dart, akin to the narrow.dart and submessage.dart and so on that we already have.)
| /// As in [UpdateMessageEvent.propagateMode]. | ||
| @JsonEnum(fieldRename: FieldRename.snake) | ||
| enum PropagateMode { |
There was a problem hiding this comment.
And this can move from lib/api/model/events.dart to lib/api/model/model.dart, since it's no longer specific to an event.
My local branch for #744 has gotten pretty long and I think it'll benefit from being split into multiple PRs. 🙂 Here's one.