ui: Add starred message count in UI. #951

klfoulk16 · 2021-03-16T20:58:43Z

Commit 1:

starred_messages are added to the list of initial_data_to_fetch in model.py.

Commit 2:

StarredButton now stores the count of starred_messages.
The displayed starred_message count is now updated during appropriate message
flag events.
Styles were addd to theme.py for the displayed starred_message count. To do this I:
a. Added optional count_style arg for TopButton.
b. Set this arg to 'unread_count' or 'starred_count' for the appropriate
buttons.
c. Pass count_style to TopButton.update_count function.

zee-bit · 2021-03-16T21:54:24Z

@klfoulk16 Thanks for working on this. 👍
This seems to work pretty well functionally.(though I have not tested it extensively)

Seems like you haven't rebased on top of the recent changes in upstream, you may want to refer the docs here.
Apart from that, I tried running tests on your PR and it seems to fail in some cases. Did you ran the tests locally before pushing?
ZT-development guide can be quite helpful in figuring out how to run the tests locally.

[SIDENOTE: If you want the count to be not yellow(like unread_count), you may want to use update_widget instead of update_count. See my reaction_count commit and how it displays the count visually different from unread_count using update_widget].

klfoulk16 · 2021-03-16T23:21:39Z

@zee-bit Good points! My bad, this was my first code PR and clearly I forgot some things in my excitement. I'll do the rebase, check out the tests and also write some new tests to make sure everything I added is working properly (so far I've been manually checking things - oopsies). Thanks for looking over this

Ezio-Sarthak

Thanks for taking this up 👍 With some refining, this would be quite useful :)

I understand getting started with code/debugging, push/pull is quite messy 😛. It's always cleaner to remove any diffs and commented code that isn't necessary for the PR. This makes it more convenient to view actual changes in the PR :)

Also, I'd encourage always to split UI and event handling elements into their separate commits.

minor: It isn't required to mention PR number in the title itself :)

zulipterminal/config/themes.py

zulipterminal/ui_tools/views.py

klfoulk16 · 2021-03-17T19:50:25Z

Images of the UI. Personally I feel the star count should mimic the text styling so as to not be confused with the "unread_counts." In zt_dark I made the count grey instead of white because the white looked to similar to yellow in my opinion

klfoulk16 · 2021-03-24T15:38:46Z

The tests currently aren't passing because I'm attempting to modify test_model.test_update_star_status to also test the recent changes; however, I'm running into lots of issues figuring out how this test and the function it's testing should be working. I've added some comments in the code to explain the issues I'm finding...I'd love if someone could give me a helping hand explaining why things work the way they work

klfoulk16 · 2021-04-02T13:51:13Z

Ready for review! Finally finished figuring out some tests for the updating the starred message count during a message flags event.

Rohitth007

It's a nice addition to ZT. 👍
Left a few comments, didn't go through everything as I haven't properly explored the message_flags section of the code base.

zulipterminal/model.py

zulipterminal/ui_tools/buttons.py

klfoulk16 · 2021-04-08T16:15:50Z

Updated and ready for another review!

klfoulk16 · 2021-04-10T17:15:01Z

Updated once again and all comments have been resolved 👍

zee-bit

@klfoulk16 Thanks for continuing your work on this. This definitely seems to be on the right path. 👍

I like the slightly grayed-out color for star_count, in the default theme. It is very different from unread_counts and is less distracting to look at, which is important, since the star count is not as critical to the user as unread count.
Apart from the general UI, this seems to mostly work fine, although I did encounter a KeyError while testing. I am not sure if this is particularly related to this PR, but you should be able to reproduce this yourself if you unstar a message from the webapp while being in the starred_message narrow in ZT.
Oh, and as already pointed out by @Ezio-Sarthak, it's better if you could split out the event handling and UI part into separate commits. Below is a more detailed review, hope it helps. :)

tests/model/test_model.py

tests/ui_tools/test_buttons.py

zulipterminal/config/themes.py

zulipterminal/model.py

zee-bit · 2021-04-19T14:15:25Z

zulipterminal/ui_tools/buttons.py

        text_color: Optional[str]=None,
        count: int=0,
+        count_theme: str='unread_count',


Would it be better to use count_color, to have a name that is consistent with text_color?
Also, you might want to consider having a TypeAnnotation similar to text_color.

@Rohitth007 and I discussed naming it count_theme because 'starred_count'/'unread_count' are styles from the theme palette so what's being stored in the arg is the item's style name versus the actual 'color'. Does that make sense? I'd be happy to discuss further and change it if we decide count_color would be better. I'll look into the TypeAnnotation, should it be Optional[str] as well? I need to research more about that

Actually even text_color is a style in the palette. Eg: The user button uses user_idle, user_active etc.
Now that you brought this up I feel both the names feel wrong.

text_color technically can be used not just to change the text color (foreground) but also the background along with styles like bold, italics, etc. This can be used when we select a different background color from the default.

count_theme feels wrong as theme is more like a collection of styles?

text_style and count_style seem apt here. This has to be done in a different commit if going forward with it.
Thoughts?

Now that you brought this up I feel both the names feel wrong.

Right. I was about to say this.
*_style here seems fine. urwid, however, likes to call those display_attributes, but that seems a bit too long IMO? And, yes, definitely keep the name change in a different commit, since text_color would require changing it in multiple places.

Re TypeAnnotation, Note that there's also a None display_attr in themes, which could also be passed here, so its better to generalize the type as Optional[str].

So shall I update the name of count_theme in the original commit? Seems logical to name it correct from the start. I can create a third commit to fix the text_color name. I agree with the name change I think it makes a lot more sense

If I change count_style to Optional[str]=None it causes an error with the TopButton.update_widget function...Technically the UserButton doesn't have a count_style but nor does it use the update_widget function...Not sure how to handle this. Would you change the types that update_widget accepts? Or would you change the TypeAnnotation for the count_style arg?

This was the error message:
zulipterminal/ui_tools/buttons.py:67: error: Argument 1 to "update_widget" of "TopButton" has incompatible type "Tuple[Optional[str], str]"; expected "Tuple[str, str]"

Fixing the Type of update widget would do as previously unread_count was hard coded.
Re UserButton, it too displays count in yellow ryt?
Also I just saw that UserButton uses update_widget in a weird way, it uses (you) as count text. Maybe it can be changed in another PR.

I can't seem to find where UserButton uses update_widget, do you mind sharing the file/line number where you found that?

I fixed the TopButton.update_widget's TypeAnnotation to be Tuple[Optional[str], str]. I also declared count_style='unread_count' in all of the non-starred TopButton children's' super.__init__ functions....except for the user button because I couldn't find where it was used. Shall I go in and add count_style='unread_count', to the UserButton's init function as well?

klfoulk16 · 2021-04-20T00:55:48Z

@klfoulk16 Thanks for continuing your work on this. This definitely seems to be on the right path. 👍

I like the slightly grayed-out color for star_count, in the default theme. It is very different from unread_counts and is less distracting to look at, which is important, since the star count is not as critical to the user as unread count.
Apart from the general UI, this seems to mostly work fine, although I did encounter a KeyError while testing. I am not sure if this is particularly related to this PR, but you should be able to reproduce this yourself if you unstar a message from the webapp while being in the starred_message narrow in ZT.
Oh, and as already pointed out by @Ezio-Sarthak, it's better if you could split out the event handling and UI part into separate commits. Below is a more detailed review, hope it helps. :)

So for separating the event handling and UI part, would that just involve making the changes to the theme file and the addition of the 'count_theme' arg to the TopButton a different commit? Also thanks for the review! You pointed out a lot of idiosyncrasies

zee-bit · 2021-04-20T11:50:18Z

So for separating the event handling and UI part, would that just involve making the changes to the theme file and the addition of the 'count_theme' arg to the TopButton a different commit?

Yep, that should be all.
Generally, the model.py file handles all the events so it's better to split that change into a single commit at the beginning.
You can also add the changes in the themes file in an intermediate commit after the event handling.
Then work on the UI part in the later commit(s). :)

klfoulk16 · 2021-04-20T15:29:43Z

@zee-bit I can't seem to reproduce the KeyError you speak of? I've tried unstarring and starring multiple messages from within the webapp while keeping ZT open to the starred_messages narrow and everything seems to work as expected?

zee-bit · 2021-04-26T16:20:31Z

@klfoulk16 I am really sorry for replying so late (I have my Endsem's going on and would probably be occupied, atleast until 3rd). I am not sure if you were able to reproduce the KeyError that I mentioned earlier. In case you weren't able to reproduce that, here are the steps:

Open both webapp and ZT side-by-side.
Scroll up to some old message in the webapp.
Star a few messages in the webapp and narrow to the starred section in ZT to confirm they are present.
Unstar those message from the web app. And when you are done, scroll through those messages in ZT, and you should probably get a KeyError.

Here is a screenshot of the KeyError along with another bug I encountered while testing, i.e. negative count. The negative starred count seems to be pretty similar to the existing bug of negative unreads(see #642 and #574). However, I currently haven't figured out a reliable way of reproducing that. Will report back as soon as I do.

klfoulk16 · 2021-04-26T16:58:44Z

Changes in the most recent push:

Condensed the relatively similar functions for testing add, remove star_count into one function.
Removed unnecessary StarredButton.update_widget test and changed it to tests checking the StarredButton's init functions to check that the new count_style arg is correctly assigned.
Renamed TopButton's text_color arg to text_style per our discussions above

Thanks @zee-bit for the clarification about the bug you found; however, I still am unable to reproduce it. After the final part where I unstar the messages from the web app and then scroll through those messages in ZT (I assume I am scrolling through them in the starred_messages narrow?) I do not get a TypeError. Instead it appears as if I had unstarred the messages from ZT, with the messages still displayed in the starred_messages narrow, but they no longer have a star nor are they included in the displayed Starred Messages count. Scrolling through them causes no issues.

neiljp

@klfoulk16 Just done a first pass over this having tested it - there are a few things to fix/improve, but this is looking good 👍 I've mainly focused on functionality and commit structure this time, as smaller commits are that much easier to review. If there are additional parts I've not indicated that are independent separate improvements - maybe to tests? - then feel free to break into a separate commit too if that.

zulipterminal/ui_tools/buttons.py

zulipterminal/model.py

neiljp

@klfoulk16 Thanks for the reworking - the split of commits makes this much cleaner to review 👍 As before this seems to work fine, with the possible exception of monochrome mode (1-color bit depth), having now read the theme changes in their own commit.

I didn't focus as much on tests last time, but with smaller commits it's easier to read and so that's the bulk of my (mostly minor) suggestions this time around.

Some of the edits do change multiple files which aren't reflected in the commit areas (eg model: title vs model/buttons/views: title?)

We could keep the color/style naming refactor (you can add that as a separate commit prefix, if you see in git log) here or in another PR if you like, since there seem to be a cascade of other names which might be affected?

In any case I'd like to get the core of this merged as it'll be useful! 👍
(a good follow-up would be to use the new server feature where this is enabled/disabled via a setting, but we can save that for later in an issue if you'd prefer)

zulipterminal/config/themes.py

neiljp · 2021-05-05T22:56:42Z

tests/model/test_model.py

+                           starred_msg_ids=set([msg_id
+                                                for msg_id in indexed_ids
+                                                if 'starred' in flags_before]))


What I don't understand here is that the we're not updating the behavior of the code which uses this data. I do see that the tests are failing without this though.

tests/model/test_model.py

zulipterminal/ui_tools/buttons.py

neiljp · 2021-05-05T23:36:11Z

tests/ui_tools/test_buttons.py

+        assert (((top_button.count_style, '11'), None)
+                == top_button.update_widget.call_args[0])


I can see how this works, but this might be cleaner with ...update_widget.assert_called_once_with(...)?

I agree that version would be cleaner, but for some reason I keep getting the following error when I use that:

E AssertionError: assert None E + where None = <bound method wrap_assert_called_once_with of <MagicMock name='update_widget' id='4393243264'>>(('unread_count', '11'), None) E + where <bound method wrap_assert_called_once_with of <MagicMock name='update_widget' id='4393243264'>> = <MagicMock name='update_widget' id='4393243264'>.assert_called_once_with E + where <MagicMock name='update_widget' id='4393243264'> = <TopButton selectable flow widget ''>.update_widget tests/ui_tools/test_buttons.py:107: AssertionError

Yet if I get rid of 'None' then the test fails and send this:

E AssertionError: expected call not found. E Expected: update_widget('unread_count', '11') E Actual: update_widget(('unread_count', '11'), None)

I found that top_button.update_widget.call_args[0]) was a workaround that had the expected behavior. Any ideas why the update_widget.assert_called_once_with was acting funky?

neiljp · 2021-05-05T23:39:25Z

tests/ui_tools/test_buttons.py

+                               caption='caption',
+                               show_function=show_function,
+                               width=12,
+                               count=10,
+                               count_style='unread_count')


We don't use show_function, so you could inline the mocker.Mock() here?

Also all the perhaps somewhat arbitrary parameters can be set as parameters to the test function with default values, which you can refer to later using variable names or expressions - such as for the 11 constant used later? (the same applies elsewhere)

Good suggestions, thank you!

tests/ui_tools/test_buttons.py

zulipterminal/ui_tools/buttons.py

neiljp · 2021-05-05T23:48:19Z

zulipterminal/ui_tools/buttons.py

@@ -258,7 +258,7 @@ def __init__(self, user: Dict[str, Any], controller: Any,
            show_function=self._narrow_with_compose,
            width=width,
            prefix_character=(color, state_marker),
-            text_color=color,
+            text_style=color,


I added some comments in the zulip topic about this...I went down a bit of a rabbit hole wondering whether we should actually change text_color to text_style

Updated tests.

1. buttons/views: Store starred_messages count from initial_data in StarredButton. 2. model: Update the displayed starred message count during appropriate message flag events for all messages, regardless of whether they're in the index or not. Tests updated.

Create new style called starred_count to style the new displayed starred_count with, distinct from unread_count. Create variations for each theme.

1. Set argument to unread_count or starred_count for appropriate buttons. 2. Pass count_style arg to TopButton.update_count function so the counts are styled appropriately. Tests updated.

neiljp · 2021-05-09T20:22:03Z

@klfoulk16 Thanks! As mentioned in the stream I pushed some minor changes, left CI to run and have just merged 🎉!

You may like to review the changes to follow what may not have worked for you earlier or what you could explore in future - eg. I adjusted some tests to push constants into test function parameters, got that alternative assert style working and made a few minor renames.

zulipbot added the size: M [Automatic label added by zulipbot] label Mar 16, 2021

klfoulk16 changed the title ~~[WIP] ui: Add starred message count in UI. #578~~ [WIP] ui: Add starred message count in UI. T#578 Mar 16, 2021

klfoulk16 changed the title ~~[WIP] ui: Add starred message count in UI. T#578~~ [WIP] ui: Add starred message count in UI. #T578 Mar 16, 2021

Ezio-Sarthak suggested changes Mar 17, 2021

View reviewed changes

zulipterminal/config/themes.py Outdated Show resolved Hide resolved

zulipterminal/ui_tools/views.py Outdated Show resolved Hide resolved

klfoulk16 changed the title ~~[WIP] ui: Add starred message count in UI. #T578~~ [WIP] ui: Add starred message count in UI. Mar 17, 2021

klfoulk16 force-pushed the issue-578-starred-message-count-display branch from 182a281 to 250ee86 Compare March 17, 2021 13:02

klfoulk16 marked this pull request as draft March 17, 2021 19:48

klfoulk16 force-pushed the issue-578-starred-message-count-display branch from 2cad9fb to f7b069c Compare March 24, 2021 15:33

zulipbot added size: XL [Automatic label added by zulipbot] and removed size: M [Automatic label added by zulipbot] labels Mar 24, 2021

klfoulk16 force-pushed the issue-578-starred-message-count-display branch from f7b069c to 7a05b82 Compare March 24, 2021 15:34

klfoulk16 force-pushed the issue-578-starred-message-count-display branch 3 times, most recently from aa76c20 to 007c5d3 Compare March 24, 2021 16:28

klfoulk16 force-pushed the issue-578-starred-message-count-display branch from 007c5d3 to 3baad7b Compare April 2, 2021 13:27

klfoulk16 changed the title ~~[WIP] ui: Add starred message count in UI.~~ ui: Add starred message count in UI. Apr 2, 2021

klfoulk16 force-pushed the issue-578-starred-message-count-display branch from 3baad7b to 636e31b Compare April 2, 2021 13:33

klfoulk16 marked this pull request as ready for review April 2, 2021 13:49

Rohitth007 reviewed Apr 2, 2021

View reviewed changes

zulipterminal/model.py Outdated Show resolved Hide resolved

zulipterminal/ui_tools/buttons.py Outdated Show resolved Hide resolved

klfoulk16 force-pushed the issue-578-starred-message-count-display branch 4 times, most recently from f43e9ac to eb956c6 Compare April 8, 2021 16:13

klfoulk16 force-pushed the issue-578-starred-message-count-display branch from eb956c6 to a07239f Compare April 8, 2021 20:05

zee-bit reviewed Apr 19, 2021

View reviewed changes

klfoulk16 force-pushed the issue-578-starred-message-count-display branch 5 times, most recently from 315e15e to ab5e652 Compare April 26, 2021 15:39

neiljp reviewed Apr 28, 2021

View reviewed changes

zulipterminal/ui_tools/buttons.py Show resolved Hide resolved

zulipterminal/model.py Outdated Show resolved Hide resolved

klfoulk16 force-pushed the issue-578-starred-message-count-display branch 5 times, most recently from fb07d60 to 7b7d813 Compare May 1, 2021 23:51

neiljp reviewed May 6, 2021

View reviewed changes

model: Add starred_messages to list of initial_data_to_fetch.

0c5f247

Updated tests.

klfoulk16 force-pushed the issue-578-starred-message-count-display branch from 7b7d813 to 4bef25c Compare May 6, 2021 17:40

klfoulk16 added 3 commits May 7, 2021 14:48

themes: Add new starred_count style for each theme.

1c1312d

Create new style called starred_count to style the new displayed starred_count with, distinct from unread_count. Create variations for each theme.

buttons: Add optional count_style argument for TopButton.

c853245

1. Set argument to unread_count or starred_count for appropriate buttons. 2. Pass count_style arg to TopButton.update_count function so the counts are styled appropriately. Tests updated.

neiljp force-pushed the issue-578-starred-message-count-display branch from 4bef25c to c853245 Compare May 9, 2021 13:27

neiljp merged commit 00c8993 into zulip:main May 9, 2021

neiljp added this to the Next Release milestone May 9, 2021

This was referenced May 9, 2021

Show number of starred messages next to button #578

Closed

Respect user display setting on server for whether to show starred count #1027

Open

		assert (((top_button.count_style, '11'), None)
		== top_button.update_widget.call_args[0])

ui: Add starred message count in UI. #951

ui: Add starred message count in UI. #951

Conversation

klfoulk16 commented Mar 16, 2021 • edited Loading

zee-bit commented Mar 16, 2021 • edited Loading

klfoulk16 commented Mar 16, 2021 • edited Loading

Ezio-Sarthak left a comment

Choose a reason for hiding this comment

klfoulk16 commented Mar 17, 2021

klfoulk16 commented Mar 24, 2021

klfoulk16 commented Apr 2, 2021

Rohitth007 left a comment

Choose a reason for hiding this comment

klfoulk16 commented Apr 8, 2021

klfoulk16 commented Apr 10, 2021

zee-bit left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klfoulk16 Apr 20, 2021 • edited Loading

Choose a reason for hiding this comment

Rohitth007 Apr 20, 2021 • edited Loading

Choose a reason for hiding this comment

zee-bit Apr 20, 2021 • edited Loading

Choose a reason for hiding this comment

klfoulk16 Apr 20, 2021 • edited Loading

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

klfoulk16 commented Apr 20, 2021 • edited Loading

zee-bit commented Apr 20, 2021

klfoulk16 commented Apr 20, 2021

zee-bit commented Apr 26, 2021

klfoulk16 commented Apr 26, 2021 • edited Loading

neiljp left a comment

Choose a reason for hiding this comment

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klfoulk16 May 6, 2021 • edited Loading

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

klfoulk16 May 6, 2021 • edited Loading

Choose a reason for hiding this comment

neiljp commented May 9, 2021

klfoulk16 commented Mar 16, 2021 •

edited

Loading

zee-bit commented Mar 16, 2021 •

edited

Loading

klfoulk16 commented Mar 16, 2021 •

edited

Loading

zee-bit left a comment •

edited

Loading

klfoulk16 Apr 20, 2021 •

edited

Loading

Rohitth007 Apr 20, 2021 •

edited

Loading

zee-bit Apr 20, 2021 •

edited

Loading

klfoulk16 Apr 20, 2021 •

edited

Loading

klfoulk16 commented Apr 20, 2021 •

edited

Loading

klfoulk16 commented Apr 26, 2021 •

edited

Loading

klfoulk16 May 6, 2021 •

edited

Loading

klfoulk16 May 6, 2021 •

edited

Loading