Displays unread message counts of muted streams and topics. #802

manojkestur · 2020-09-27T17:34:08Z

This PR displays unread message count of muted streams and topics.
Names of muted streams and topics are displayed in a different color instead of 'M'.

Fixes #209

manojkestur · 2020-10-06T13:24:52Z

@neiljp Can you review this PR

neiljp

@manojkestur Hey! Thanks for taking an interest in the project!

You're addressing part of quite an old issue, so the scope of what to do is perhaps less clear than it was, making this more challenging than it might be!

Your git commit text matches our current style 👍 However, it would be useful to separate each change to the muted stream/topic text and counts (UI) into separate commits, and definitely keep that work separate from any change to the logic. That will aid in assessing which of these aspects to go forward with, and understanding more clearly what changes you're making in each case.

I've tried the PR:

This is how I noticed the varying unread counts values; having it in a separate commit it might be clearer what is intended there
Indicating the muted elements with the text styling is definitely an improvement 👍 Though, I think it might need tuning in some themes since we likely want to have the text faded slightly, but not as unreadable as it seems in eg. the light theme?
I'm less clear on just showing the counts of muted streams, as it looks like they are contributing towards the unread totals being in the same style. I know this was suggested in the original issue, so we should discuss that - we could style the muted counts differently, or maybe use a different character than M? I'm not personally completely convinced it's important to show a count when it's muted though.

I'm going to start a conversation at chat.zulip.org in #zulip-terminal regarding this - you're welcome to join in and comment :)

neiljp · 2020-10-06T17:04:23Z

zulipterminal/config/themes.py

+        ('muted_stream', 'light gray',          'black',
+         None,            DEF['light_gray'],          DEF['black']),


Please align these so they look like the surrounding layout - similarly in the other locations you added.

neiljp · 2020-10-06T17:07:33Z

zulipterminal/helper.py

        stream_topic = (stream_id, stream['topic'])
        unread_counts['unread_topics'][stream_topic] = count
        if not unread_counts['streams'].get(stream_id):
            unread_counts['streams'][stream_id] = count
        else:
            unread_counts['streams'][stream_id] += count
-        if stream_id not in model.muted_streams:
+        if stream_id not in model.muted_streams and not topic_muted:


This and the other associated changes (in this file and elsewhere) appear to be a separate bugfix - is that correct? If so, it would be good to place these changes in a separate commit.

If this is intended as a bugfix, I'm a little concerned this correct, since the unread counts appear very different to running the current git version.

neiljp · 2020-10-06T21:49:20Z

zulipterminal/config/themes.py

@@ -242,6 +245,8 @@
         None,             YELLOW,            BLACK),
        ('edit_time',      'light blue',      'black',
         None,             LIGHTBLUE,         BLACK),
+        ('muted_stream',   'yellow',   'black',


You're applying this to both streams and topics, and specifically only the text names, so it'd be good to use a more general name - perhaps muted_name?

manojkestur · 2020-10-07T02:42:54Z

@neiljp Thanks for the review. I will make the suggested changes.

manojkestur · 2020-10-07T06:16:20Z

I have to do 2 separate commits for dimming muted streams and displaying counts right?

neiljp · 2020-10-07T22:57:03Z

@manojkestur That would be preferable, as we can always then pick just one commit to apply easily, separate from the other. If commits are earlier in a branch then they are less likely to have dependencies upon your other commits and easier to 'cherry pick' separately.

neiljp · 2020-10-27T00:14:31Z

@manojkestur I'm not sure if you're ready for a review of this yet, but from a quick scan I can see you have a few unrelated changes in commits, and you replaced the M in one commit by an empty string only to replace that in the next commit (it'd be cleaner to leave it as 'M' and change it in the second).

Re the mute symbol, it looks a little strange here, particularly with a smaller font, though I've not found a better unicode symbol. Did you look at many? Are you available to discuss on chat.zulip.org some time?

manojkestur · 2020-10-27T02:54:56Z

@neiljp I thought only dimming of muted streams and topic without M was needed. Now i will change that to M.
I saw same unicode symbols with different styling but when i pasted it, it's taking basic unicode symbol only.
Yes, i am available to discuss on chat.zulip.org.

manojkestur · 2020-10-28T13:38:40Z

@neiljp I have done the suggested changes.Let me know if any changes required.

manojkestur · 2020-10-28T13:52:57Z

manojkestur · 2020-11-05T17:28:43Z

manojkestur · 2020-11-05T17:31:47Z

@neiljp I have resolved the conflicts and have done the required changes.

neiljp · 2020-11-06T01:24:33Z

@manojkestur Thanks for rebasing and updating the last commit 👍

I do like the possibility of this remaining change, though as I indicated when we discussed it in the stream, I also like the idea of not being distracted by whatever the counts are in those muted streams. Having the option of being like the webapp does appear useful in any case, though I've started a discussion in #general to better understand the motivation in the webapp.

This commit does seem to render OK at first glance, but doesn't respond to muting events properly? I also don't understand at first reading why you are adjusting the helper code. The internal data should be being updated properly, but please let us know if you've found a bug in the logic there - though we should keep that different from changing how the UI is styled.

The key methods - at least for streams - are mark_muted and mark_unmuted. Can we just update those to use the update_coiunt method, perhaps with an optional parameter for the muted style, the same as we have for the text color?

Fixes zulip#209

manojkestur · 2020-11-06T17:37:48Z

@neiljp I have checked the muted events multiple times it is working fine.
When topic is muted and stream is not muted, unread messages were not added to all messages, but when we mute the stream where muted topic resides, all messages count was reducing. To fix this bug I have done little changes in model.py and helper.py.
Unread count was not updated when stream was muted so I changed that in helper.py.
I have kept unread_count_color as parameter in update_count

zulipbot · 2021-01-30T20:34:19Z

Heads up @manojkestur, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

zulipbot added size: M [Automatic label added by zulipbot] bug Something isn't working enhancement New feature or request labels Sep 27, 2020

neiljp reviewed Oct 6, 2020

View reviewed changes

neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed bug Something isn't working labels Oct 6, 2020

manojkestur force-pushed the issue-209 branch from 3bddcf9 to c34a72d Compare October 22, 2020 11:03

neiljp mentioned this pull request Oct 22, 2020

Display '(you)' beside logged-in-user in user list. #798

Closed

manojkestur force-pushed the issue-209 branch 8 times, most recently from 558b460 to 5692847 Compare October 24, 2020 03:10

zulipbot added size: L [Automatic label added by zulipbot] and removed size: M [Automatic label added by zulipbot] labels Oct 27, 2020

manojkestur force-pushed the issue-209 branch from 78475a4 to 83bf54b Compare October 28, 2020 04:44

zulipbot added size: M [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Oct 28, 2020

manojkestur force-pushed the issue-209 branch 3 times, most recently from 9ed95ef to 78f9e5d Compare October 28, 2020 11:01

zulipbot added size: M [Automatic label added by zulipbot] size: L [Automatic label added by zulipbot] and removed size: S [Automatic label added by zulipbot] size: M [Automatic label added by zulipbot] labels Oct 28, 2020

manojkestur force-pushed the issue-209 branch from 9d977d6 to 68b0b52 Compare November 5, 2020 14:49

zulipbot added size: M [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Nov 5, 2020

manojkestur force-pushed the issue-209 branch from 68b0b52 to 9005f9d Compare November 5, 2020 15:04

zulipbot added size: S [Automatic label added by zulipbot] and removed size: M [Automatic label added by zulipbot] labels Nov 5, 2020

manojkestur force-pushed the issue-209 branch 2 times, most recently from 70bb31e to cab45e9 Compare November 5, 2020 17:16

zulipbot added size: M [Automatic label added by zulipbot] and removed size: S [Automatic label added by zulipbot] labels Nov 5, 2020

manojkestur force-pushed the issue-209 branch from cab45e9 to e64c216 Compare November 5, 2020 17:25

manojkestur force-pushed the issue-209 branch from e64c216 to 5ee9755 Compare November 6, 2020 16:42

buttons/helper/model: Display unread counts of muted streams and topics.

a2621db

Fixes zulip#209

manojkestur force-pushed the issue-209 branch from 5ee9755 to a2621db Compare November 6, 2020 16:59

neiljp added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Nov 21, 2020

Base automatically changed from master to main January 30, 2021 20:31

zulipbot added the has conflicts label Jan 30, 2021

neiljp force-pushed the main branch from f9f483a to 793e73d Compare December 15, 2021 21:30

		('muted_stream', 'light gray', 'black',
		None, DEF['light_gray'], DEF['black']),

Uh oh!

Displays unread message counts of muted streams and topics. #802

Are you sure you want to change the base?

Displays unread message counts of muted streams and topics. #802

Uh oh!

Conversation

manojkestur commented Sep 27, 2020

Uh oh!

manojkestur commented Oct 6, 2020

Uh oh!

neiljp left a comment

Choose a reason for hiding this comment

Uh oh!

neiljp Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

neiljp Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

neiljp Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

manojkestur commented Oct 7, 2020

Uh oh!

manojkestur commented Oct 7, 2020

Uh oh!

neiljp commented Oct 7, 2020

Uh oh!

neiljp commented Oct 27, 2020

Uh oh!

manojkestur commented Oct 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

manojkestur commented Oct 28, 2020

Uh oh!

manojkestur commented Oct 28, 2020

Uh oh!

manojkestur commented Nov 5, 2020

Uh oh!

manojkestur commented Nov 5, 2020

Uh oh!

neiljp commented Nov 6, 2020

Uh oh!

manojkestur commented Nov 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zulipbot commented Jan 30, 2021

Uh oh!

Uh oh!

manojkestur commented Oct 27, 2020 •

edited

Loading

manojkestur commented Nov 6, 2020 •

edited

Loading