Add compose box for topic and DM narrows #180
Conversation
lib/widgets/compose_box.dart
Outdated
| /// The compose box for writing a stream message. | ||
| class StreamComposeBox extends StatefulWidget { | ||
| const StreamComposeBox({super.key, required this.narrow, required this.streamId}); | ||
|
|
||
| /// The narrow on view in the message list. | ||
| final Narrow narrow; | ||
| const StreamComposeBox({super.key, required this.narrow}); | ||
|
|
||
| final int streamId; | ||
| final StreamNarrow narrow; |
There was a problem hiding this comment.
compose [nfc]: Have StreamComposeBox take a StreamNarrow
This lets us deduplicate information between `narrow` and `streamId`.
I think this comment still applies; is there a reason to remove it?
/// The narrow on view in the message list.In particular, MentionAutocompleteView depends (well, will depend) on its passed narrow being that narrow.
It also would apply on the same-named property on the new _FixedDestinationComposeBox, later in the branch, I think.
I guess if we want to avoid repeating the same comment in both places, we could put it on the narrow property of ComposeBox, the common parent of those two widgets.
There was a problem hiding this comment.
In 8aec8f7, I also considered having this widget take a StreamNarrow instead of a Narrow plus stream ID. I decided not to, because it would mean depart from the doc, which I wasn't sure we wanted to do:
/// The compose box for writing a stream message.by changing the widget's use case:
- before, it was about what kind of message you could compose with it
- after, it's about how the adjacent message list is filtered (what kind of narrow is shown).
After thinking some more on this, and #149, I actually think it's a fine change to make, especially seeing your nicely simple implementation of a fixed-stream-and-topic compose box. But let's update this widget's doc to match its new purpose. We could even give the widget a more specific name, like StreamNarrowComposeBox, if you think that would be helpful.
There was a problem hiding this comment.
I think this comment still applies; is there a reason to remove it?
Yeah, I agree. I think what happened is just:
- I had a draft of this branch that replaced
int streamIdwithStreamNarrow narrow; - then that commit in the autocomplete PR added
Narrow narrow; - when I rebased and resolved the conflict at that hunk, I just took the new version from the change being rebased.
it would mean depart from the doc
Ah yeah, the doc should be amended. I'd missed that it said that, thanks.
As you see in this PR's code, I think there's a natural difference between "compose box that will send a stream message and needs a topic input, because it's shown on a stream narrow" vs. "compose box that will send a stream message and doesn't need a topic input, because it's shown on a topic narrow", and that's a bigger difference than between the latter and "compose box that will send a DM and doesn't need a topic input, because it's shown on a DM-conversation narrow".
| final ComposeContentController contentController; | ||
| final MessageDestination Function() getDestination; |
There was a problem hiding this comment.
I see that MessageDestination is marked for use in [sendMessage]:
/// Which conversation to send a message to, in [sendMessage].
///
/// This is either a [StreamDestination] or a [DmDestination].I could imagine it being convenient (in later work) to keep an up-to-date MessageDestination to be read in a build method, but from reading that doc, I wonder if you have reservations about doing that, perhaps because of performance.
I think reading an up-to-date MessageDestination in a build method could help us offer a more predictable send button, or compose box more generally. We could do that to conveniently show (in a tooltip or Semantics label, etc.) where the message will be sent, before the user has pressed the send button. We could show the user that the destination is unsendable (like #announce for most CZO users) before they've composed a message, instead of after.
There was a problem hiding this comment.
I could imagine it being convenient (in later work) to keep an up-to-date
MessageDestinationto be read in a build method
Sure, nothing wrong with that in general.
I think probably SendableNarrow will be more convenient — particularly because DmNarrow comes with handy other facilities on it, enabled by the invariants it maintains. That's why _FixedDestinationContentInput takes a SendableNarrow, rather than just a MessageDestination.
The reason the [MessageDestination] doc talks about [sendMessage], though, is just that [MessageDestination] is part of the API bindings, and [sendMessage] is the part of the API that it's for. It's similar to how the doc on [Anchor] says it's for [getMessages].
In particular the "in [sendMessage]" contrasts with how we specify a conversation when we talk to [getMessages], which is with an [ApiNarrow]. (Which needn't be a conversation, but very often is.)
This lets us deduplicate information between `narrow` and `streamId`.
This is a required part of the protocol for a state node to listen to objects that can change, as described in the docs for [State.initState] and [State.dispose]. I don't think it's real likely to come up for these particular widgets in practice -- I think they probably never will have the controllers swapped out on an existing state node. But best to follow the protocol anyway, as part of a pattern of systematically doing so.
I'll be giving these a common base class, and with the current pattern the natural name would be ComposeTextEditingController... which would look far too close to ContentTextEditingController. Instead, rename them to a pattern where they can each have the base class's name plus a word.
We're already computing this information eagerly on each edit, so we might as well do so in a way that lets us memoize centrally. This will also let us simplify, next, some of the stateful logic in the send button.
In particular because these methods are on a State, not a StatelessWidget, they have direct access to a BuildContext and so we don't need to pass one around.
Now that much of the state-managing logic has been moved out of this widget and into the text controllers, we mostly just make the topic controller nullable and sprinkle some `?.` operators around. We also let the caller specify how to compute the destination.
This separates the layout of the compose box from the setup of its state, which will let us share the former with compose boxes for other types of narrows.
gnprice
force-pushed
the
pr-compose-topic-dm
branch
from
e175073 to
c2c0599
Compare
|
Thanks for the review! Merging with the docs changes discussed above. |
Fixes: #144