zulip-terminal icon indicating copy to clipboard operation
zulip-terminal copied to clipboard

Shows/updates content header for message when starred

Open shreyamalviya opened this issue 5 years ago • 5 comments

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.

shreyamalviya avatar Mar 30 '19 12:03 shreyamalviya

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.

shreyamalviya avatar Apr 03 '19 05:04 shreyamalviya

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.

shreyamalviya avatar Apr 09 '19 17:04 shreyamalviya

@neiljp, are there any changes required in this?

shreyamalviya avatar Apr 16 '19 13:04 shreyamalviya

@neiljp, does this require any changes?

shreyamalviya avatar Jun 07 '19 04:06 shreyamalviya

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.

zulipbot avatar Apr 17 '20 00:04 zulipbot