msglist: Support deleting a message completely #1861
Merged
Conversation
This file contains hidden or 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
|
rajveermalviya
approved these changes
Sep 22, 2025
There was a problem hiding this comment.
Thanks @chrisbobbe! All LGTM, and tests great. One nit below.
test/widgets/dialog_checks.dart
Outdated
| /// Checks for a suggested-action dialog matching an expected title and message. | ||
| /// Fails if none is found. | ||
| /// | ||
| /// If testing on iOS, uses [expectDestructiveActionButton] to check whether |
There was a problem hiding this comment.
nit:
Suggested change
| /// If testing on iOS, uses [expectDestructiveActionButton] to check whether | |
| /// If testing on iOS, use [expectDestructiveActionButton] to check whether |
?
rajveermalviya
added
integration review
gnprice
reviewed
Sep 22, 2025
There was a problem hiding this comment.
Thanks @chrisbobbe, and thanks @rajveermalviya for the previous review!
Just a few nits below. Let's also have a couple of quick tests for this:
- one test case showing it does the thing;
- one showing that if you dismiss the confirmation dialog, nothing happens (no request to the server).
Comment on lines
+164
to
+166
| contextMenuItemBgDanger: const Color(0xffc0070a), // TODO(#831) red/550 | ||
| contextMenuItemIcon: const Color(0xff4f42c9), | ||
| contextMenuItemIconDanger: const Color(0xffac0508), // TODO(#831) red/600 |
There was a problem hiding this comment.
Ah neat — I like this way of concisely recording what color from the palette these are linked to.
Comment on lines
378
to
+382
| return ZulipMenuItemButton( | ||
| style: destructive | ||
| ? ZulipMenuItemButtonStyle.menuDestructive | ||
| : ZulipMenuItemButtonStyle.menu, |
There was a problem hiding this comment.
nit: maybe make this param required now?
lib/widgets/action_sheet.dart
Outdated
Comment on lines
1616
to
1617
| class DeleteButton extends MessageActionSheetMenuItemButton { | ||
| DeleteButton({super.key, required super.message, required super.pageContext}); |
There was a problem hiding this comment.
nit: make private?
Or if not, then give a more explicit name: e.g. DeleteMessageButton.
gnprice
reviewed
Sep 22, 2025
lib/api/route/messages.dart
Outdated
Comment on lines
254
to
258
| Future<void> deleteMessage( | ||
| ApiConnection connection, { | ||
| required int messageId, | ||
| }) { | ||
| return connection.delete('updateMessage', (_) {}, 'messages/$messageId', {}); |
There was a problem hiding this comment.
Looks like a copy-paste-o here 🙂
chrisbobbe
force-pushed
the
pr-delete-message-button
branch
from
91e1875 to
a22be95
Compare
|
Thanks! Revision pushed. |
|
Thanks! Would you add a test case like this one too?
Otherwise all LGTM. |
chrisbobbe
force-pushed
the
pr-delete-message-button
branch
from
a22be95 to
e812d51
Compare
|
Yep! Done; revision pushed (forgot to comment yesterday saying that). |
…fault But only when we're simulating recent servers, as the comment says. (FL 291 removed realmDeleteOwnMessagePolicy and added realmCanDeleteOwnMessageGroup, so to be representative, tests should always specify one but not the other.)
Fixes zulip#1032. See Apple's HIG document: https://developer.apple.com/design/human-interface-guidelines/alerts And note that Android doesn't vary the button styles in these ways; see the description of zulip#1032.
Fixes zulip#1548. For the confirmation dialog, we considered following web by including a button linking to /help/delete-a-message#delete-a-message-completely but decided not to because there wasn't a natural way to make it subtle/low-attention.
|
Thanks! Looks good; merging. |
gnprice
force-pushed
the
pr-delete-message-button
branch
from
e812d51 to
f89ecb2
Compare
12 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Fixes #1548.
Fixes #1032.
Screenshots coming soon!