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.

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

Typing Indicator Animation #990

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Rohitth007
Copy link
Collaborator

@Rohitth007 Rohitth007 commented Apr 9, 2021

This commit animates the typing feedback using an iterable cycle
object. This is done using an asynchronous function
_show_typing_animation and a boolean is_recipient_typing to
start and end the animation.

The corner case of not recieving a stop signal is handled by
setting TYPING_STARTED_EXPIRY_PERIOD=15 and checking this
condition using datetime.

This has to be handled differently from #915 as footer keep updating every half second
so, footer text's duration variable can't be used.

Tests added.

animate_typing

@zulipbot zulipbot added the size: S [Automatic label added by zulipbot] label Apr 9, 2021
Copy link
Member

@preetmishra preetmishra left a comment

Choose a reason for hiding this comment

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

@Rohitth007 Thanks for putting this forward! While this is a small addition, this does seem to improve the experience.

I have tested this locally and it seems to work well. 👍

Other than the in-line comments, there are a few things that be improved:

  • We follow a particular convention for the commit messages. You can read the commit guidelines that we have in the README and also $ git log --oneline. I would recommend to you install gitlint.
  • We would appreciate some tests to go along with the feature. Tests give us the ability to ship new features confidently.
  • We use mypy for static type checking. Please add type annotation wherever needed.

Feel free to drop a message in #zulip-terminal if you need any assistance with anything.

@Rohitth007
Copy link
Collaborator Author

@preetmishra Yes, I am aware of the things specified that I have not added yet. This is just a functional prototype and since it's small enough to understand, I was planning to write a proper commit structure after making those changes. I am in between my exams right now hence the not so perfect PR. Hope you understand :)

@preetmishra
Copy link
Member

@Rohitth007 No worries. Best of luck with the exams!

@zulipbot zulipbot added size: XL [Automatic label added by zulipbot] and removed size: S [Automatic label added by zulipbot] labels Apr 11, 2021
@Rohitth007
Copy link
Collaborator Author

@preetmishra PR updated for review

@Rohitth007 Rohitth007 changed the title [WIP] ui: model: Typing Animation ui: model: Typing Animation Apr 12, 2021
Copy link
Member

@preetmishra preetmishra left a comment

Choose a reason for hiding this comment

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

@Rohitth007 Hey, thanks for your work! I have given quick feedback on the approach. Let's discuss it before moving ahead for another review iteration.

@preetmishra preetmishra added the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Apr 18, 2021
@Rohitth007
Copy link
Collaborator Author

@preetmishra I have made a few more changes to handle corner cases better. Even the tests work better now. (I have moved time forward instead of reducing it)
It all works but PEP8 tells me that there is a trailing whitespace error on a line I haven't even touched.
I even tried using VSCode's Trim Trailing Whitespace command.
zulipterminal/model.py:960:68: W291 trailing whitespace

@Rohitth007
Copy link
Collaborator Author

@zulipbot add "PR needs review"
@zulipbot remove "PR awaiting update"

@zulipbot zulipbot 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 Apr 19, 2021
@zee-bit
Copy link
Member

zee-bit commented Apr 19, 2021

It all works but PEP8 tells me that there is a trailing whitespace error on a line I haven't even touched.

You did change that line. You can take a look at the Files changed tab of this PR and notice that line 960 is changed. You may be in a subsequent commit while running the linter and thought that you didn't change that line. 😅
(That's why it's important to run tests for each commit so that you find your mistakes in that commit itself.)

PRO-TIP: git blame can prove really helpful in finding out who changed a particular line in a repository.

@Rohitth007
Copy link
Collaborator Author

Rohitth007 commented Apr 19, 2021

@zee-bit Oh wait I was checking in a different file 😅 , I mistook model.py for test_model.py cause that was the only file I was editing at that time. 🤦
Yes, I use something similar to git blame 👍

Copy link
Member

@Ezio-Sarthak Ezio-Sarthak left a comment

Choose a reason for hiding this comment

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

Thanks for continuing work on this! Leaving some minor code-wise comments 👍

The commit title could be improved a bit, something more specific?

@Rohitth007 Rohitth007 force-pushed the typing-animation branch 2 times, most recently from 14cb442 to 6ecf4bc Compare June 5, 2021 16:52
@Rohitth007 Rohitth007 requested a review from preetmishra June 17, 2021 12:36
@Rohitth007 Rohitth007 added the PR ready to be merged PR has been reviewed & is ready to be merged label Jun 17, 2021
@Rohitth007 Rohitth007 changed the title ui: model: Typing Animation ui: model: Typing Indicator Animation Jun 17, 2021
Copy link
Member

@preetmishra preetmishra left a comment

Choose a reason for hiding this comment

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

@Rohitth007 Thanks for the improvements! This seems to work well locally. 👍

The implementation looks good! I have left a few in-line comments. We could have another test for last_updated_time case.

@Rohitth007
Copy link
Collaborator Author

@preetmishra This is ready for another review.

@Rohitth007 Rohitth007 requested a review from preetmishra June 19, 2021 15:01
@Rohitth007 Rohitth007 force-pushed the typing-animation branch 19 times, most recently from c9333ce to 09e485d Compare August 19, 2021 13:25
@zulipbot zulipbot added size: M [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Aug 19, 2021
The corner case of not receiving a stop signal is handled by
setting TYPING_STARTED_EXPIRY_PERIOD=15 and checking this
condition using datetime and comparing it with
`typing_start_time` state of `active_conversation_info`.

`typing_start_time` is used so that we don't miss a start
event (which is received every 10 seconds) and misinterpret it as
stop not received.

`active_conversation_info` is stored in a variable so that it
doesn't change dynamically while inside the loop.

Tests added.
The `narrow` state in active_conversation_info is used so as to end
the notification if the user switches narrow by comparing with
`model.narrow`.

Tests added.
@zulipbot zulipbot added size: L [Automatic label added by zulipbot] and removed size: M [Automatic label added by zulipbot] labels Aug 19, 2021
@zulipbot
Copy link
Member

Heads up @Rohitth007, we just merged some commits that conflict with the changes you 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: typing notifications has conflicts PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback size: L [Automatic label added by zulipbot]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants