action_sheet: Add "Mark as unread from here" button #779

Khader-1 · 2024-06-30T19:52:02Z

Fixes: #131
This adds a new button "Mark as unread from here" to the bottom sheet activated for read messages, when clicked all messages starting from selected one down are marked as unread.

Before	After

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-07-11.at.13.25.34.mp4

sm-sayedi

Thanks @Khader-1 for this. A few comments below.

Also, I would write the commit message as such:

action_sheet: Add "Mark as unread from here" button

Using `` is usually used for distinguishing code-related words from the rest of the commit message.

assets/l10n/app_en.arb

lib/widgets/action_sheet.dart

sm-sayedi

Thanks @Khader-1 for the revision. Looks good to me. Moving on to the mentor review from @kenclary!

kenclary

lgtm

alya · 2024-07-16T18:04:25Z

@Khader-1 , as usual, please add screenshots of all UI changes to the PR description, in addition to any videos.

Khader-1 · 2024-07-17T09:10:11Z

Thanks @alya, I've updated the description.
Additionally, rebased to the latest revision from #811.

alya · 2024-07-17T20:35:30Z

Thanks! Putting the new option just between "Quote and reply" and "Copy message text" would be most consistent with the web app UI, so let's do that.

When an issue doesn't specify where to put a new option, you can go ahead and flag it for discussion.

alya · 2024-07-17T20:36:21Z

We might need to figure out how to organize that menu soon -- it's getting long already, and there's more to come. :)

Khader-1 · 2024-07-18T08:41:03Z

Thanks @alya! I've just reordered the list.

We might need to figure out how to organize that menu soon -- it's getting long already, and there's more to come. :)

Maybe consider filing an issue to track this? I'm happy to work on any reordering or restructuring that may be needed.

Additionally, I've rebased back onto the main branch since #811 was merged.

Khader-1 · 2024-08-08T11:16:35Z

Thanks for the catch @PIG208! I've just pushed a new revision PTAL.
Marking this for integration review..

gnprice

Thanks @Khader-1, and thanks @sm-sayedi @alya @kenclary @PIG208 for the previous reviews!

I've read through the first few commits:
f2dec7f actions [nfc]: Port generic error handling to markNarrowAsRead
d4d2615 actions [nfc]: Port handleAllMessagesReadSuccess to markNarrowAsRead
45a5702 actions [nfc]: Move legacy-server check inside markNarrowAsRead

Generally they look good — thanks for completing those refactors. Going AFK for a bit, so here's the comments I have on those.

Later I'll return and review the remaining commits:
4793beb actions [nfc]: Factor out updateMessageFlagsStartingFromAnchor
7594536 actions_test [nfc]: Remove unnecessary whitespace
337451a actions_test: Create updateMessageFlagsStartingFromAnchor test group
680d491 actions_test [nfc]: Move generic cases out of markNarrowAsRead
4a86408 actions_test [nfc]: Use updateMessageFlagsStartingFromAnchor in it's cases
a5525e7 action_sheet: Add "Mark as unread from here" button

lib/widgets/message_list.dart

lib/widgets/actions.dart

gnprice

OK, finished reading the branch — comments on the remaining commits below.

I'll also want to make a pass over the commit structure and comments; but that'll be relatively quick once these items are addressed.

lib/widgets/actions.dart

test/widgets/actions_test.dart

assets/l10n/app_en.arb

lib/widgets/action_sheet.dart

lib/widgets/actions.dart

Khader-1 · 2024-08-09T11:36:01Z

Thanks @gnprice! I've pushed a new revision PTAL.

gnprice

Thanks for the revision! Just nits below, and I've also pushed a few added commits on the end:
463e270 actions [nfc]: Simplify includeAnchor logic slightly in update-flags loop
4f6283e actions [nfc]: Simplify name of anchor parameter to update-flags
fab52ef actions [nfc]: Revise doc comment for updateMessageFlagsStartingFromAnchor

After fixing those nits, this will be ready to merge.

gnprice · 2024-08-09T19:09:58Z

lib/widgets/actions.dart

@@ -16,101 +16,174 @@ import '../model/narrow.dart';
 import 'dialog.dart';
 import 'store.dart';

-Future<void> markNarrowAsRead(


nit: see previous commits for what summary-line prefixes are used on a given file, and match those

msg_list: Ensure markNarrowAsRead passes before acting on it

See https://github.com/zulip/zulip-mobile/blob/main/docs/howto/git.md for tips on reading history, including filtering to changes touching a file.

gnprice · 2024-08-09T19:31:17Z

test/widgets/action_sheet_test.dart

@@ -542,4 +544,63 @@ void main() {
      check(mockSharePlus.sharedString).isNull();
    });
  });
+
+  group('MarkAsUnread', () {


nit: tests in same order as code under test, to keep it easy to navigate each file from code in the other

test/widgets/actions_test.dart

gnprice · 2024-08-09T19:50:26Z

test/widgets/actions_test.dart

+      check(await didPass).isTrue();
+    });
+
+    testWidgets('pagination', (WidgetTester tester) async {


commit-structure nit:

2370dee actions_test [nfc]: Move generic cases out of markNarrowAsRead
69bd093 actions_test [nfc]: Use updateMessageFlagsStartingFromAnchor in it's cases

These are best squashed together — that way we don't have an intermediate state where there are tests that are described as being about updateMessageFlagsStartingFromAnchor but actually invoke markNarrowAsRead. Compare d439e65, where I moved tests from MarkAsReadWidget to markNarrowAsRead.

Similarly this previous commit can be squashed in:
c4fee15 actions_test [nfc]: Create updateMessageFlagsStartingFromAnchor test group

And then this previous commit may as well be squashed in too:
f37e338 actions_test [nfc]: Remove unnecessary whitespace

since those other changes are rearranging the list of test cases, and that one is a slight formatting fix to the same list.

bump on this part:

Similarly this previous commit can be squashed in:
c4fee15 actions_test [nfc]: Create updateMessageFlagsStartingFromAnchor test group

Khader-1 · 2024-08-12T10:50:45Z

Thanks @gnprice! I've pushed a new revision PTAL.

Khader-1 · 2024-08-12T10:51:20Z

Thanks @gnprice! I've pushed a new revision PTAL.

gnprice

Thanks @Khader-1 for the revision! Two comments below.

I think these will both be fastest for me to just fix up, so I'll do that and merge. But please do take a look through the material about handling Git merge/rebase conflicts and about git range-diff.

gnprice · 2024-08-13T02:56:14Z

test/widgets/action_sheet_test.dart

@@ -385,6 +387,65 @@ void main() {
    });
  });

+  group('MarkAsUnread', () {
+    testWidgets('not visible if message is not read', (WidgetTester tester) async {


Suggested change

testWidgets('not visible if message is not read', (WidgetTester tester) async {

testWidgets('not visible if message is not read', (tester) async {

That's a change that happened in main, so this looks like an error in merge-conflict resolution. I think the demo I did of those was one of the weeks you weren't able to join the call; but I recommend taking a look at these threads which have much the same information:
https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/rebase.20conflicts/near/1895505
https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/rebase.20conflicts/near/1136429
https://chat.zulip.org/#narrow/stream/2-general/topic/navigating.20code.20review.3A.20Rebasing.20work/near/1737371

See also this discussion of git range-diff:
https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/code.20reviews/near/1852971
That's in the context of reviewing someone else's PR (for comparing a new revision to a revision I previously reviewed), but I also regularly use it for self-review. In its output comparing your old revision to this revision, you'll see:

-- testWidgets('pagination', (WidgetTester tester) async { +- testWidgets('pagination', (tester) async { - // Check that `lastProcessedId` returned from an initial - // response is used as `anchorId` for the subsequent request. - final narrow = TopicNarrow.ofMessage(eg.streamMessage());

and later

+ testWidgets('pagination', (WidgetTester tester) async { + // Check that `lastProcessedId` returned from an initial + // response is used as `anchorId` for the subsequent request. -+ final narrow = TopicNarrow.ofMessage(eg.streamMessage()); + await prepare(tester);

Or it's a bit clearer with its red and green colors:

Note the change in the red side of the code from WidgetTester tester to just tester, and the absence of a corresponding change in the green side of the code. That's how I noticed this in this review.

gnprice · 2024-08-13T03:03:51Z

test/widgets/actions_test.dart

+      check(await didPass).isTrue();
+    });
+
+    testWidgets('pagination', (WidgetTester tester) async {


bump on this part:

Similarly this previous commit can be squashed in:
c4fee15 actions_test [nfc]: Create updateMessageFlagsStartingFromAnchor test group

Most of the logic in `markNarrowAsRead` is not specific to marking narrow as read, rather it is actually updating message flags for the narrow starting from the oldest anchor. As we are going to use this logic for several other actions starting with the next commit, it is better to have that generic logic factored out and reused.

…FlagsStartingFromAnchor

Fixes: zulip#131

…loop By updating the `includeAnchor` local immediately next to where we update the anchor itself, their relationship is made clear without a comment.

From the perspective of the callers, this anchor works just like the "anchor" parameters anywhere else, including the Zulip server API endpoints that take an anchor and narrow: the function operates on messages starting from there. The fact that it does so with potentially multiple server API calls, each with a different anchor, is an implementation detail. So we can just call the parameter "anchor".

…nchor

Khader-1 force-pushed the mark-as-unread branch from 800461c to 6af6ee0 Compare June 30, 2024 19:54

This comment was marked as outdated.

Sign in to view

Khader-1 force-pushed the mark-as-unread branch 2 times, most recently from f6e803a to fa7c150 Compare July 11, 2024 10:28

Khader-1 changed the title ~~(WIP) Mark as unread~~ action_sheet: Add Mark as unread from here button Jul 11, 2024

Khader-1 marked this pull request as ready for review July 11, 2024 10:32

Khader-1 added the buddy review GSoC buddy review needed. label Jul 11, 2024

Khader-1 requested a review from sm-sayedi July 11, 2024 10:33

sm-sayedi reviewed Jul 12, 2024

View reviewed changes

Khader-1 force-pushed the mark-as-unread branch from fa7c150 to 550011c Compare July 13, 2024 09:55

gnprice mentioned this pull request Jul 13, 2024

msglist: Provide narrow to MessageListPage descendants #811

Merged

Khader-1 force-pushed the mark-as-unread branch from 550011c to b5da0c8 Compare July 15, 2024 07:40

Khader-1 changed the title ~~action_sheet: Add Mark as unread from here button~~ action_sheet: Add "Mark as unread from here" button Jul 15, 2024

Khader-1 requested a review from sm-sayedi July 15, 2024 08:08

sm-sayedi approved these changes Jul 15, 2024

View reviewed changes

sm-sayedi added mentor review GSoC mentor review needed. and removed buddy review GSoC buddy review needed. labels Jul 15, 2024

sm-sayedi requested a review from kenclary July 15, 2024 09:43

kenclary approved these changes Jul 16, 2024

View reviewed changes

gnprice added maintainer review PR ready for review by Zulip maintainers and removed mentor review GSoC mentor review needed. labels Jul 16, 2024

gnprice requested a review from chrisbobbe July 16, 2024 17:52

Khader-1 force-pushed the mark-as-unread branch from b5da0c8 to 5215bd6 Compare July 17, 2024 09:07

Khader-1 force-pushed the mark-as-unread branch from 5215bd6 to 64f3451 Compare July 18, 2024 08:33

Khader-1 force-pushed the mark-as-unread branch from 64f3451 to 1abcafe Compare July 18, 2024 09:01

Khader-1 force-pushed the mark-as-unread branch from ce72213 to a5525e7 Compare August 8, 2024 10:09

Khader-1 added the integration review Added by maintainers when PR may be ready for integration label Aug 8, 2024

gnprice reviewed Aug 9, 2024

View reviewed changes

lib/widgets/message_list.dart Show resolved Hide resolved

lib/widgets/actions.dart Outdated Show resolved Hide resolved

gnprice reviewed Aug 9, 2024

View reviewed changes

Khader-1 force-pushed the mark-as-unread branch from a5525e7 to 3fc179a Compare August 9, 2024 11:35

Khader-1 requested a review from gnprice August 9, 2024 11:36

gnprice reviewed Aug 9, 2024

View reviewed changes

Khader-1 force-pushed the mark-as-unread branch from fab52ef to 1596022 Compare August 12, 2024 10:48

Khader-1 requested a review from gnprice August 12, 2024 10:49

Khader-1 force-pushed the mark-as-unread branch 2 times, most recently from cfdbddf to eafb710 Compare August 12, 2024 11:06

gnprice reviewed Aug 13, 2024

View reviewed changes

Khader-1 and others added 10 commits August 12, 2024 20:06

msglist: Ensure markNarrowAsRead passes before acting on it

7392ccb

actions [nfc]: Port generic error handling to markNarrowAsRead

2a734c7

actions [nfc]: Port handleAllMessagesReadSuccess to markNarrowAsRead

367e82e

actions [nfc]: Move legacy-server check inside markNarrowAsRead

28f5c2c

actions test [nfc]: Move most markNarrowAsRead tests to updateMessage…

5bb673f

…FlagsStartingFromAnchor

action_sheet: Add "Mark as unread from here" button

4788a63

Fixes: zulip#131

actions [nfc]: Simplify includeAnchor logic slightly in update-flags …

929783d

…loop By updating the `includeAnchor` local immediately next to where we update the anchor itself, their relationship is made clear without a comment.

actions [nfc]: Revise doc comment for updateMessageFlagsStartingFromA…

4e25bc8

…nchor

gnprice force-pushed the mark-as-unread branch from eafb710 to 4e25bc8 Compare August 13, 2024 03:10

gnprice merged commit 4e25bc8 into zulip:main Aug 13, 2024

gnprice mentioned this pull request Aug 13, 2024

Internal_link: Add ApiIsNarrow. #869

Merged

Khader-1 deleted the mark-as-unread branch August 13, 2024 09:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

action_sheet: Add "Mark as unread from here" button #779

action_sheet: Add "Mark as unread from here" button #779

Khader-1 commented Jun 30, 2024 •

edited

Loading

This comment was marked as outdated.

sm-sayedi left a comment

sm-sayedi left a comment

kenclary left a comment

alya commented Jul 16, 2024

Khader-1 commented Jul 17, 2024 •

edited

Loading

alya commented Jul 17, 2024

alya commented Jul 17, 2024

Khader-1 commented Jul 18, 2024

Khader-1 commented Aug 8, 2024

gnprice left a comment

gnprice left a comment

Khader-1 commented Aug 9, 2024

gnprice left a comment

gnprice Aug 9, 2024

gnprice Aug 9, 2024

gnprice Aug 9, 2024

gnprice Aug 13, 2024

Khader-1 commented Aug 12, 2024

Khader-1 commented Aug 12, 2024

gnprice left a comment

gnprice Aug 13, 2024

gnprice Aug 13, 2024

	testWidgets('not visible if message is not read', (WidgetTester tester) async {
	testWidgets('not visible if message is not read', (tester) async {

action_sheet: Add "Mark as unread from here" button #779

action_sheet: Add "Mark as unread from here" button #779

Conversation

Khader-1 commented Jun 30, 2024 • edited Loading

This comment was marked as outdated.

sm-sayedi left a comment

Choose a reason for hiding this comment

sm-sayedi left a comment

Choose a reason for hiding this comment

kenclary left a comment

Choose a reason for hiding this comment

alya commented Jul 16, 2024

Khader-1 commented Jul 17, 2024 • edited Loading

alya commented Jul 17, 2024

alya commented Jul 17, 2024

Khader-1 commented Jul 18, 2024

Khader-1 commented Aug 8, 2024

gnprice left a comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Khader-1 commented Aug 9, 2024

gnprice left a comment

Choose a reason for hiding this comment

gnprice Aug 9, 2024

Choose a reason for hiding this comment

gnprice Aug 9, 2024

Choose a reason for hiding this comment

gnprice Aug 9, 2024

Choose a reason for hiding this comment

gnprice Aug 13, 2024

Choose a reason for hiding this comment

Khader-1 commented Aug 12, 2024

Khader-1 commented Aug 12, 2024

gnprice left a comment

Choose a reason for hiding this comment

gnprice Aug 13, 2024

Choose a reason for hiding this comment

gnprice Aug 13, 2024

Choose a reason for hiding this comment

Khader-1 commented Jun 30, 2024 •

edited

Loading

Khader-1 commented Jul 17, 2024 •

edited

Loading