Skip to content

Virtual scrolling for conversations list#10262

Merged
ShGKme merged 6 commits intomasterfrom
perf/virtual-scrolling-conversations-list
Aug 21, 2023
Merged

Virtual scrolling for conversations list#10262
ShGKme merged 6 commits intomasterfrom
perf/virtual-scrolling-conversations-list

Conversation

@ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Aug 18, 2023

PR is made on top of #10051

☑️ Resolves

Performance issues:

  1. Too many conversations avatars request:
    Opening a page with 500 conversations immediately makes 500 requests for conversation avatars
  2. Initial loading is slow for long chat lists
  3. Scrolling a long conversations list and interacting with the menu could be slow on a long conversations list

🖼️ Screenshots

Almost no visual changes, except scrolling behavior.

By default scroll to position in virtual scrolling just "teleport" to a new position instantly:

To the end To unread
instant instant-2

It doesn't look good and it is different from the current Talk behavior. And it makes it less understandable from what direction (up/down) the user have moved.

But we cannot just use smooth scrolling... Smooth scrolling from the beginning to the end means - loading ALL avatars in between and not very performant in general.

The idea is "imitating" the smooth scrolling:

  1. If the new position is far from the current - first instantly move to a distance of one visual list height from the target
  2. Then scroll with smooth just for a small number of elements
Scroll close Scroll a bit far Scroll from the end of 500
scroll-close scroll-far-1 scroll-far-2

Throw 500 chats scrolling is not super smooth but smooth a bit :D

🚧 Tasks

  • Choose and add a virtual scroller
  • Use a virtual scroller for the conversations list
    • Change scrolling and unread mentions handling with the virtual list
  • Use <img> with implicit key instead of <NcAvatar> for conversations
    • Without a key on <img> there is a glitch effect on scrolling because Vue updates src of the images. Even though images are cached after the first load, the glitch effect is still visible.
    • It is still a problem for 1-1
With glitching Without glitching
with-glitch no-glitch

📝 Notes and drawbacks

No virtual scrolling in the search results

A virtual scroller is used ONLY for the main conversations list (incl. filtered conversations) and NOT for the search results. The search results are more complicated because it is not a homogeneous list but several lists with different elements in between.

Glitching effect with 1-1 conversations avatars

To avoid it we need to stop using <NcAvatar> for 1-1 (what is problematic with the user status) or add key to <NcAvatar> and make it more performant on props updating.

👀 Alternative solutions

If we only want to fix the first problem with avatar fetching, we can just implement lazy loading for conversation avatars.

🏁 Checklist

@ShGKme ShGKme added this to the 💙 Next Major (28) milestone Aug 18, 2023
@ShGKme ShGKme self-assigned this Aug 18, 2023
@ShGKme ShGKme force-pushed the perf/virtual-scrolling-conversations-list branch from 95ea979 to 472e534 Compare August 18, 2023 15:55
@ShGKme
Copy link
Contributor Author

ShGKme commented Aug 18, 2023

  • Rebased onto refactoring PR
  • Fixed according to the review - linting, unreadNum variable
  • Fixed regression with not showing joined conversation during loading

@ShGKme
Copy link
Contributor Author

ShGKme commented Aug 18, 2023

/backport to stable27

@ShGKme ShGKme force-pushed the perf/virtual-scrolling-conversations-list branch from 472e534 to 4bff57f Compare August 18, 2023 18:18
<li ref="container" class="left-sidebar__list">
<ul class="scroller h-100">
<!-- Conversations List -->
<template v-if="!isSearching">
Copy link
Contributor

Choose a reason for hiding this comment

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

UPD: using v-show and keeping scroller in DOM helps with keeping the scrol position

Copy link
Contributor

Choose a reason for hiding this comment

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

Still thinking, that it's an issue: try scroll to the bottom and use search from there

@ShGKme ShGKme force-pushed the perf/virtual-scrolling-conversations-list branch from 4bff57f to b93a1b4 Compare August 21, 2023 13:33
@ShGKme
Copy link
Contributor Author

ShGKme commented Aug 21, 2023

Rebased onto master with new cachedConversations feature.

See commits from fixup! remove unused code:

  • Fix linting errors and remove unused code
  • Fix scrolling behavior
  • Fix some issues with abort search and scrolling to newly selected conversations

@Antreesy please, have a look.

@ShGKme
Copy link
Contributor Author

ShGKme commented Aug 21, 2023

Join a conversation from search update:

Before After
before after

Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

Tested, better now
Let's rebase onto master and squash, and think again, if v-show is applicable

<li ref="container" class="left-sidebar__list">
<ul class="scroller h-100">
<!-- Conversations List -->
<template v-if="!isSearching">
Copy link
Contributor

Choose a reason for hiding this comment

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

Still thinking, that it's an issue: try scroll to the bottom and use search from there

@ShGKme
Copy link
Contributor Author

ShGKme commented Aug 21, 2023

Though, having an ability to switch between list and search with v-show/keepalive was one of the goal of the refactoring PR with separating listing and search, I'd not mix this feature with virtual scrolling.

If we use v-show, we will increase rendering size twice during the search.

Using KeepAlive could be better, though, it's very limited in Vue 2. It can only work with dinamyc component, not just any content.

@Antreesy
Copy link
Contributor

I'd not mix this feature with virtual scrolling.

Then let's consider it for the follow-up

@nickvergessen
Copy link
Member

So time for the fixup rebase?

Signed-off-by: Grigorii K. Shartsev <[email protected]>
@ShGKme ShGKme force-pushed the perf/virtual-scrolling-conversations-list branch 2 times, most recently from 33734f5 to bcbc30e Compare August 21, 2023 15:15
@ShGKme ShGKme force-pushed the perf/virtual-scrolling-conversations-list branch 2 times, most recently from 01d6a3a to 02d5fd6 Compare August 21, 2023 15:43
ShGKme added 2 commits August 21, 2023 17:48
`vue-virtual-scroller` requires `key` on `<img>` to avoid glitch on scroll

Signed-off-by: Grigorii K. Shartsev <[email protected]>
@ShGKme ShGKme force-pushed the perf/virtual-scrolling-conversations-list branch from 02d5fd6 to 8552005 Compare August 21, 2023 15:49
@ShGKme
Copy link
Contributor Author

ShGKme commented Aug 21, 2023

Forced pushed with squashed commits and very minor changes like typos in comments

@ShGKme ShGKme enabled auto-merge August 21, 2023 15:52
@ShGKme ShGKme merged commit 687f54a into master Aug 21, 2023
@ShGKme ShGKme deleted the perf/virtual-scrolling-conversations-list branch August 21, 2023 16:46
@backportbot-nextcloud
Copy link

The backport to stable27 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable27
git pull origin stable27

# Create the new backport branch
git checkout -b fix/foo-stable27

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123

# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable27

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants