Skip to content

compose: Support editing an already-sent message #126

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
chrisbobbe opened this issue May 26, 2023 · 9 comments · May be fixed by #1498
Open

compose: Support editing an already-sent message #126

chrisbobbe opened this issue May 26, 2023 · 9 comments · May be fixed by #1498
Assignees
Labels
a-msglist The message-list screen, except what's label:a-content beta feedback Things beta users have specifically asked for

Comments

@chrisbobbe
Copy link
Collaborator

chrisbobbe commented May 26, 2023

Design from @terpimost:
image

Details are in Figma:
https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=4010-6347&m=dev

Updates from the details in Figma:

  • For the text when saving the edit is in progress let's say "saving edit…" rather than "sending message…".
  • For the text when saving the edit fails let's say "edit wasn't saved" rather than "message wasn't sent".
  • Further updates in design thread in chat: #mobile-design > edit message @ 💬

Original description:

We should probably offer this as an in-place text field within the message list. As Greg points out about this feature in zulip-mobile (link):

In general the edit-message UI in the mobile app is somewhat unloved; as you noted, the UX for it is borrowed from composing a message (and rather awkwardly so), and the code to implement editing a message is likewise rather jammed into the code for composing. One consequence is that it's easy to accidentally pick up behaviors that compose has even when they aren't intended.

@gnprice gnprice added the a-msglist The message-list screen, except what's label:a-content label May 26, 2023
@gnprice gnprice added this to the Beta milestone May 27, 2023
@gnprice gnprice removed the m-beta label May 27, 2023
@gnprice gnprice modified the milestones: Beta, Launch Sep 22, 2023
@gnprice
Copy link
Member

gnprice commented Mar 5, 2024

When we implement this, we should aim to offer editing on all message lists, including "All messages" (zulip/zulip-mobile#5619). If we successfully keep at least the code cleanly separated from that for composing a new message, as described above, then that should be straightforward.

(Potentially we'll decide that we want the UX for editing to remain similar to that for composing a message after all. In that case the code will probably have significant pieces in common. But the key will just be to have the differences between edit and compose cleanly factored out, rather than the edit logic being jammed in awkwardly as it is in zulip-mobile.)

@gnprice
Copy link
Member

gnprice commented Aug 23, 2024

This now has a design — thanks @terpimost! I've edited the issue description to show the design.

Chat thread on a couple of specific points in the design:
https://chat.zulip.org/#narrow/stream/48-mobile/topic/edit-message.20design/near/1930070

I think this is already ripe to work on, though. Both of those are things that will be easy to change later.

Those points have been resolved now; Vlad edited the Figma design and I edited the description above, and together those have the current design that's ready for implementation.

@chrisbobbe
Copy link
Collaborator Author

We've had a few requests for this in beta feedback: https://chat.zulip.org/#narrow/channel/48-mobile/topic/Flutter.20beta.3A.20cannot.20edit.20messages/near/2011951

@gnprice gnprice modified the milestones: M6: Post-launch, M5: Launch Dec 30, 2024
@lakshya1goel
Copy link
Contributor

Hello I want to work on this issue. Please assign this to me.
I have implemented the basic functionality, need to work more on UI part.
Here is the screen recording of the same:

WhatsApp.Video.2025-01-30.at.8.12.20.PM.mp4

lakshya1goel added a commit to lakshya1goel/zulip-flutter that referenced this issue Jan 30, 2025
lakshya1goel added a commit to lakshya1goel/zulip-flutter that referenced this issue Feb 3, 2025
lakshya1goel added a commit to lakshya1goel/zulip-flutter that referenced this issue Feb 3, 2025
@gnprice
Copy link
Member

gnprice commented Feb 6, 2025

@lakshya1goel Sounds good, thanks. As I mentioned just now at #1314 (comment), let's pause work on post-launch features like this one until after the launch is ready; but we can plan for you to pick up the work from there, if you'd like, when that time comes.

(And as a result we'll leave it "unassigned" in GitHub for now, just because I like to reserve the GitHub "assignment" feature on issues to track what work is actively happening rather than what work is planned for the future.)

@gnprice
Copy link
Member

gnprice commented Mar 17, 2025

I misread when writing my previous comment above: this is a launch issue, not post-launch. It's a feature we'd like to have in time for the app's launch, because the legacy app has it and we've heard from beta users that they miss it.

It's also a complex feature whose logic will intertwine deeply with the existing code for the compose box, and is likely to involve some refactors to that code. It'd be easy for an implementation of this, if not careful, to end up not only being hard to understand (and therefore prone to bugs, and expensive to make changes to) but also making the compose box in general hard to understand even when not trying to think about the editing case. That's the situation of the corresponding logic in zulip-mobile, which I've never been satisfied by.

For those reasons this is a feature that we'll want someone from the core team to be driving in order to end up with a successful implementation, and I've asked @chrisbobbe to take it on. @lakshya1goel thanks for trying this out; I'd encourage first revising your reviewed PRs, and then taking a look at our other launch issues. On the board I see at least one "help wanted" launch issue still unclaimed.

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Apr 16, 2025
With this API, we can use showSuggestedActionDialog in an "early
return" style -- await the user's choice, and early return if it was
"Cancel". That's particularly helpful when the confirmation dialog
belongs in an `if` block.

We'll use this for the upcoming "edit message" feature (zulip#126), where
we plan to offer a confirmation dialog before entering an
edit-message session, but only if the compose box has text for a new
message in it (which would be discarded if the user wants to go
ahead).
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Apr 17, 2025
With this API, we can use showSuggestedActionDialog in an "early
return" style -- await the user's choice, and early return if they
chose to dismiss the dialog. That's particularly helpful when the
confirmation dialog is opened in an `if` block.

We'll use this for the upcoming "edit message" feature (zulip#126), where
we plan to offer a confirmation dialog before entering an
edit-message session, but only if the compose box has text for a new
message in it (which would be discarded if the user wants to go
ahead).
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Apr 18, 2025
With this API, we can use showSuggestedActionDialog in an "early
return" style -- await the user's choice, and early return if they
chose to dismiss the dialog. That's particularly helpful when the
confirmation dialog is opened in an `if` block.

We'll use this for the upcoming "edit message" feature (zulip#126), where
we plan to offer a confirmation dialog before entering an
edit-message session, but only if the compose box has text for a new
message in it (which would be discarded if the user wants to go
ahead).
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Apr 24, 2025
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Apr 25, 2025
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Apr 25, 2025
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Apr 30, 2025
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue May 1, 2025
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue May 1, 2025
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue May 2, 2025
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue May 3, 2025
This needs tests for:
- compose box
- message list

Fixes: zulip#126
@chrisbobbe chrisbobbe linked a pull request May 3, 2025 that will close this issue
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue May 6, 2025
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue May 6, 2025
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue May 6, 2025
PIG208 pushed a commit to PIG208/zulip-flutter that referenced this issue May 7, 2025
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue May 10, 2025
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue May 10, 2025
PIG208 pushed a commit to PIG208/zulip-flutter that referenced this issue May 13, 2025
PIG208 pushed a commit to PIG208/zulip-flutter that referenced this issue May 14, 2025
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue May 15, 2025
PIG208 pushed a commit to PIG208/zulip-flutter that referenced this issue May 16, 2025
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue May 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-msglist The message-list screen, except what's label:a-content beta feedback Things beta users have specifically asked for
Projects
Status: No status
3 participants