Add stream_id to Outbox values #5000
Conversation
gnprice
added
a-compose/send
gnprice
force-pushed
the
pr-outbox-streamid
branch
from
332be9b to
451bbe7
Compare
There was a problem hiding this comment.
Thanks, @gnprice! See just a few small comments below.
like showing the compose box on a wider variety of narrows
Yeah.
I think one obstacle to this is, when you're composing a new message, there are some kinds of narrows from which it's hard to determine whether the user wants to write a PM or a stream message. We currently can't show the compose box without knowing that in advance. Some of these narrows are ones where a normal workflow might include going to that narrow view and sending a message from it, like the home narrow.
The web app has nice "new topic" and "new private message" buttons, so the user can choose which interface to use when they're not replying to a specific message. (We've got #4379 for "new topic".)
For the case where the user does see a specific message that they want to reply to, I think the web app handles that nicely, too. It always keeps a visible pointer (with a blue outline) to a single message that's in view. From the message at the pointer, it's very easy to know whether to use the stream or PM interface for the compose box: just look at the message's type. But not only that—it means that an appropriate, fully specified destination narrow is always at hand, so it's easy to initialize the topic input (slash PM-recipients input, but we don't have that yet) with a sensible value.
I think that's a pretty reasonable way to get an "appropriate destination narrow" for composing a new message, and that if we implemented it, there'd be a short path to showing the compose box on all narrows if we wanted to—complete with a sensible value pre-populated in the topic input (or PM-recipient input if we grow one). So far, the "appropriate destination narrow" has sometimes been a simple function of the narrow in view (props.narrow), and it's sometimes given nothing or a wrong value. That function has developed in part from how we've seen stream narrows work on CZO, and our own educated guesses about what's least confusing for most users in their own stream narrows. I think this situation may force us to make some unfortunate tradeoffs. An example from this morning: we learned about some tension between #4764 and #4999 (see discussion).
I think we've said we don't want to keep a message pointer like the web app does, for basically a UI reason: mobile doesn't have keyboard shortcuts like the arrow keys. I do use my trackpad on the web a lot, and that's pretty similar to how scrolling works on mobile, and I like it. But I also just generally think we should keep track of how nice it is in the web app to have an appropriate destination narrow at hand all the time, in case we find a different, more acceptably mobile-friendly way to achieve that. Maybe we have a message pointer that's not as visible and that doesn't change with the scroll position, it just updates with every message you tap, or something.
| @@ -17,7 +17,7 @@ import InvalidNarrow from './InvalidNarrow'; | |||
| import { fetchMessagesInNarrow } from '../message/fetchActions'; | |||
| import ComposeBox from '../compose/ComposeBox'; | |||
| import UnreadNotice from './UnreadNotice'; | |||
| import { canSendToNarrow, caseNarrowDefault, keyFromNarrow } from '../utils/narrow'; | |||
| import { showComposeBoxOnNarrow, caseNarrowDefault, keyFromNarrow } from '../utils/narrow'; | |||
There was a problem hiding this comment.
compose [nfc]: Rename and document showComposeBoxOnNarrow
The name `canSendToNarrow` was misleading: we never actually send
to a whole-stream narrow, but this function returns true for them.
The real point is that we show the compose box when you're looking
at that narrow.
(And then the compose box offers a text input for a topic, and
`ComposeBox#getDestinationNarrow` ensures that we send to the
chosen topic's specific narrow.)
Commit-message nit:
And then the compose box offers a text input for a topic
It doesn't offer it when you're composing a new PM or editing one. Maybe one day it'll grow an input for PM recipients?
There was a problem hiding this comment.
Ah, the parenthetical was meant to be about the whole-stream case -- i.e. the case where this isn't a narrow you can actually send to. But that wasn't clear. I'll reword.
| return narrow; | ||
| }; | ||
|
|
||
| handleSend = () => { | ||
| const { dispatch } = this.props; | ||
| const { message } = this.state; | ||
| const narrow = this.getDestinationNarrow(); | ||
| const destinationNarrow = this.getDestinationNarrow(); |
There was a problem hiding this comment.
I'd be happy see this getDestinationNarrow helper replaced with a this.state.destinationNarrow. Would you be up for doing that, or should we do it later, or do you like it the way it is?
There was a problem hiding this comment.
The thing about that is that it'd mean we have two different state properties topic and destinationNarrow that need to stay in sync -- there'd be an invariant that there needs to be a certain relationship between them. So any place where we updated one, we'd need to be sure to appropriately update the other at the same time.
That certainly can be done, but it's something that can easily acquire subtle bugs. So I'd keep the function until we have a good reason to avoid it.
In a future where we determine the destination narrow in some other way, the function also keeps it flexible for us to change that: its callsites just keep calling it. It might in the future refer to some other piece of state (like a current message, with the idea in your main comment above), or to a state.destinationNarrow. If we end up doing the latter, it'd be an easy followup to replace the function calls with direct references to the state then.
There was a problem hiding this comment.
it'd mean we have two different state properties
topicanddestinationNarrowthat need to stay in sync
I agree that this would get tricky. But when the user is composing a PM, in some sense it's a bug that ComposeBox is maintaining a topic state at all. (In the same way that it would be a bug to maintain a PM-recipients state when composing a stream message.) I'd been thinking—see #4773 (comment) —that state.topic could basically be replaced in ComposeBox by a state.destinationNarrow. If we need lower-level management of the topic input's state (e.g., for validation that topics aren't empty), we could split off a child component for that, that shows the topic input exactly when we're composing a stream message. I guess extracting that new child component would make even more sense if we're planning to have a counterpart for handling a PM-recipients input for composing PMs.
Thinking some more about this, it occurs to me that this.state.destinationNarrow vs. this.getDestinationNarrow() will basically collapse once this becomes a Hooks component:
// (if these end up lingering in `ComposeBox` and not split off to new child components)
const [ topic, setTopic ] = useState(/* ... */);
const [ pmRecipients, setPmRecipients ] = useState(/* ... */);
const destinationNarrow = /* ... */;
// Then it's very easy to react to changes in `destinationNarrow` in a
// `useEffect`, e.g., to send typing-stop and typing-start notifications as
// the destination narrow changesSo I think after all we don't have a strong need to make a state.destinationNarrow now.
There was a problem hiding this comment.
Yeah. Another angle is to say that for any future where we change what's in the state, if that makes something like state.destinationNarrow make more sense than getDestinationNarrow, then we can make that change then. It won't be any harder of a refactor than it would be now, and that way we'll only do it if we're confident we'll actually have a need for it.
| invariant(stream, 'narrow must be known stream'); | ||
| return { | ||
| type: 'stream', | ||
| stream_id: stream.stream_id, |
There was a problem hiding this comment.
Thanks for doing this! Sorry I missed it in #4667. 🙂
The name `canSendToNarrow` was misleading: we never actually send to a whole-stream narrow, but this function returns true for them. The real point is that we show the compose box when you're looking at that narrow. (When you do look at a whole-stream narrow and we show the compose box, it then offers a text input for a topic, and ensures through its `getDestinationNarrow` that we send to the chosen topic's specific narrow rather than the whole stream.)
The asserted invariant follows from the documented one, because when `narrow` is a whole-stream narrow we always return a topic narrow instead. The documented invariant is satisfied because we only render a `ComposeBox` when the narrow is a PM conversation, a topic, or a stream. That's determined by the `showComposeBox` logic at its only parent, `ChatScreen`.
In the future, we might have a distinct type (a subtype of Narrow) for narrows that can be used as the destination for sending a message. At present, we don't. But we can at least make the distinction explicit in the names of parameters and locals. That helps trace the data flow, and it makes the code more readable in the context of `ComposeBox`, where there's another narrow floating around (namely `props.narrow`) which may be different from this one. All of these variables named `destinationNarrow` get their values from the return value of a call to `getDestinationNarrow`. While we're touching the `onSend` callback prop, it also benefits from a name to clarify the meaning of its string parameter.
It wouldn't make any sense to pass a whole-stream narrow here, and indeed we don't. The destination narrow passed to `addToOutbox` always comes from a return value of `ComposeBox#getDestinationNarrow`, and an assertion there already ensures that's a conversation. The parameter name `destinationNarrow` indicates the value should come from that method. Tracing from the two callsites of `addToOutbox` confirms that's the case: * `uploadFile` in `fetchActions`, called by `ComposeMenu`, where the narrow comes from its prop `destinationNarrow`. The only parent of `ComposeMenu` is `ComposeBox`, which gets that prop's value from `getDestinationNarrow`. * `ChatScreen`, in its `sendCallback` callback. That callback is passed only to `ComposeBox`, as its `onSend` prop; that in turn is invoked only at the latter component's `handleSend` method, where the narrow comes from `getDestinationNarrow`. With this assertion added here, we can cheerfully drop the code for this case (which, as the comment suspected, was already dead). If in the future we do pass a whole-stream narrow here, we'll get an exception, just as we already would have if passing another inappropriate narrow like starred-messages.
The old name made more sense when this returned a property called `to`, instead of one called `display_recipient`.
Compare a1fad7c, which did the same for the sender_id property. We actually did part of this a few months ago, in fce3e8b in zulip#4667. That commit added it to the type, but we hadn't yet taken the step of supplying this property in any actual Outbox values. Also add back to a TODO comment a bit that got dropped in 8171f2d. This makes zulip#3998 mostly complete: the remaining steps will be to get a release with this change out, wait a few weeks, then make `stream_id` required with a migration to drop `Outbox` values that lack it, just as we did for `sender_id` before. Issue: zulip#3998
gnprice
force-pushed
the
pr-outbox-streamid
branch
from
451bbe7 to
0c251bf
Compare
|
Thanks for the review! Merged, with that clarification in a commit message. |
We started supplying this property in 0c251bf / #5000. That went out to users a few weeks ago, in v17.171 which rolled out 2021-09-23, so by this point the bulk of our users have upgraded to that version. Now we mark the property as required, and drop any old outbox messages that lack it in the unlikely case that there are any. This completes #3998 because we've already done the same thing for `sender_id`, in a1fad7c / #4304 and then 60847c9 / #4667. Fixes: #3998
We started supplying this property in 0c251bf / zulip#5000. That went out to users a few weeks ago, in v17.171 which rolled out 2021-09-23, so by this point the bulk of our users have upgraded to that version. Now we mark the property as required, and drop any old outbox messages that lack it in the unlikely case that there are any. This completes zulip#3998 because we've already done the same thing for `sender_id`, in a1fad7c / zulip#4304 and then 60847c9 / zulip#4667. Fixes: zulip#3998
This makes #3998 mostly complete: the remaining steps will be to get a release with this change out, wait a few weeks, then make
stream_idrequired with a migration to drop Outbox values that lack it, just as we did forsender_idbefore.Along the way, we pin down some invariants about the different narrows involved in the compose code. I remember having to work through some of this same reasoning in order to verify some of the refactoring in #4773 (around the
onSendcallback, IIRC); hopefully the added jsdoc and asserts here will help memoize that work for the future, and also ensure the logic gets revised appropriately when we make changes that would affect its assumptions (like showing the compose box on a wider variety of narrows.)