Cache hasAvatar info in sessionStorage to avoid flicker #1457
Conversation
|
Any thoughts on this approach ? @ma12-co @nickvergessen @skjnldsv If we are fine caching this information, where would be a better place ? Ideally that info should be available to other components as well like the VideoBackground in the Talk app which uses another separate query to get the avatar to put into the background in blur mode. |
src/components/Avatar/Avatar.vue
Outdated
| @@ -466,17 +466,35 @@ export default { | |||
| urlGenerator(this.user, this.size * 4) + ' 4x', | |||
| ].join(', ') | |||
|
|
|||
| // skip loading | |||
| if (!window.userHasAvatar) { | |||
There was a problem hiding this comment.
Use browserstorage instead? Also helps cross tabs?
There was a problem hiding this comment.
Yes, we are cleaning the window over the last nc releases, we should try our best not to pollute the window namespace anymore :)
There was a problem hiding this comment.
if we were to store information available for all apps, where would that be ?
in the past it used to be in the OC namespace
There was a problem hiding this comment.
browserstorage is good enough I think, or localstorage if you want 🤷
There was a problem hiding this comment.
ok sounds good.
will likely also need some form of expiration, but I'm not sure if worth it as the use case would be that someone deletes their avatar and do not set a new one, which is rather rare
There was a problem hiding this comment.
I think I'll go with sessionStorage. It only lives within the current page and should be enough for the use case: preventing flickering within a call session.
Then it's easier to tell people to force refresh the page than clear cache whenever they noticed that an avatar of a collegue did not get updated.
PVince81
force-pushed
the
poc-cache-hasavatar
branch
from
0dcc162
to
090b666
Compare
|
Ready to review. Note: I went for the format "userHasAvatar-" + userId instead of a single object to avoid repeated serialization/deserialization (also semi-concurrently) |
|
Weird, I just checked the browser console and somehow nothing appears under sessionStorage. and also somehow I didn't see flickering any more. I don't understand how it can be working. investigating... |
|
ok got it... |
skjnldsv
added
3. to review
Improve performance by saving repeated HTTP calls when retrieving information whether a user has an avatar set. Note that even HTTP calls that hit the cache cause a delay. This is useful to avoid flickering when switching/re-rendering vue components that render avatars. Signed-off-by: Vincent Petry <vincent@nextcloud.com>
PVince81
force-pushed
the
poc-cache-hasavatar
branch
from
090b666
to
15c7212
Compare
|
works nicely with Chrome, but in Firefox I still observe a flicker sometimes when hitting "stop following", despite cache hit. let's still move forward with this as it at least helps resolve the annoying flicker on speaker change, which is already a big step forward |
PVince81
changed the title
skjnldsv
added
2. developing
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
|
adjusted in 7613d44 I decided against introduce a service for just this once single occurrence, considering that there are no services in nextcloud-vue. If this becomes commonplace later on we could adjust. Please also check nextcloud/spreed#4379 (last commit) which uses the same storage location for the video background. |
PVince81
added
3. to review
|
good to merge ? 😄 |
skjnldsv
requested review from
raimund-schluessler,
marcoambrosini and
korelstar
This is a crude POC that shows that when we save the "does this user has an avatar call", performance improves.
Possible solution for nextcloud/spreed#4284
Maybe this should be stored in a store or something, but as I understand, Vue components in the library don't really have a store available.
Another approach would be to supply an optional property "hasAvatar" and the caching could be done from the outside in situations where the caller thinks it makes sense.