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

Add expire_ts compatibility to matrixRTC #4032

Merged
merged 10 commits into from
Jan 29, 2024
Merged

Conversation

toger5
Copy link
Contributor

@toger5 toger5 commented Jan 24, 2024

Currenlty we use a combination of two fields in the Call member state events (created_ts + expires)
We want to move to just use a expire_ts.
This needs rethinking the logic behind syncing clients clocks.
created_ts was mirring the origin_server_timestamp on update.
But expire_ts can only be known when the clock sync is known.

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

Here's what your changelog entry will look like:

✨ Features

  • Add expire_ts compatibility to matrixRTC (#4032). Contributed by @toger5.

Signed-off-by: Timo K <[email protected]>
Copy link

@fkwp fkwp left a comment

Choose a reason for hiding this comment

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

lgtm

const relativeCreationTime = this.parentEvent.getTs() - this.createdTs();
if (this.data.expires_ts) {
// With expires_ts we cannot convert to local time.
// TODO: Check the server timestamp and compute a diff to local time.
Copy link

Choose a reason for hiding this comment

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

do we already have a follow-up ticket for that TODO?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed I refactored this to use its own class for tracking the server/client diff with unimplmeneted clock sync diffs.
(It actally computes the diff once we get an event with an age property and from that point on its using this offset)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The refactor is now on a different branch so that we can merge this "low risk"/"lightweight" version.

We will do the server-client-time sync properly here: #4038

@@ -624,6 +624,9 @@ export class MatrixRTCSession extends TypedEventEmitter<MatrixRTCSessionEvent, M
};

if (prevMembership) m.created_ts = prevMembership.createdTs();
if (m.created_ts) m.expires_ts = m.created_ts + (m.expires ?? 0);
// TODO: Date.now() should be the origin_server_ts (now).
Copy link

Choose a reason for hiding this comment

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

do we have the follow-up ticket for that TODO already?

@toger5 toger5 enabled auto-merge January 29, 2024 14:42
@toger5 toger5 added this pull request to the merge queue Jan 29, 2024
Merged via the queue into develop with commit 99600e8 Jan 29, 2024
23 checks passed
@toger5 toger5 deleted the toger5/matrix-rtc-expires_at branch January 29, 2024 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants