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

fix: tiles not updated after reconnection #7780

Merged

Conversation

Rash419
Copy link
Contributor

@Rash419 Rash419 commented Dec 5, 2023

from idle

The following scenario was affected:

  • user A and B both goes idle
  • user B starts typing
  • user A reconnects, all tiles updated by user B were not getting reflected for user A

Mismatch of canonical ids was causing the issue. On reconnection, tilecombine messages were sent before uno:ChangeTheme can update the canonical id on server side, therefore the received tile cache from server were from old canonical id(

TileCombined tileCombined = TileCombined::parse(tokens);
).

By reseting the previousTheme on connection we make sure there after canonicalidchange we invalidate all the tiles

Change-Id: I874bfe0bd71d176bacf0c7aa768e49613535ebd5

  • Resolves: #
  • Target version: master

Summary

TODO

  • ...

Checklist

  • Code is properly formatted
  • All commits have Change-Id
  • I have run tests with make check
  • I have issued make run and manually verified that everything looks okay
  • Documentation (manuals or wiki) has been updated or is not required

from idle

The following scenario was affected:
- user A and B both goes idle
- user B starts typing
- user A reconnects, all tiles updated by user B were not getting
  reflected for user A

Mismatch of canonical ids was causing the issue. On reconnection,
tilecombine messages were sent before uno:ChangeTheme can update the
canonical id on server side, therefore the received tile cache from server were
from old canonical id https://github.com/CollaboraOnline/online/blob/4a8974d107896c7fb4b7c35e326b52a986144a8b/wsd/ClientSession.cpp#L1379
https://github.com/CollaboraOnline/online/blob/4a8974d107896c7fb4b7c35e326b52a986144a8b/wsd/TileCache.cpp#L482

By reseting the previousTheme on connection we make sure there after
canonicalidchange we invalidate all the tiles

Signed-off-by: Rash419 <rashesh.padia@collabora.com>
Change-Id: I874bfe0bd71d176bacf0c7aa768e49613535ebd5
Signed-off-by: Rash419 <rashesh.padia@collabora.com>
@Rash419 Rash419 force-pushed the tiles-not-updated-on-reconnect branch from c0f726c to db57387 Compare December 5, 2023 06:44
@Rash419 Rash419 requested a review from caolanm December 5, 2023 07:32
@@ -1557,6 +1557,7 @@ app.definitions.Socket = L.Class.extend({
this._map._isCursorVisible = false;

this._map._docLayer._resetCanonicalIdStatus();
this._map.uiManager.previousTheme = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

If _resetCanonicalIdStatus sets this._canonicalIdInitialized to false I then see that we have:

if (!this._canonicalIdInitialized) {
this._canonicalIdInitialized = true;
this._update();
} else if (viewRenderedState !== this._map.uiManager.previousTheme) {
this._requestNewTiles();
this._invalidateAllPreviews();
this.redraw();
}

inside else if (textMsg.startsWith('canonicalidchange:'))

does that mean we get two canonicalidchange messages on a reconnect?, one setting this._canonicalIdInitialized back to true, and then a second one which triggers the else this._requestNewTiles ... ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we get two canonicalidchange message when we load/reconnect

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might be interesting
#7324

Copy link
Contributor

@caolanm caolanm left a comment

Choose a reason for hiding this comment

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

Yeah, so debugged a bit more and I see that this works and isn't harmful.

I think there's an underlying oddness a viewid 1000 which I think is the reason we get stale tiles back and I'll have a look at a later follow up on that thought.

@caolanm caolanm merged commit 4ea31bf into CollaboraOnline:master Dec 13, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants