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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fade out user cursor labels after five seconds of inactivity #4336

Merged
merged 3 commits into from Jul 28, 2023

Conversation

mejo-
Copy link
Member

@mejo- mejo- commented Jun 21, 2023

馃摑 Summary

Fade out user cursor labels after inactivity.

Implementation details:

  • Add a custom CollaborationCursor Tiptap extension
  • Add CSS to fade out the cursor label after some time.
  • Listen for Yjs updates.
    • If it's a doc change by ourself, update the timestamp of own user in
      awareness state.
  • If it's a remote awareness update, add back the CSS class to the
    corresponding cursor.
  • Wait 50ms before showing the cursor in the DOM to account for cases
    where the cursor gets re-rendered by y-prosemirror.

Fixes: #4126

馃弫 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
  • Documentation is not required

@mejo- mejo- added enhancement New feature or request 2. developing labels Jun 21, 2023
@mejo- mejo- added bug Something isn't working and removed enhancement New feature or request labels Jun 21, 2023
@cypress
Copy link

cypress bot commented Jun 21, 2023

Passing run #11351 鈫楋笌

0 149 2 0 Flakiness 0

Details:

Fade out user cursor labels after five seconds of inactivity
Project: Text Commit: 7332d992b1
Status: Passed Duration: 03:27 馃挕
Started: Jul 28, 2023 6:40 AM Ended: Jul 28, 2023 6:43 AM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

src/helpers/yjs.js Outdated Show resolved Hide resolved
Copy link
Member

@juliushaertl juliushaertl left a comment

Choose a reason for hiding this comment

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

Generally the change looks good to me, animation behaviour seems very stable and looks nice. However updating the timestamp on the awareness state seems to cause two push requests for typing an individual character (one for the awareness, one for the step). Maybe we can combine them in some way?

@max-nextcloud max-nextcloud requested review from juliushaertl and removed request for max-nextcloud July 27, 2023 14:19
@max-nextcloud
Copy link
Collaborator

max-nextcloud commented Jul 27, 2023

Generally the change looks good to me, animation behaviour seems very stable and looks nice. However updating the timestamp on the awareness state seems to cause two push requests for typing an individual character (one for the awareness, one for the step). Maybe we can combine them in some way?

We're now waiting 50ms before sending out the first push. This way the awareness update will be part of it.

@@ -165,7 +162,7 @@ class SyncService {
this.#sendIntervalId = null
this._sendSteps(getSendable).then(resolve).catch(reject)
}
}, 200)
}, 50)
Copy link
Member

Choose a reason for hiding this comment

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

This could have an impact on the resource demand due to increased push requests on the server. I think the PR itself should also be fine with the existing 200ms. Any reason to decrease that specific to the cursor hiding?

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 a bit tricky as we change the initial waiting. Before if there was no request pending we would just send it immediately - so the 200 ms had no effect at all. Now we also delay this initial message.

I remember that 200ms are roughly what the human brain starts to notice as a delay. But that is only if one is waiting for a direct response from the UI. This will only delay the sync between different parties. So 200 ms are probably okay there as well.

I'd say we go with the 200 ms and if we notice usability issues with it we can change it to 50 ms on the initial request and 200 when another request is pending.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay... so i gave this a try locally with 200 and noticed that the polling delay still is the main delay one notices. If I type fairly fast 200ms will result in two keystrokes getting combined in one push request. That seems totally reasonable. If this actually reduces the number of requests send - even better.

Copy link
Member

@juliushaertl juliushaertl left a comment

Choose a reason for hiding this comment

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

Tested and works, one question regarding reducing the push interval inline

Implementation details:
* Add a custom CollaborationCursor Tiptap extension
* Add CSS to fade out the cursor label after some time.
* Listen for Yjs updates.
  - If it's a doc change by ourself, update the timestamp of own user in
    awareness state.
  - If it's a remote awareness update, add back the CSS class to the
    corresponding cursor.
  - Wait 50ms before showing the cursor in the DOM to account for cases
    where the cursor gets re-rendered by y-prosemirror.

Fixes: #4126

Signed-off-by: Jonas <jonas@freesources.org>
@max-nextcloud
Copy link
Collaborator

/compile

@max-nextcloud
Copy link
Collaborator

Funny that lint passes even though we introduced a warning.
I think the workflow probably was updated from a template.

@max-nextcloud
Copy link
Collaborator

/compile

Document changes trigger awareness updates.
Wait before sending them in a push request so they can be combined.

Also make use of prosemirrors transactions to detect own changes
instead of listening to yjs updates.

Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud
Copy link
Collaborator

/compile

@max-nextcloud
Copy link
Collaborator

/backport b7baceb,928fe3527a7467c78612e7db43fe35dead65819a to stable27

@max-nextcloud
Copy link
Collaborator

backport b7baceb,928fe3527a7467c78612e7db43fe35dead65819a to stable26

Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@max-nextcloud
Copy link
Collaborator

I'm not sure about the backports. 27 is pretty clear to me also as this does not break any api and 28 will still be quite some time into the future. But 26 I don't know.

@max-nextcloud max-nextcloud merged commit a573d74 into main Jul 28, 2023
32 checks passed
@max-nextcloud max-nextcloud deleted the enh/hide_cursors branch July 28, 2023 06:55
@mejo-
Copy link
Member Author

mejo- commented Jul 28, 2023

I think we discussed this already and decided to backport this to stable26 as well, because it fixes an annoying usability bug.

@max-nextcloud
Copy link
Collaborator

/backport b7baceb,928fe3527a7467c78612e7db43fe35dead65819a to stable26

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hide user cursors
4 participants