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

Adjust typing indicator behaviour #9689

Merged
merged 7 commits into from Jun 7, 2023

Conversation

Antreesy
Copy link
Contributor

@Antreesy Antreesy commented Jun 2, 2023

☑️ Resolves

🏚️ Before 🏡 After
image image

After the first key stroke, we send first true signal start to watching after input value, if it was changed within intervals:

  • 0-10s: if something else was typed, after 10s from the start we send another true signal;
  • 10-20s: if still typing, after 20s from the start we send anothertrue signal;
  • ... and repeat for each 10s interval.

In that case, first possible true signal will be received in 0s + delay after actor started typing;
consequent signals will come every 10s + delay.


false signal will be received:

  • after actor sent message: in 0s + delay;
    false signal will be set locally by client:
  • after actor left chat: in 0s;
  • after actor stopped typing (input value wasn't changed in 10s interval): in 15-24s;

As typing person

  • Send "Typing" every 10 sec when there was a change - done with interval
  • Stop on leave room - already done on receiving end
  • Stop on send message - the only stop-signal to be sent

As receiver

  • Hide user from indicator after not receive "Typing" in the last 15 sec - done with store
  • Group sessions by actor type + actor id - should work as-is
  • Ignore self - done with store, adjusted in tests

🚧 Tasks

  • Code review
  • Behaviour check

🏁 Checklist

@Antreesy Antreesy added this to the 💜 Next Major (28) milestone Jun 2, 2023
@Antreesy Antreesy self-assigned this Jun 2, 2023
Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

✅ Looks gokd from the screenshot but didnt review the code and didnt test

Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

I haven't tested yet and haven't checked changes in webrtc and tests but there are some blocking comments.

src/components/NewMessage/NewMessageTypingIndicator.vue Outdated Show resolved Hide resolved
src/store/participantsStore.js Show resolved Hide resolved
src/store/participantsStore.js Show resolved Hide resolved
src/store/participantsStore.js Outdated Show resolved Hide resolved
src/components/NewMessage/NewMessage.vue Outdated Show resolved Hide resolved
src/utils/SignalingTypingHandler.js Outdated Show resolved Hide resolved
src/utils/SignalingTypingHandler.spec.js Outdated Show resolved Hide resolved
src/utils/webrtc/index.js Outdated Show resolved Hide resolved
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@Antreesy Antreesy force-pushed the fix/9624/adjust-typing-indicator-behaviour branch from cfa26e8 to 70d732c Compare June 7, 2023 15:02
…ests

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
…typing

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@Antreesy Antreesy force-pushed the fix/9624/adjust-typing-indicator-behaviour branch from 70d732c to 32003ea Compare June 7, 2023 15:20
@Antreesy Antreesy requested a review from ShGKme June 7, 2023 15:27
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
Copy link
Contributor

@SystemKeeper SystemKeeper left a comment

Choose a reason for hiding this comment

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

Tested locally and works as described.
Personally it feels a bit long until the indicator disappears, because when you type 2 characters, we are always sending the second "startedTyping" after 10 seconds.

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@ShGKme
Copy link
Contributor

ShGKme commented Jun 7, 2023

Personally it feels a bit long until the indicator disappears, because when you type 2 characters, we are always sending the second "startedTyping" after 10 seconds.

Yes, but this respects #9624

@ShGKme ShGKme force-pushed the fix/9624/adjust-typing-indicator-behaviour branch from 99a3ad2 to ff098a6 Compare June 7, 2023 18:18
@ShGKme
Copy link
Contributor

ShGKme commented Jun 7, 2023

For the record: my force push was with dummy commit and its drop.

GitHub had weird behavior, the new Maksim's commits appeared in the branch but not in the PR o_O

Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

Thank you for your great work!

@DorraJaouad
Copy link
Contributor

I checked the behavior:
When deleting, the typing indicator appears and disappears after a +10s delay. I also think that having a delay of >10s is a bit long. Maybe we should consider reducing it.

Recording.2023-06-07.223802.mp4

@Antreesy Antreesy merged commit 2f00bb3 into master Jun 7, 2023
19 checks passed
@Antreesy Antreesy deleted the fix/9624/adjust-typing-indicator-behaviour branch June 7, 2023 21:05
@ShGKme
Copy link
Contributor

ShGKme commented Jun 7, 2023

I checked the behavior:
When deleting, the typing indicator appears and disappears after a +10s delay. I also think that having a delay of >10s is a bit long. Maybe we should consider reducing it.

I think, users usually type for a longer time, so 10-25s doesn't usually looks as long as in this video.

Thought, it would be good if we have some way to change those values: #9717

Adding a small delay may also help with the situation on the video, then a user suddenly typed some symbols for a second: #9718

@DorraJaouad
Copy link
Contributor

I checked the behavior:
When deleting, the typing indicator appears and disappears after a +10s delay. I also think that having a delay of >10s is a bit long. Maybe we should consider reducing it.

I think, users usually type for a longer time, so 10-25s doesn't usually looks as long as in this video.

Thought, it would be good if we have some way to change those values: #9717

Adding a small delay may also help with the situation on the video, then a user suddenly typed some symbols for a second: #9718

What about the typing indicator when the user is deleting the text, is it considered accurate?

@ShGKme
Copy link
Contributor

ShGKme commented Jun 8, 2023

What about the typing indicator when the user is deleting the text, is it considered accurate?

Yes, erasing a text is also considered as typing

@ShGKme
Copy link
Contributor

ShGKme commented Jun 8, 2023

/backport to stable27

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants