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

[WIP (tests/locking?)] Update read and starred messages methods

Open neiljp opened this issue 6 years ago • 5 comments

This is intended to add read-flag updates sent from the server, so if messages are read in the webapp and ZT is running, the intention is that the unread counts and status in ZT should update at the same time. It also extends the capability of the similar starred feature to cover where multiple message updates occur at once, in line with the read-flag version.

I would appreciate any testing of this PR :)

CAVEATS:

  • On reading many messages quickly (in the webapp), the numbers can become out of sync with ZT (is this due to some kind of locking issue? multiple messages sent from the server which aren't responded to?)
  • Some stream totals are occasionally wrong on loading?
  • I'd like to add some tests for the new method, but wanted to look for feedback first.

neiljp avatar Dec 19 '18 22:12 neiljp

For current master, reading messages on ZT directly update webapp's unread count. I have confirmed that it is not the case when the causal order is the other way around.

rht avatar Dec 23 '18 01:12 rht

@rht Do you find the numbers match, or are there certain ones which become out of sync?

I'm fairly convinced that this PR would be an improvement over master, but there are definitely some further updates to make it match completely, which can perhaps be filed in a separate issue?

neiljp avatar Dec 23 '18 01:12 neiljp

On this branch, I tried reading on webapp 2 messages, then on ZT, toggled the stream list display with q then esc. Didn't see any update even after 20s+.

rht avatar Dec 23 '18 01:12 rht

@rht Do you have a comparison with master and this branch? It is difficult to duplicate without running a server, perhaps? Also, I've found it may work better when I had the current autohide code reverted, so I'm not sure if this branch plus the one I pushed recently for better autohide behavior (particularly if autohide is disabled using it) may work better, though of course we want it to work with autohide too!

neiljp avatar Dec 23 '18 02:12 neiljp

Heads up @neiljp, 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