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.

Sign up for GitHub

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

Jump to bottom

Live Location Sharing - Attach location data to beacon info state event #5711

Merged
merged 8 commits into from
Apr 11, 2022
Merged

Live Location Sharing - Attach location data to beacon info state event #5711

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
aae281a
Support aggregation of live location beacon events.
Apr 6, 2022
197b542
Check if live location data is valid.
Apr 6, 2022
faa0751
Update last location content of beacon info state event.
Apr 6, 2022
6708ed8
Lint fixes.
Apr 6, 2022
44c6d88
Changelog added.
Apr 6, 2022
15e1c7b
Code review fixes.
Apr 8, 2022
28f4838
Code review fixes.
Apr 8, 2022
1c5cf6b
Code review fixes.
Apr 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/5711.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Live Location Sharing - Attach location data to beacon info state event
8 changes: 7 additions & 1 deletion ...a/org/matrix/android/sdk/api/session/room/model/livelocation/LiveLocationBeaconContent.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import com.squareup.moshi.Json
import com.squareup.moshi.JsonClass
import org.matrix.android.sdk.api.session.room.model.message.LocationAsset
import org.matrix.android.sdk.api.session.room.model.message.LocationAssetType
import org.matrix.android.sdk.api.session.room.model.message.MessageLiveLocationContent

@JsonClass(generateAdapter = true)
data class LiveLocationBeaconContent(
Expand All @@ -37,7 +38,12 @@ data class LiveLocationBeaconContent(
* Live location asset type.
*/
@Json(name = "org.matrix.msc3488.asset") val unstableLocationAsset: LocationAsset = LocationAsset(LocationAssetType.SELF),
@Json(name = "m.asset") val locationAsset: LocationAsset? = null
@Json(name = "m.asset") val locationAsset: LocationAsset? = null,

/**
* Client side tracking of the last location
*/
var lastLocationContent: MessageLiveLocationContent? = null
) {

fun getBestBeaconInfo() = beaconInfo ?: unstableBeaconInfo
Expand Down
83 changes: 70 additions & 13 deletions ...n/java/org/matrix/android/sdk/internal/session/room/EventRelationsAggregationProcessor.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,10 @@ import org.matrix.android.sdk.api.session.room.model.PowerLevelsContent
import org.matrix.android.sdk.api.session.room.model.ReferencesAggregatedContent
import org.matrix.android.sdk.api.session.room.model.VoteInfo
import org.matrix.android.sdk.api.session.room.model.VoteSummary
import org.matrix.android.sdk.api.session.room.model.livelocation.LiveLocationBeaconContent
import org.matrix.android.sdk.api.session.room.model.message.MessageContent
import org.matrix.android.sdk.api.session.room.model.message.MessageEndPollContent
import org.matrix.android.sdk.api.session.room.model.message.MessageLiveLocationContent
import org.matrix.android.sdk.api.session.room.model.message.MessagePollContent
import org.matrix.android.sdk.api.session.room.model.message.MessagePollResponseContent
import org.matrix.android.sdk.api.session.room.model.message.MessageRelationContent
Expand All @@ -47,6 +49,7 @@ import org.matrix.android.sdk.internal.crypto.verification.toState
import org.matrix.android.sdk.internal.database.helper.findRootThreadEvent
import org.matrix.android.sdk.internal.database.mapper.ContentMapper
import org.matrix.android.sdk.internal.database.mapper.EventMapper
import org.matrix.android.sdk.internal.database.model.CurrentStateEventEntity
import org.matrix.android.sdk.internal.database.model.EditAggregatedSummaryEntity
import org.matrix.android.sdk.internal.database.model.EditionOfEvent
import org.matrix.android.sdk.internal.database.model.EventAnnotationsSummaryEntity
Expand All @@ -60,6 +63,7 @@ import org.matrix.android.sdk.internal.database.model.TimelineEventEntity
import org.matrix.android.sdk.internal.database.model.TimelineEventEntityFields
import org.matrix.android.sdk.internal.database.query.create
import org.matrix.android.sdk.internal.database.query.getOrCreate
import org.matrix.android.sdk.internal.database.query.getOrNull
import org.matrix.android.sdk.internal.database.query.where
import org.matrix.android.sdk.internal.di.SessionId
import org.matrix.android.sdk.internal.di.UserId
Expand Down Expand Up @@ -88,7 +92,7 @@ internal class EventRelationsAggregationProcessor @Inject constructor(
// EventType.KEY_VERIFICATION_READY,
EventType.KEY_VERIFICATION_KEY,
EventType.ENCRYPTED
) + EventType.POLL_START + EventType.POLL_RESPONSE + EventType.POLL_END
) + EventType.POLL_START + EventType.POLL_RESPONSE + EventType.POLL_END + EventType.BEACON_LOCATION_DATA

override fun shouldProcess(eventId: String, eventType: String, insertType: EventInsertType): Boolean {
return allowedTypes.contains(eventType)
Expand All @@ -103,12 +107,12 @@ internal class EventRelationsAggregationProcessor @Inject constructor(
}
val isLocalEcho = LocalEcho.isLocalEchoId(event.eventId ?: "")
when (event.type) {
EventType.REACTION -> {
EventType.REACTION -> {
// we got a reaction!!
Timber.v("###REACTION in room $roomId , reaction eventID ${event.eventId}")
handleReaction(realm, event, roomId, isLocalEcho)
}
EventType.MESSAGE -> {
EventType.MESSAGE -> {
if (event.unsignedData?.relations?.annotations != null) {
Timber.v("###REACTION Aggregation in room $roomId for event ${event.eventId}")
handleInitialAggregatedRelations(realm, event, roomId, event.unsignedData.relations.annotations)
Expand All @@ -134,7 +138,7 @@ internal class EventRelationsAggregationProcessor @Inject constructor(
EventType.KEY_VERIFICATION_START,
EventType.KEY_VERIFICATION_MAC,
EventType.KEY_VERIFICATION_READY,
EventType.KEY_VERIFICATION_KEY -> {
EventType.KEY_VERIFICATION_KEY -> {
Timber.v("## SAS REF in room $roomId for event ${event.eventId}")
event.content.toModel<MessageRelationContent>()?.relatesTo?.let {
if (it.type == RelationType.REFERENCE && it.eventId != null) {
Expand All @@ -143,7 +147,7 @@ internal class EventRelationsAggregationProcessor @Inject constructor(
}
}

EventType.ENCRYPTED -> {
EventType.ENCRYPTED -> {
// Relation type is in clear
val encryptedEventContent = event.content.toModel<EncryptedEventContent>()
if (encryptedEventContent?.relatesTo?.type == RelationType.REPLACE ||
Expand All @@ -169,22 +173,27 @@ internal class EventRelationsAggregationProcessor @Inject constructor(
EventType.KEY_VERIFICATION_START,
EventType.KEY_VERIFICATION_MAC,
EventType.KEY_VERIFICATION_READY,
EventType.KEY_VERIFICATION_KEY -> {
EventType.KEY_VERIFICATION_KEY -> {
Timber.v("## SAS REF in room $roomId for event ${event.eventId}")
encryptedEventContent.relatesTo.eventId?.let {
handleVerification(realm, event, roomId, isLocalEcho, it)
}
}
in EventType.POLL_RESPONSE -> {
in EventType.POLL_RESPONSE -> {
event.getClearContent().toModel<MessagePollResponseContent>(catchError = true)?.let {
handleResponse(realm, event, it, roomId, isLocalEcho, event.getRelationContent()?.eventId)
}
}
in EventType.POLL_END -> {
in EventType.POLL_END -> {
event.content.toModel<MessageEndPollContent>(catchError = true)?.let {
handleEndPoll(realm, event, it, roomId, isLocalEcho)
}
}
in EventType.BEACON_LOCATION_DATA -> {
event.content.toModel<MessageLiveLocationContent>(catchError = true)?.let {
handleLiveLocation(realm, event, it, roomId, isLocalEcho)
}
}
}
} else if (encryptedEventContent?.relatesTo?.type == RelationType.ANNOTATION) {
// Reaction
Expand All @@ -205,7 +214,7 @@ internal class EventRelationsAggregationProcessor @Inject constructor(
// }
// }
}
EventType.REDACTION -> {
EventType.REDACTION -> {
val eventToPrune = event.redacts?.let { EventEntity.where(realm, eventId = it).findFirst() }
?: return
when (eventToPrune.type) {
Expand All @@ -225,25 +234,30 @@ internal class EventRelationsAggregationProcessor @Inject constructor(
}
}
}
in EventType.POLL_START -> {
in EventType.POLL_START -> {
val content: MessagePollContent? = event.content.toModel()
if (content?.relatesTo?.type == RelationType.REPLACE) {
Timber.v("###REPLACE in room $roomId for event ${event.eventId}")
// A replace!
handleReplace(realm, event, content, roomId, isLocalEcho)
}
}
in EventType.POLL_RESPONSE -> {
in EventType.POLL_RESPONSE -> {
event.content.toModel<MessagePollResponseContent>(catchError = true)?.let {
handleResponse(realm, event, it, roomId, isLocalEcho)
}
}
in EventType.POLL_END -> {
in EventType.POLL_END -> {
event.content.toModel<MessageEndPollContent>(catchError = true)?.let {
handleEndPoll(realm, event, it, roomId, isLocalEcho)
}
}
else -> Timber.v("UnHandled event ${event.eventId}")
in EventType.BEACON_LOCATION_DATA -> {
event.content.toModel<MessageLiveLocationContent>(catchError = true)?.let {
handleLiveLocation(realm, event, it, roomId, isLocalEcho)
}
}
else -> Timber.v("UnHandled event ${event.eventId}")
}
} catch (t: Throwable) {
Timber.e(t, "## Should not happen ")
Expand Down Expand Up @@ -532,6 +546,49 @@ internal class EventRelationsAggregationProcessor @Inject constructor(
}
}

private fun handleLiveLocation(realm: Realm,
Copy link
Contributor

Choose a reason for hiding this comment

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

In my opininion, we clearly need to stop adding code to this class. It is too big. To separate easily the code, I propose using delegation pattern. The idea is:

  • create a dedicated package named aggregation.livelocation.
  • In this package, create an interface as follow:
interface LiveLocationAggregationProcessor {
    fun handleLiveLocation(realm: Realm,
                           event: Event,
                           content: MessageLiveLocationContent,
                           roomId: String,
                           isLocalEcho: Boolean)
}
  • In the same package create the class to implement the aggregation (moved from EventRelationsAggregationProcessor)
  • Finally make EventRelationsAggregationProcessor implements the LiveLocationAggregationProcessor interface using delegation:
EventRelationsAggregationProcessor(...) : LiveLocationAggregationProcessor by DefaultLiveLocationAggregationProcessor() {...}

This way we have a class dedicated to aggregating a single event type and it is easier to add unit tests on it.
What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, done. It is also easy to test now.

event: Event,
content: MessageLiveLocationContent,
roomId: String,
isLocalEcho: Boolean) {
val locationSenderId = event.senderId ?: return

// We shouldn't process local echos
if (isLocalEcho) {
return
}

// A beacon info state event has to be sent before sending location
val beaconInfoEntity = CurrentStateEventEntity.getOrNull(realm, roomId, locationSenderId, EventType.STATE_ROOM_BEACON_INFO.first())
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to check for both the stable and unstable prefixes for compaibility reason? And using first(), we are not sure to use the correct prefix since the list may change over time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, nice catch! Done.

if (beaconInfoEntity == null) {
Timber.v("## LIVE LOCATION. There is not any beacon info which should be emitted before sending location updates")
return
}
val beaconInfoContent = ContentMapper.map(beaconInfoEntity.root?.content)?.toModel<LiveLocationBeaconContent>(catchError = true)
if (beaconInfoContent == null) {
Timber.v("## LIVE LOCATION. Beacon info content is invalid")
return
}

// Check if beacon info is outdated
if (isBeaconInfoOutdated(beaconInfoContent, content)) {
Timber.v("## LIVE LOCATION. Beacon info content is invalid")
return
}

// Update last location info of the beacon state event
beaconInfoContent.lastLocationContent = content
beaconInfoEntity.root?.content = ContentMapper.map(beaconInfoContent.toContent())
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, I don't understand how the write into the database of the new value is done. Can you explain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I debugged on TimelineEventViewModel.init() like room.getStateEvent(EventType.STATE_ROOM_BEACON_INFO.first()).content. I don't want to block your work in timeline for now. But I am going to create integration tests next.

}

private fun isBeaconInfoOutdated(beaconInfoContent: LiveLocationBeaconContent,
liveLocationContent: MessageLiveLocationContent): Boolean {
val beaconInfoStartTime = beaconInfoContent.getBestTimestampAsMilliseconds() ?: 0
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we should first check if the beacon is still live before checking the timeout, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, so we need to set live=false to stop sharing live location.

val liveLocationEventTime = liveLocationContent.getBestTs() ?: 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to do some renamings on MessageLiveLocationContent for the timestamps fields and the method getBestTs() to precise the unit and not use abreviation? For the method we would have getBestTimestampAsMilliseconds().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

val timeout = beaconInfoContent.getBestBeaconInfo()?.timeout ?: 0
return liveLocationEventTime - beaconInfoStartTime > timeout
}

private fun handleInitialAggregatedRelations(realm: Realm,
event: Event,
roomId: String,
Expand Down