subscription_list: Show @-mention indicator when that applies #875

Khader-1 · 2024-08-09T19:11:02Z

Fixes: #747

unreads changes here are reflections of what web has at unread.ts

sm-sayedi

Thanks @Khader-1 for this. LGTM! Moving on to the mentor review! cc @kenclary

kenclary

lgtm

Khader-1 · 2024-08-14T09:48:13Z

Thanks @sm-sayedi and @kenclary for the reviews!
I figured that there are more cases that still need handling..
Specifically, the unread count / muted unread badge are related have more complex cases when there are mentions, I'll make more investigation on zulip/zulip and update this comment.

And currently, we ~~don't~~ have a record for muted mentions which ~~should be~~ is fixed in this PR by:
e5945bb msglist: Add message to mentions if mentioned.
So marking this as a draft for now until it is ready.

chrisbobbe · 2024-08-15T18:31:51Z

GitHub is showing there's a conflict in lib/widgets/theme.dart, could you resolve that please?

Khader-1 · 2024-08-15T19:01:47Z

Sure! just fixed PTAL @chrisbobbe.

chrisbobbe

Thanks! I've read through the first six commits:

69abbbc msglist: Add message to mentions if mentioned
200129e unreads [nfc]: Introduce _reverseStreamsLookup data structure
674c4ad unreads [nfc]: Enhance performance in _isPresentInStreams
6e9afd5 unreads [nfc]: Enhance performance in _removeAllInStreams
195c1de inbox [nfc]: Move AtMentionMarker to unread_count_badge
2e18afd inbox [nfc]: Introduce muted property to AtMentionMarker

with the last two left for next time:

0c52009 subscription_list: Show @-mention marker when that applies
64ac89e subscription_list: Show unread count when hasOnlyMutedMentions

because it looks like there's something to be straightened out in that sixth commit, as noted below. (All of AtMentionMarker's callers passing muted: true, which I think doesn't make sense intuitively.)

lib/model/message_list.dart

lib/model/unreads.dart

lib/widgets/inbox.dart

lib/widgets/unread_count_badge.dart

lib/widgets/theme.dart

chrisbobbe · 2024-08-16T00:01:21Z

lib/widgets/theme.dart

-      atMentionMarker: const HSLColor.fromAHSL(0.5, 0, 0, 0.2).toColor(),
+      atMentionMarker: const HSLColor.fromAHSL(0.7, 0, 0, 0.2).toColor(),
+      mutedAtMentionMarker: const HSLColor.fromAHSL(0.5, 0, 0, 0.2).toColor(),


Could you explain how you chose both of these colors? I see `atMentionMarker was changed from its previous value, but I don't understand why.

Well I've tried to copy the values from the web app.. here is my inspector values:
for unmuted mentions and mentions in inbox screen
hsl(0, 0, 0.2) or #333 with an opacity 0.7

And for muted mentions:

hsla(0, 0%, 20%, .5) with an opacity 0.7

Which could be simplified as: hsla(0, 0%, 20%, 0.35)

Please let me know if I am missing something.

It will probably be helpful to link (in the commit message) the lines containing the CSS styles in effect.

PIG208

Thanks for the revision! Left some comments.

When testing this manually, I observed that the @-mention marker shows up for all channels with unreads in the subscription list, even if those channels do not contain unread mentions. So that's something we can cover in tests as well.

PIG208 · 2024-09-13T19:17:10Z

lib/model/unreads.dart

@@ -236,6 +234,19 @@ class Unreads extends ChangeNotifier {
    notifyListeners();
  }

+  void onMessagesFetched(List<Message> messages) {


nit: onSomething typically names a parameter that takes a callback function. reconcileMessages might be a better name.

nit: Logically, the new method is different from the group of handleFooEvents methods, so perhaps we should move it before handleMessageEvent. This way the implementation matches the current order in tests too.

lib/model/unreads.dart

test/model/unreads_test.dart

lib/model/unreads.dart

PIG208 · 2024-09-13T20:23:33Z

lib/widgets/subscription_list.dart

@@ -230,7 +235,8 @@ class SubscriptionItem extends StatelessWidget {

    final swatch = colorSwatchFor(context, subscription);
    final hasUnreads = (unreadCount > 0);
-    final opacity = subscription.isMuted ? 0.55 : 1.0;
+    const mutedOpacity = 0.55;


It would be helpful to link the source of the opacity.

I am not sure if I got it correctly; I just used the same one that we've used before for the channel name and unread count badge.

Are you suggesting to link it to zulip mobile or web design?

Oh I see. Yeah 0.55 is not new here; it was added in b0b8a50. The design came from Vlad here. Perhaps we can link to that GitHub comment in an NFC commit.

test/widgets/subscription_list_test.dart

Fixes: zulip#649

This will be helpful for efficiently checking for @-mention in subscription_list.

AtMentionMarker is going to be used in subscription_list as well as inbox, so it would be better to reuse it by having it declared as a public class.

Fixes: zulip#747

gnprice · 2024-10-11T22:19:20Z

lib/widgets/subscription_list.dart

+        final hasOnlyMutedMentions = !subscription.isMuted && hasMentions
+          && !channelsWithUnmutedMentions.contains(subscription.streamId);
+        final mutedUnreadCount = hasOnlyMutedMentions && unreadCount == 0 ?
+          unreadsModel!.countAll(subscription.streamId) : 0;


It feels like there's a lot of different conditions that are interacting here, which means there are a lot of different cases for how this can end up behaving. I can't easily tell from reading this code what all those cases are and what its behavior is in all of them, which makes it hard to tell if the behavior is what we want.

Can you try laying out (just in text, in this thread) what you believe the different cases are for a given channel, and how you believe this screen should behave in each case? Then we can discuss at that level what behavior we want, and then go back to the code and make sure the code reflects that behavior.

(Ultimately I suspect we can express the desired behavior with somewhat simpler logic than this.)

I agree with Greg that laying this out will be helpful.

Adding the @-markers to the SubscriptionItem, in addition to the unreads seem to complicate the widget more. Perhaps we should be cautious about adding more flags to that widget and simplify these conditions.

PIG208 · 2024-10-29T18:36:35Z

lib/widgets/subscription_list.dart

+              child: UnreadCountBadge(
+                count: mutedUnreadCount,
+                backgroundColor: swatch,
+                bold: true)),


Looks like the later part is mostly identical to the previous branch except for the opacity/unread count. I think it would be beneficial to find a way that do not couple AtMentionMarker with UnreadCountBadge, so that each has individual logic, making the change easier to verify.

For example, we can have something like (not sure if the conditions are right):

if (hasMentions) ...[ AtMentionMarker(muted: hasOnlyMutedMentions || subscription.isMuted) ]

that is adjancent to but independent from the UnreadCountBadge branches

Khader-1 force-pushed the subscription-list-mention branch 2 times, most recently from 838e874 to 2656d84 Compare August 9, 2024 20:30

Khader-1 marked this pull request as ready for review August 9, 2024 20:31

Khader-1 requested a review from sm-sayedi August 9, 2024 20:31

Khader-1 added the buddy review GSoC buddy review needed. label Aug 9, 2024

sm-sayedi reviewed Aug 10, 2024

View reviewed changes

sm-sayedi added mentor review GSoC mentor review needed. and removed buddy review GSoC buddy review needed. labels Aug 10, 2024

Khader-1 force-pushed the subscription-list-mention branch 2 times, most recently from 99d2413 to e159b22 Compare August 10, 2024 20:23

Khader-1 requested a review from kenclary August 10, 2024 20:24

kenclary approved these changes Aug 10, 2024

View reviewed changes

Khader-1 added maintainer review PR ready for review by Zulip maintainers and removed mentor review GSoC mentor review needed. labels Aug 11, 2024

Khader-1 force-pushed the subscription-list-mention branch 2 times, most recently from 39188fd to d7a742d Compare August 13, 2024 09:26

Khader-1 force-pushed the subscription-list-mention branch 2 times, most recently from 9874510 to e4b4101 Compare August 14, 2024 17:00

Khader-1 force-pushed the subscription-list-mention branch from e4b4101 to 64ac89e Compare August 15, 2024 19:01

chrisbobbe reviewed Aug 16, 2024

View reviewed changes

Khader-1 force-pushed the subscription-list-mention branch 4 times, most recently from 2d1e718 to 74a010b Compare September 11, 2024 08:22

Khader-1 requested a review from chrisbobbe September 11, 2024 08:29

gnprice requested review from PIG208 and removed request for chrisbobbe September 12, 2024 18:49

gnprice assigned PIG208 Sep 12, 2024

PIG208 reviewed Sep 13, 2024

View reviewed changes

Khader-1 added 6 commits October 11, 2024 17:03

msglist: Add message to mentions if mentioned

4362559

Fixes: zulip#649

unreads [nfc]: Introduce _streamIdsAndTopicsByMessageId data structure

b3b9f53

This will be helpful for efficiently checking for @-mention in subscription_list.

inbox [nfc]: Move AtMentionMarker to unread_count_badge

2a00ea1

AtMentionMarker is going to be used in subscription_list as well as inbox, so it would be better to reuse it by having it declared as a public class.

inbox [nfc]: Introduce muted property to AtMentionMarker

ef6d4c3

subscription_list: Show @-mention marker when that applies

b1e8fb7

Fixes: zulip#747

subscription_list: Show unread count when hasOnlyMutedMentions

a635d4f

Khader-1 force-pushed the subscription-list-mention branch from 74a010b to a635d4f Compare October 11, 2024 15:00

gnprice reviewed Oct 11, 2024

View reviewed changes

PIG208 self-requested a review October 28, 2024 23:29

PIG208 reviewed Oct 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

subscription_list: Show @-mention indicator when that applies #875

subscription_list: Show @-mention indicator when that applies #875

Khader-1 commented Aug 9, 2024 •

edited

Loading

sm-sayedi left a comment

kenclary left a comment

Khader-1 commented Aug 14, 2024 •

edited

Loading

chrisbobbe commented Aug 15, 2024

Khader-1 commented Aug 15, 2024

chrisbobbe left a comment

chrisbobbe Aug 16, 2024

Khader-1 Sep 11, 2024

PIG208 Sep 12, 2024

PIG208 left a comment

PIG208 Sep 13, 2024

PIG208 Sep 13, 2024

PIG208 Sep 13, 2024

Khader-1 Oct 11, 2024

PIG208 Oct 11, 2024

gnprice Oct 11, 2024

PIG208 Oct 29, 2024

PIG208 Oct 29, 2024

PIG208 Oct 29, 2024

subscription_list: Show @-mention indicator when that applies #875

Are you sure you want to change the base?

subscription_list: Show @-mention indicator when that applies #875

Conversation

Khader-1 commented Aug 9, 2024 • edited Loading

sm-sayedi left a comment

Choose a reason for hiding this comment

kenclary left a comment

Choose a reason for hiding this comment

Khader-1 commented Aug 14, 2024 • edited Loading

chrisbobbe commented Aug 15, 2024

Khader-1 commented Aug 15, 2024

chrisbobbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Khader-1 commented Aug 9, 2024 •

edited

Loading

Khader-1 commented Aug 14, 2024 •

edited

Loading