Live Location Sharing - Attach location data to beacon info state event #5711
Conversation
onurays
requested review from
a team and
mnaturel
and removed request for
a team
| @@ -532,6 +546,49 @@ internal class EventRelationsAggregationProcessor @Inject constructor( | |||
| } | |||
| } | |||
|
|
|||
| private fun handleLiveLocation(realm: Realm, | |||
There was a problem hiding this comment.
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
EventRelationsAggregationProcessorimplements theLiveLocationAggregationProcessorinterface 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?
There was a problem hiding this comment.
Right, done. It is also easy to test now.
| } | ||
|
|
||
| // 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()) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh, nice catch! Done.
| private fun isBeaconInfoOutdated(beaconInfoContent: LiveLocationBeaconContent, | ||
| liveLocationContent: MessageLiveLocationContent): Boolean { | ||
| val beaconInfoStartTime = beaconInfoContent.getBestTimestampAsMilliseconds() ?: 0 | ||
| val liveLocationEventTime = liveLocationContent.getBestTs() ?: 0 |
There was a problem hiding this comment.
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().
|
|
||
| private fun isBeaconInfoOutdated(beaconInfoContent: LiveLocationBeaconContent, | ||
| liveLocationContent: MessageLiveLocationContent): Boolean { | ||
| val beaconInfoStartTime = beaconInfoContent.getBestTimestampAsMilliseconds() ?: 0 |
There was a problem hiding this comment.
I guess we should first check if the beacon is still live before checking the timeout, what do you think?
There was a problem hiding this comment.
Right, so we need to set live=false to stop sharing live location.
|
|
||
| // Update last location info of the beacon state event | ||
| beaconInfoContent.lastLocationContent = content | ||
| beaconInfoEntity.root?.content = ContentMapper.map(beaconInfoContent.toContent()) |
There was a problem hiding this comment.
Out of curiosity, I don't understand how the write into the database of the new value is done. Can you explain?
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| // 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()) |
There was a problem hiding this comment.
I guess we should check all values of the list of EventType.STATE_ROOM_BEACON_INFO in case the size of the list change in the future. Something like that:
var beaconInfoEntity: CurrentStateEventEntity? = null
val eventTypesIterator = EventType.STATE_ROOM_BEACON_INFO.iterator()
while (beaconInfoEntity == null && eventTypesIterator.hasNext()) {
beaconInfoEntity = CurrentStateEventEntity.getOrNull(realm, roomId, locationSenderId, eventTypesIterator.next())
}
First
m.beacon_infostate event is sent and thenm.beaconevent with location data is sent by referencing the state event. Aggregating beacon info state event with the last location will be robust for UI rendering.