Skip to content

Handle read events from server #535

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
Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Handle read events from server #535

wants to merge 3 commits into from

Conversation

kaustubh-nair
Copy link
Member

@kaustubh-nair kaustubh-nair commented Mar 10, 2020 •
edited
Loading

TODO:

  • Limit number of unread msgs that are stored to ~10k

@zulipbot zulipbot added the size: L [Automatic label added by zulipbot] label Mar 10, 2020
@kaustubh-nair kaustubh-nair mentioned this pull request Mar 10, 2020
Closed
@kaustubh-nair kaustubh-nair changed the title Handle read events from server WIP: Handle read events from server Mar 10, 2020
@neiljp
Copy link
Collaborator

neiljp commented Mar 17, 2020

@kaustubh-nair I know this is WIP, but continuing this work would be helpful to reliably track unread counts, depending on how busy you are with other things right now.

@kaustubh-nair kaustubh-nair mentioned this pull request Mar 18, 2020
Open
10 tasks
@kaustubh-nair
Copy link
Member Author

@neiljp I was just finishing up with the typeahead footer PR, I'll be working on this one now.
Added some tests for set_counts in the PR referenced above.

@kaustubh-nair kaustubh-nair marked this pull request as ready for review March 18, 2020 18:33
@kaustubh-nair kaustubh-nair force-pushed the unread_v2 branch 4 times, most recently from 5687b88 to 4378646 Compare March 22, 2020 18:32
@zulipbot zulipbot added size: XL [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Mar 22, 2020
@kaustubh-nair kaustubh-nair force-pushed the unread_v2 branch 2 times, most recently from 9d7ba6c to 02eba43 Compare March 23, 2020 13:46
@zulipbot zulipbot added size: L [Automatic label added by zulipbot] and removed size: XL [Automatic label added by zulipbot] labels Mar 23, 2020
@kaustubh-nair kaustubh-nair force-pushed the unread_v2 branch 2 times, most recently from 18bfb3c to 4d04157 Compare March 23, 2020 16:31
@zulipbot zulipbot added size: XL [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Apr 1, 2020
@kaustubh-nair kaustubh-nair changed the title WIP: Handle read events from server Handle read events from server Apr 2, 2020
kaustubh-nair
kaustubh-nair commented Apr 2, 2020
View reviewed changes
zulipterminal/helper.py

# If we sent this message, don't increase the count
if user_id == controller.model.user_id:
continue
Copy link
Member Author

Choose a reason for hiding this comment

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

This is kind of hacky - we don't have sender_id for stream messages in initial_data['unread_msgs']
Though on close inspection this piece of code doesn't seem to be doing anything?
Refer to conversation on czo https://chat.zulip.org/#narrow/stream/206-zulip-terminal/topic/Handling.20read.20events/near/845434

@neiljp neiljp added the PR needs review PR requires feedback to proceed label Apr 3, 2020
@kaustubh-nair kaustubh-nair force-pushed the unread_v2 branch 2 times, most recently from 2a628e7 to 06ddfb7 Compare April 3, 2020 10:45
neiljp
neiljp reviewed Apr 5, 2020
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.

@kaustubh-nair The PR title refers to 'handle read events', and in this sense it seems to work well 👍

I didn't anticipate the approach you've taken - rather messages+unread being handled separately in each case where they would be updated.

Functionality while it's not strictly part of the PR intent, it would seem good to handle the addition (append'ing) of messages too.

I'm wary that we're ending up with two datastructures which hold overlapping information, and in some cases lots of duplicate information.

I also get a bug where changed_messages (helper.py, L191) has a KeyError (id not in messages, presumably) in some cases.

I'm concerned by the change of the elif statement for 1-1 pms etc, but we can look into that - I've not dug deep into that.

@neiljp neiljp removed the PR needs review PR requires feedback to proceed label Apr 5, 2020
@kaustubh-nair
Copy link
Member Author

@neiljp Thanks for the review!
I've dealt with handling appending of messages as well. I'm pretty sure the keyerror you came across was due to this, since set_count was being called while new messages was not added to the index. If you still come across the exception, let me know!

Last two commits can be squashed later; I've kept them separate for now.

@neiljp neiljp added the PR needs review PR requires feedback to proceed label Apr 9, 2020
@neiljp
Copy link
Collaborator

neiljp commented May 9, 2020

@kaustubh-nair I'm still getting tracebacks in this PR; I've not read through it again in detail though, so can't make further comments at this stage.

@kaustubh-nair
helper: Store unread message data in index.
e7621d5 
Add 'unread_msgs' attribute in index to store all data pertaining to
unread messages obtained from initial_data.

Modify classify_unread_counts to return this unread_msgs data structure.

Tests amended.
@kaustubh-nair
helper: Modify set_counts to use index['unread_msgs'].
c2a1380 
Per-message 'sender_id' is not available for stream messages. Due to
this, messages sent by user might cause undesirable behaviours.
@kaustubh-nair
model: Update index['unread_msgs'] when appending messages.
0169add 
Entries are added to unread message datastructure when new messsages are
added, so that unread count can be updated.

Tests amended.
@kaustubh-nair kaustubh-nair mentioned this pull request Jun 29, 2020
Open
Base automatically changed from master to main January 30, 2021 20:30
@zulipbot
Copy link
Member

Heads up @kaustubh-nair, 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/main branch and resolve your pull request's merge conflicts accordingly.

@mkp6781
Copy link
Contributor

mkp6781 commented Apr 4, 2021

@kaustubh-nair can I take this up if you are not planning to continue work on this?

@mkp6781 mkp6781 mentioned this pull request Apr 14, 2021
Open
@neiljp neiljp added this to the Release after next milestone Apr 12, 2022
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 PR needs review PR requires feedback to proceed size: XL [Automatic label added by zulipbot]
Projects
None yet
Milestone
Next Release
Development

Successfully merging this pull request may close these issues.
4 participants
@kaustubh-nair @neiljp @zulipbot @mkp6781