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

Ensure all-messages narrow is not empty via is-private/unmuted-streams

Open neiljp opened this issue 5 years ago • 2 comments

This should resolve #326; the approach is somewhat WIP but is functional and could be merged once tests are updated/added.

The approach taken is as follows:

  • extend fetching messages to be from some narrow only (optionally)
  • keep track of (initial) unmuted streams
  • if there are no unmuted messages at startup, fetch more.

This is more deterministic than the original approach discussed and proposed in #341.

@sumanthvrao This was the alternative approach I thought of and probably discussed with you.

neiljp avatar Jul 31 '19 18:07 neiljp

I wrote the following, and I think it could be useful, but there may be an API-based solution which would make this easier, though Tim's comment on #326 originally pushed me away from that idea.


@amanagr I am totally open to discussion on this, and I think it could be a case of working on various iterations, as some would 'fix' the crash, but wouldn't give a full solution.

My understanding is that the current situation is that we attempt to fetch 30 messages initially, but if all these messages are muted (in muted streams/topics) then we end up with no messages in the all-messages narrow and can crash. If we take the approach of wanting to show at least one message (assuming one is present!) then we need to do at least one additional initial fetch (the alternative being just showing an 'empty' narrow as per the almost ready #278 from @sumanthvrao - for some time!).

Fetching from the all messages narrow would give recent messages, but depending on where they are (maybe lots are muted?) then the same situation holds, and the client could keep fetching messages back in time, over and over again, and maybe just keep finding muted messages. This is what concerned me about #326.

The alternative is to be selective on a per-narrow basis; the obvious solution is PMs, which are guaranteed not to be muted, but while this should avoid a crash, doing this alone doesn't seem representative of showing all messages in time order - given that there could be recent un-muted stream messages not shown in that interleaved view if we just fetched PMs. This is why I added the fetching from un-muted streams.

These unmuted streams are only those subscribed, not all those on the server, as I understand it. While people could be subscribed to a lot, this implementation is currently only triggered if people get a lot of messages in muted streams. That is an obvious use-case of muting, but depends on the balance of messages people receive in muted vs non-muted streams.

One additional factor is that we could exclude unpinned streams from the secondary fetch, though I'm not a fan of that approach either, as while they're "less important", we still show content from them, unliked muted.

neiljp avatar Aug 03 '19 00:08 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