Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix handling un-read count for un-indexed messages. #398

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix handling un-read count for un-indexed messages. #398

wants to merge 1 commit into from

Conversation

sumanthvrao
Copy link
Member

@sumanthvrao sumanthvrao commented Jun 20, 2019 •
edited
Loading

This PR is a step in the direction of fixing #395.

TODO:
Add tests

@zulipbot zulipbot added the size: L [Automatic label added by zulipbot] label Jun 20, 2019
@sumanthvrao
Copy link
Member Author

sumanthvrao commented Jun 20, 2019 •
edited
Loading

@amanagr provided you pointed out the incorrect fix in #393, what do you think of this approach

@amanagr
Copy link
Member

amanagr commented Jun 20, 2019

Thanks @sumanthvrao. I just checked the response we receive from the server and it includes found_newest in it. This I found out is a new parameter which server gives us(https://chat.zulip.org/api/get-messages). So, while czo gives us this new parameter, older versions of the webapp will not, so we should primarily use found_newest to determine if we have fetched all the messages in the narrow, and then use the logic you came up with to determine if there are more messages to be fetched.

@sumanthvrao
Copy link
Member Author

sumanthvrao commented Jun 20, 2019 •
edited
Loading

Great. Will check it out 👍 !

@sumanthvrao sumanthvrao mentioned this pull request Jun 21, 2019
Closed
@sumanthvrao sumanthvrao force-pushed the zt-update-flag branch 4 times, most recently from 5a8a330 to 90b427e Compare June 26, 2019 05:55
@sumanthvrao
Copy link
Member Author

Updated this 👍

@neiljp The first 3 commits here are quite independent, there were also some of the changes we were discussing in #393 . This includes, a small indentation change (commit 1), renaming update to found_newest, which is also the variable the API returns (@punchagan what do you think of this name?) and fixing it as a property of model (commit 2) , and making use of the found_newest to update the flag (Thanks for the information @amanagr ) (commit 3)

The later commits (Last 2) are sort of techniques I came up with, which works for now (regardless of them being the best solution). I would love to have your feedback on them.

@sumanthvrao sumanthvrao force-pushed the zt-update-flag branch 3 times, most recently from de9eadc to 611ff86 Compare June 27, 2019 06:02
neiljp
neiljp reviewed Sep 11, 2019
View reviewed changes
Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sumanthvrao While I've left some comments, I think this may be simpler if we associated the 'decrease' (ie. transfer from unindexed to indexed) with just being called whenever we successfully get_messages?

That would make it easier to keep the state in the Model and maybe even remove the need for at least one of the extra methods - and maybe rename the other?

zulipterminal/ui_tools/views.py Outdated
@@ -87,6 +87,7 @@ def load_new_messages(self, anchor: int) -> None:
else:
last_message = None

self.model.decrease_unindexed_message_count(new_ids)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is necessary for load_old_messages, too, I suspect?

zulipterminal/model.py
def increase_unindexed_message_count(self,
message: Dict[str, Any]) -> None:
unindexed_messages[message['id']] = message
set_count([message['id']], self.controller, 1)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to check for whether the message is already marked as read?

zulipterminal/helper.py Outdated
@@ -39,6 +39,7 @@
all_starred=set(),
)

unindexed_messages = dict() # type: Dict[int, Any]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand why you might want to avoid having this in the Index itself, but I'm unconvinced it should be a global variable.

For now, placing this in the model might be simpler?

@sumanthvrao
bugfix: Update unread count for messages which cannot be indexed.
8e9be70 
For narrows, in which not all the messages have been fetched yet,
when a new message arrives, it cannot be appended to the view (index).
However, even its unread count does not increase. To avoid this,
we maintain an un-indexed message collection.

When, a new message arrives, we add its id to the un-indexed collection
and increment its count.

When we narrow to a stream, or load_new_messages, narrow_to_user, etc,
we pop the fetched message which are un-indexed as they
are now indexed.

The indexed and un-indexed collections are mutually exclusive to each other.

Tests amended.

Fixes: zulip#395
@zulipbot zulipbot added size: M [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Sep 27, 2019
@zulipbot
Copy link
Member

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

Base automatically changed from master to main January 30, 2021 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neiljp neiljp neiljp left review comments

Assignees
No one assigned
Labels
has conflicts size: M [Automatic label added by zulipbot]
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sumanthvrao @amanagr @zulipbot @neiljp