Skip to content

model/boxes: Add functionality to send typing event to server for PMs. #845

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
Closed
wants to merge 3 commits into from
Closed

model/boxes: Add functionality to send typing event to server for PMs. #845

wants to merge 3 commits into from

Conversation

prah23
Copy link
Member

@prah23 prah23 commented Dec 9, 2020

Added a function that enables ZT to be sending typing events to the server.
This function is asynchronous to avoid the lag that occurs before the key is displayed on the screen, which is generated by the client making the API call to send the event.

Configured private_box_view to use the function to send the event.
Fixes #593.

neiljp
neiljp reviewed Dec 17, 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.

@prah23 Thanks for looking at this 👍 I'll review this in more detail once you rebase against master to get the tests running, but I've left a few comments.

Also this doesn't seem to send typing stop events before cancelling/sending - eg. if the sender pauses for some time?

@neiljp neiljp added the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Dec 17, 2020
@prah23
Copy link
Member Author

prah23 commented Dec 17, 2020

@prah23 Thanks for looking at this 👍 I'll review this in more detail once you rebase against master to get the tests running, but I've left a few comments.

Also this doesn't seem to send typing stop events before cancelling/sending - eg. if the sender pauses for some time?

I haven't sent typing stop events before cancelling/sending because the API seems to do it on its own if it doesn't receive a start event for about 10 seconds.

@zulipbot zulipbot added the size: L [Automatic label added by zulipbot] label Dec 18, 2020
@prah23 prah23 force-pushed the send_typing_event branch 2 times, most recently from 7056fcf to baeeaad Compare December 18, 2020 19:27
@neiljp neiljp added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Dec 20, 2020
neiljp
neiljp reviewed Dec 20, 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.

@prah23 This appears to work quite well, including the revised stop events 👍

The windowing on the start/stop events seems to limit the sending of too many messages to the server, which is an important characteristic. For an improved version I would look to avoid generating the many extra threads each time we type. They only sleep and do nothing for such rapid typing, but perhaps we could simplify that in a future refined version.

Could you use the gitlint hook or otherwise wrap your commit text onto multiple lines - gitlint should give helpful feedback (and we can refine the rules we use)

@neiljp neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Dec 20, 2020
@prah23
Copy link
Member Author

prah23 commented Dec 22, 2020

Thank you for the gitlint suggestion. I set the hook up and used it to lint my commit messages.

@prah23
Copy link
Member Author

prah23 commented Dec 24, 2020 •
edited
Loading

@zulipbot add "PR needs review"

@zulipbot zulipbot added the PR needs review PR requires feedback to proceed label Dec 24, 2020
@prah23
Copy link
Member Author

prah23 commented Dec 24, 2020

@zulipbot remove "PR needs update"

@zulipbot zulipbot removed the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Dec 24, 2020
@prah23 prah23 force-pushed the send_typing_event branch from c7dd775 to e8eb246 Compare January 2, 2021 17:51
@prah23 prah23 requested a review from neiljp January 3, 2021 16:27
@prah23 prah23 force-pushed the send_typing_event branch from e8eb246 to 73b0f51 Compare January 9, 2021 10:55
neiljp
neiljp reviewed Jan 11, 2021
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.

@prah23 This looks mostly close to being ready 👍 The commit messages look much tidier.

Hopefully most of my points should be straightforward to respond to or address, but let me know :)

I think the remaining concern I have is regarding the number of threads we generate - one on each keypress!
They do mostly sleep and quickly disappear after the sleep period, but I noticed about 78 extra at one point after some rapid typing!
Any thoughts on a slight variation to address this?

@neiljp neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Jan 11, 2021
@prah23 prah23 force-pushed the send_typing_event branch 2 times, most recently from 7e99d2c to dc421ae Compare January 12, 2021 08:29
@prah23
Copy link
Member Author

prah23 commented Jan 12, 2021

@neiljp I've modified the idleness check method to be called inside the start status function, every time a start status is sent (which is once every 10 seconds, given the user is still typing), and once the idle status function is called, it runs till it finds a 5-second halt. If there is already an idle status thread running, we don't call another one. This way, we'd have very few threads, compared to the previous approach.

@prah23 prah23 requested a review from neiljp January 12, 2021 21:28
@prah23
Copy link
Member Author

prah23 commented Jan 12, 2021

My apologies for re-requesting a review, @zulipbot seems to be down, just wanted to let you know it is updated. I'll wait till you have some time to spare.

@prah23 prah23 force-pushed the send_typing_event branch 3 times, most recently from 5a058c6 to db28def Compare January 13, 2021 19:10
neiljp
neiljp reviewed Jan 13, 2021
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.

@prah23 Thanks for the update - this shows a good adjustment of the algorithm to avoid the extra threads and still works 👍

I've made some notes inline, but mostly we need to avoid that busy wait, which is the alternative to the multiple threads right now. We have fewer threads, but now one is keeping the CPU busy. Now there's only one extra thread it may be easier to adjust further, however?

Ideally we'd have tests for this; that may be challenging to write, but may make refactoring the code into different forms easier to do, if they're good quality tests!

@prah23 prah23 force-pushed the send_typing_event branch 2 times, most recently from 19b4520 to 3be2bda Compare January 14, 2021 21:23
@zulipbot zulipbot added size: XL [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Jan 14, 2021
@prah23 prah23 requested a review from neiljp January 15, 2021 10:49
@prah23 prah23 force-pushed the send_typing_event branch from 3be2bda to a18f80f Compare January 15, 2021 16:25
@neiljp neiljp added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jan 15, 2021
@prah23 prah23 force-pushed the send_typing_event branch from a18f80f to de929b5 Compare January 20, 2021 12:45
prah23 added 3 commits January 21, 2021 03:53
@prah23
refactor: buttons: Pass user_id to private_box_view in UserButton.
554f159 
Currently, the UserButton only passes itself as the `button` parameter
to the private_box_view() when pressed, on the users list. This leads to
an inconsistency as the corresponding user_id isn't passed. This commit
passes the user_id explicitly, to ensure availability of parameters
should there be any need inside the private_box_view in the future.
@prah23
model: Add function to send typing status.
fa1c598 
This commit adds a function to send typing events to the server. The
function works asynchronously to avoid a lag between keypress and the
corresponding letter's display on the screen.

Tests added.

The write_box() fixture function in test_boxes.py currently does not have
a to_write_box attribute which causes test failures for to_write_box
calls inside the keypress tests. This commit sets it to None to support
such calls.
@prah23
boxes: Configure private_box_view to send typing status.
1cb631f 
This commit captures the user's typing status using urwid's
connect_signal(), observing for changes in the msg_write_box. The
msg_write_box is connected to a function that sends 'start' events with
a limit on how frequently the functions can be called. Idleness is
tracked with a method which is initiated when a start event is sent
and checks for a possible halt. The corresponding wait periods after the
user starts and stops typing are used to restrict calling the function
excessively.

Fixes zulip#593.
@prah23 prah23 force-pushed the send_typing_event branch from de929b5 to 1cb631f Compare January 20, 2021 22:24
@neiljp neiljp mentioned this pull request Jan 21, 2021
Merged
@neiljp
Copy link
Collaborator

neiljp commented Jan 22, 2021

This is resolved via the slight adjustment of the key commits in #884, which is now merged.

@neiljp neiljp closed this Jan 22, 2021
@neiljp neiljp added this to the Next Release milestone Jan 22, 2021
@neiljp neiljp mentioned this pull request Jan 22, 2021
Open
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neiljp neiljp Awaiting requested review from neiljp

Assignees
No one assigned
Labels
PR needs review PR requires feedback to proceed size: XL [Automatic label added by zulipbot]
Projects
None yet
Milestone
0.6.0
Development

Successfully merging this pull request may close these issues.

Send typing event to server
3 participants
@prah23 @neiljp @zulipbot