zulip-terminal
zulip-terminal copied to clipboard
Shows/updates content header for message when starred
Part of #171
Splits and shows content header for a message (having a similar timestamp as the previous message(s)) when starred.
For multiple messages in succession having similar timestamps, initially, often times, it would not show that a message is starred when the user presses *
, although it updates on other Zulip platforms.
Now, however, in such cases, the message is shown as starred along with the timestamp of that message.
~~What's left: When a message is starred, the messages following it do not split from the starred message, making it difficult for the user to figure out if it is one big message which is starred or if there are multiple messages and only one is starred.~~ Fixed.
That said, the changes here do seem larger than I would expect, and I'm not sure we can rely on the initial state and user updates in zulip-terminal being the only way that the data can change (ie. use Model.message_star_toggled that you introduced), as the status can change on the server via other clients. That is why we rely on is_starred in the messages themselves, unless there is a part I am missing that needs the new part you introduced.
@neiljp, every time the user presses *
, the function update_rendered_view
is called (after toggle_message_star_status
and update_message_flag_status
).
In order for there to be a split before the next message, update_rendered_view
needs to be called for the next message as well. I could've just modified this function so that it would cause the message box of the current message and that of the next message to be updated but that seemed very redundant since update_rendered_view
is being called at other places as well.
Hence, I introduced Model.message_star_toggled
which basically checks if toggle_message_star_status
was called. If true, only then it updates the message box of the next message as well.
Note you have no newline at the end of one file, which is why there is the small red symbol; I've not checked to see if the test fails are related to that or not.
That, and some of the lines are too long. I'll fix that in the next commit.
If you star-toggle a message not in ZT, but have ZT running, none of the code in model.py you added will be triggered. Think about the flow here.
Right, I'll read up on that and fix it. Although I think I'll be requiring some help with this, so I'll ask on czo.
That said, there were two issues that I noticed while testing manually:
- I get a traceback, at least when I toggle the star at the end
- I expected groups of starred messages to 're-merge', but they did not
Fixed the first issue. As for the second issue, I'm not sure about the idea... It'll be hard for the user to figure out whether it is one long message which is starred or multiple messages which are all starred.
@neiljp, are there any changes required in this?
@neiljp, does this require any changes?
Heads up @shreyamalviya, 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/master
branch and resolve your pull request's merge conflicts accordingly.