subscription_list: Show muted streams as faded in streams list #711

Khader-1 · 2024-05-30T01:21:10Z

Fixes: #424

After	Before

sm-sayedi

Thanks @Khader-1. LGTM! Just a few small comments below.

test/widgets/subscription_list_test.dart

Khader-1 · 2024-05-31T09:11:09Z

Thanks @sm-sayedi! I've pushed a revision PTAL.

sm-sayedi

Thanks for the revision, @Khader-1. Looks great. ~~You can now move on the the mentor review.~~

Upon reviewing #714, I feel the need for another small change in the code. See below.

sm-sayedi · 2024-05-31T14:06:27Z

lib/widgets/subscription_list.dart

-          ],
-          const SizedBox(width: 16),
-        ])));
+        child: Opacity(


Instead of Opacity wrapping the whole Row, let's make it wrap only the icon and the stream name. This will be helpful when there is a dot marker added through #714. See 714 comment.

Good catch, Thanks!

Khader-1

Thanks for the review @sm-sayedi! A new revision is pushed PTAL.

test/widgets/subscription_list_test.dart

Khader-1 · 2024-05-31T15:57:29Z

lib/widgets/subscription_list.dart

-          ],
-          const SizedBox(width: 16),
-        ])));
+        child: Opacity(


Good catch, Thanks!

sm-sayedi

Thanks for the revision @Khader-1! LGTM. Let's move on to the mentor review from @kenclary!

kenclary

looks good to me.

alya · 2024-06-03T20:38:23Z

I think we should show muted channels below unmuted ones, as we do in the left sidebar in the web app, unless we specifically have reason to do otherwise. (I know that's not how it works in the RN app.)

alya · 2024-06-03T20:39:28Z

Could you please update the screenshots in the PR description to include more muted streams, including ones both with and without unread messages?

Khader-1 · 2024-06-10T06:44:20Z

Thanks @alya! I agree. I've updated the sorting logic and the screenshot.
When it comes to notifications, the current behavior is to show nothing if the stream is muted, but this will be replaced with #714

Khader-1

Thanks for the review @chrisbobbe! I believe this ready now.

chrisbobbe

Thanks! And thanks for posting updated screenshots. I notice that the unread marker isn't faded, like it is in Vlad's draft design in the issue: #424 (comment)

Let's make it so the unread marker is also faded.

test/example_data.dart

test/widgets/subscription_list_test.dart

chrisbobbe · 2024-07-05T19:47:22Z

I see you've pushed some changes; please comment here when this is ready for another review.

Khader-1 · 2024-07-09T15:10:36Z

This is now ready; I had to update the Flutter SDK, which unfortunately took quite some time.

chrisbobbe

Thanks @Khader-1! Small comments below, and I'll go ahead and mark for Greg's review.

chrisbobbe · 2024-07-09T19:34:06Z

lib/widgets/subscription_list.dart

-            UnreadCountBadge(count: unreadCount, backgroundColor: swatch, bold: true),
+            Opacity(
+              opacity: subscription.isMuted ? 0.55 : 1,
+              child: UnreadCountBadge(
+                count: unreadCount,
+                backgroundColor: swatch,
+                bold: true)),


Can we simplify by putting a single Opacity higher in the tree, instead of two different Opacitys?

It could be done as such and it was actually but we had to change it as it will eventually be changed when working on #714; Check Sayed's comment

test/widgets/subscription_list_test.dart

gnprice

Thanks @Khader-1, and thanks @sm-sayedi and @chrisbobbe for the previous reviews! Comments below.

test/example_data.dart

test/widgets/message_list_test.dart

lib/widgets/subscription_list.dart

gnprice · 2024-07-10T07:26:41Z

lib/widgets/subscription_list.dart

+            Opacity(
+              opacity: subscription.isMuted ? 0.55 : 1,


Can this have just one Opacity around the whole thing? Or is there something I'm missing that isn't getting the Opacity applied?

If so, then that may also enable going back to a single Row instead of a pair of them nested.

It could be done as such and it was actually but we had to change it as it will eventually be changed when working on #714; Check #711 (comment)

I see.

In that case let's split the Opacity up further, so that it applies separately to the icon and name. That will save the extra layer of Expanded and Row, which I think makes this code a bit easier to read and understand.

lib/widgets/subscription_list.dart

test/widgets/subscription_list_test.dart

gnprice · 2024-07-10T07:43:33Z

test/widgets/subscription_list_test.dart

+        UserTopicItem(
+          streamId: stream1.streamId,
+          topicName: 'a',
+          lastUpdated: 1234567890,
+          visibilityPolicy: UserTopicVisibilityPolicy.unmuted,


This can be made a bit more compact (and with less noise from boring parts like lastUpdated) with an appropriate helper.

There's one already in model/stream_test.dart. So that can be moved to example_data.dart in a prep commit.

bump — you added that useful prep commit but then didn't actually use the new shared helper here 🙂

test/widgets/subscription_list_test.dart

gnprice

Thanks for the revision! This is close to merge now. Small comments below.

gnprice · 2024-07-12T23:58:39Z

test/example_data.dart

+int _nextStreamId() => (_lastStreamId += 1 + Random().nextInt(10));
+int _lastStreamId = 100;
+
+const staticStreamId = 123;


Suggested change

const staticStreamId = 123;

const defaultStreamId = 123;

I think this name will make it a bit clearer how this stream ID relates to the rest of the data.

Or I guess that name doesn't capture it either — the meaning is actually a little complicated, in that this is the default for eg.streamMessage but not for eg.stream (which defaults to the random sequence).

Let's make it:

Suggested change

const staticStreamId = 123;

const defaultStreamMessageStreamId = 123;

and also put the definition right above streamMessage.

That name is kind of wordy, but that may actually be a good thing here — the way this works is a bit surprising and we should probably reorganize so that tests aren't referring to a shared constant stream ID like this.

The streamMessage doc should also mention that there's this default.

bump on these two aspects:

and also put the definition right above streamMessage.

The streamMessage doc should also mention that there's this default.

test/example_data.dart

gnprice · 2024-07-13T00:05:14Z

test/widgets/subscription_list_test.dart

+        eg.subscription(stream2, isMuted: false)
+      ],


nit:

Suggested change

eg.subscription(stream2, isMuted: false)

],

eg.subscription(stream2, isMuted: false),

],

gnprice · 2024-07-13T00:13:36Z

test/widgets/subscription_list_test.dart

+        UserTopicItem(
+          streamId: stream1.streamId,
+          topicName: 'a',
+          lastUpdated: 1234567890,
+          visibilityPolicy: UserTopicVisibilityPolicy.unmuted,


bump — you added that useful prep commit but then didn't actually use the new shared helper here 🙂

gnprice · 2024-07-13T00:23:55Z

lib/widgets/subscription_list.dart

+            Opacity(
+              opacity: subscription.isMuted ? 0.55 : 1,


I see.

In that case let's split the Opacity up further, so that it applies separately to the icon and name. That will save the extra layer of Expanded and Row, which I think makes this code a bit easier to read and understand.

Khader-1 · 2024-07-13T10:56:55Z

Thanks Greg! Sorry about the lack of focus.. I believe This is ready now.

gnprice · 2024-07-14T22:22:33Z

test/widgets/subscription_list_test.dart

+        eg.subscription(stream2, isMuted: false)
+      ],


test/widgets/subscription_list_test.dart

gnprice · 2024-07-14T22:25:44Z

lib/widgets/subscription_list.dart

+                iconDataForStream(subscription)),
+            ),
+          ),


nit: follow the existing style for formatting these close-parens

Suggested change

iconDataForStream(subscription)),

),

),

iconDataForStream(subscription)))),

gnprice · 2024-07-14T22:25:58Z

lib/widgets/subscription_list.dart

+                  subscription.name),
+              ))),


nit: similarly here

gnprice · 2024-07-14T22:26:51Z

test/example_data.dart

+int _nextStreamId() => (_lastStreamId += 1 + Random().nextInt(10));
+int _lastStreamId = 100;
+
+const staticStreamId = 123;


bump on these two aspects:

and also put the definition right above streamMessage.

The streamMessage doc should also mention that there's this default.

gnprice · 2024-07-14T22:27:59Z

Thanks @Khader-1 for the revision! Just a couple of small things still open, and a few nits new in this revision; details above.

Khader-1 · 2024-07-15T08:07:42Z

Thanks @gnprice for the review! I've made a new revision PTAL.

This allows `eg.stream` to be used in contexts where it's important that the stream IDs don't collide or even that they're increasing, without the test having to explicitly give specific stream IDs when no other fact about them is relevant. Some tests are dependent on the stream IDs being static. Those are updated to use `eg.defaultStreamMessageStreamId` explicitly.

Fixes: zulip#424

gnprice

Thanks @Khader-1 for all your work on this! Looks good — merging.

I fixed the one nit below, and also tweaked the eg.streamMessage doc slightly:

-/// If the message stream `stream` is not given, an example stream with
-/// `defaultStreamMessageStreamId` will be used.
+/// The message will be in `stream` if given.  Otherwise,
+/// an example stream with ID `defaultStreamMessageStreamId` will be used.

test/example_data.dart

Khader-1 added the buddy review GSoC buddy review needed. label May 30, 2024

Khader-1 requested a review from sm-sayedi May 30, 2024 01:21

Khader-1 force-pushed the ui-show-muted-streams-as-faded-in-streams-list branch from f25e276 to f2b71c2 Compare May 30, 2024 06:57

sm-sayedi reviewed May 30, 2024

View reviewed changes

Khader-1 added mentor review GSoC mentor review needed. buddy review GSoC buddy review needed. and removed buddy review GSoC buddy review needed. mentor review GSoC mentor review needed. labels May 31, 2024

Khader-1 force-pushed the ui-show-muted-streams-as-faded-in-streams-list branch from f2b71c2 to 72b7dae Compare May 31, 2024 09:06

sm-sayedi approved these changes May 31, 2024

View reviewed changes

sm-sayedi added buddy review GSoC buddy review needed. and removed buddy review GSoC buddy review needed. labels May 31, 2024

sm-sayedi reviewed May 31, 2024

View reviewed changes

sm-sayedi mentioned this pull request May 31, 2024

subscription_list: Show a dot for unreads if channel is muted #714

Merged

Khader-1 force-pushed the ui-show-muted-streams-as-faded-in-streams-list branch from 72b7dae to 0620510 Compare June 1, 2024 05:05

Khader-1 commented Jun 1, 2024

View reviewed changes

Khader-1 force-pushed the ui-show-muted-streams-as-faded-in-streams-list branch from 0620510 to cf07bf8 Compare June 1, 2024 06:07

sm-sayedi approved these changes Jun 1, 2024

View reviewed changes

sm-sayedi added mentor review GSoC mentor review needed. and removed buddy review GSoC buddy review needed. labels Jun 1, 2024

sm-sayedi requested a review from kenclary June 1, 2024 06:55

kenclary approved these changes Jun 1, 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 Jun 2, 2024

Khader-1 force-pushed the ui-show-muted-streams-as-faded-in-streams-list branch from cf07bf8 to 97e1bbb Compare June 10, 2024 08:17

gnprice requested a review from chrisbobbe June 10, 2024 17:22

Khader-1 force-pushed the ui-show-muted-streams-as-faded-in-streams-list branch from 226c137 to 1e66fdf Compare June 29, 2024 12:42

Khader-1 commented Jun 29, 2024

View reviewed changes

Khader-1 requested a review from chrisbobbe June 29, 2024 12:52

chrisbobbe reviewed Jul 3, 2024

View reviewed changes

test/example_data.dart Outdated Show resolved Hide resolved

test/widgets/subscription_list_test.dart Outdated Show resolved Hide resolved

Khader-1 force-pushed the ui-show-muted-streams-as-faded-in-streams-list branch from 1e66fdf to 24d9bc1 Compare July 3, 2024 09:10

Khader-1 requested a review from chrisbobbe July 9, 2024 15:04

chrisbobbe reviewed Jul 9, 2024

View reviewed changes

chrisbobbe added the integration review Added by maintainers when PR may be ready for integration label Jul 9, 2024

gnprice reviewed Jul 10, 2024

View reviewed changes

Khader-1 force-pushed the ui-show-muted-streams-as-faded-in-streams-list branch from 24d9bc1 to 6bf222f Compare July 10, 2024 09:45

Khader-1 requested review from gnprice and chrisbobbe July 10, 2024 09:49

chrisbobbe removed the maintainer review PR ready for review by Zulip maintainers label Jul 12, 2024

gnprice reviewed Jul 13, 2024

View reviewed changes

Khader-1 force-pushed the ui-show-muted-streams-as-faded-in-streams-list branch from 6bf222f to 32e3bf6 Compare July 13, 2024 10:53

gnprice reviewed Jul 14, 2024

View reviewed changes

Khader-1 force-pushed the ui-show-muted-streams-as-faded-in-streams-list branch from 32e3bf6 to 19b53b0 Compare July 15, 2024 08:05

Khader-1 requested a review from gnprice July 15, 2024 08:06

Khader-1 added 4 commits July 15, 2024 12:43

subscription_list: Show muted streams below unmuted ones

b517593

test [nfc]: Factor out userTopicItem helper

a9ad78e

subscription_list: Show muted streams as faded in streams list

b0b8a50

Fixes: zulip#424

gnprice reviewed Jul 15, 2024

View reviewed changes

test/example_data.dart Outdated Show resolved Hide resolved

gnprice force-pushed the ui-show-muted-streams-as-faded-in-streams-list branch from 19b53b0 to b0b8a50 Compare July 15, 2024 19:45

gnprice merged commit b0b8a50 into zulip:main Jul 15, 2024

	const staticStreamId = 123;
	const defaultStreamMessageStreamId = 123;

subscription_list: Show muted streams as faded in streams list #711

subscription_list: Show muted streams as faded in streams list #711

Conversation

Khader-1 commented May 30, 2024 • edited Loading

sm-sayedi left a comment

Choose a reason for hiding this comment

Khader-1 commented May 31, 2024

sm-sayedi left a comment • edited Loading

Choose a reason for hiding this comment

sm-sayedi May 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Khader-1 left a comment

Choose a reason for hiding this 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 Jun 3, 2024

alya commented Jun 3, 2024

Khader-1 commented Jun 10, 2024

Khader-1 left a comment

Choose a reason for hiding this comment

chrisbobbe left a comment

Choose a reason for hiding this comment

chrisbobbe commented Jul 5, 2024

Khader-1 commented Jul 9, 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

gnprice 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

gnprice 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

Khader-1 commented Jul 13, 2024

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

gnprice commented Jul 14, 2024

Khader-1 commented Jul 15, 2024

gnprice left a comment

Choose a reason for hiding this comment

Khader-1 commented May 30, 2024 •

edited

Loading

sm-sayedi left a comment •

edited

Loading

sm-sayedi May 31, 2024 •

edited

Loading