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

Refresh room summaries when date or time changes in the device #3683

Merged
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ import io.element.android.libraries.matrix.api.core.RoomId
import io.element.android.libraries.matrix.api.encryption.EncryptionService
import io.element.android.libraries.matrix.api.encryption.RecoveryState
import io.element.android.libraries.matrix.api.roomlist.RoomList
import io.element.android.libraries.matrix.api.sync.SyncService
import io.element.android.libraries.matrix.api.timeline.ReceiptType
import io.element.android.libraries.preferences.api.store.SessionPreferencesStore
import io.element.android.libraries.push.api.notifications.NotificationCleaner
Expand Down Expand Up @@ -93,7 +92,6 @@ class RoomListPresenter @Inject constructor(
private val logoutPresenter: Presenter<DirectLogoutState>,
) : Presenter<RoomListState> {
private val encryptionService: EncryptionService = client.encryptionService()
private val syncService: SyncService = client.syncService()

@Composable
override fun present(): RoomListState {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ package io.element.android.features.roomlist.impl.datasource
import io.element.android.features.roomlist.impl.model.RoomListRoomSummary
import io.element.android.libraries.androidutils.diff.DiffCacheUpdater
import io.element.android.libraries.androidutils.diff.MutableListDiffCache
import io.element.android.libraries.androidutils.system.DateTimeObserver
import io.element.android.libraries.core.coroutine.CoroutineDispatchers
import io.element.android.libraries.matrix.api.core.RoomId
import io.element.android.libraries.matrix.api.notificationsettings.NotificationSettingsService
Expand All @@ -36,9 +37,11 @@ class RoomListDataSource @Inject constructor(
private val coroutineDispatchers: CoroutineDispatchers,
private val notificationSettingsService: NotificationSettingsService,
private val appScope: CoroutineScope,
private val dateTimeObserver: DateTimeObserver,
) {
init {
observeNotificationSettings()
observeDateTimeChanges()
}

private val _allRooms = MutableSharedFlow<ImmutableList<RoomListRoomSummary>>(replay = 1)
Expand Down Expand Up @@ -77,16 +80,31 @@ class RoomListDataSource @Inject constructor(
.launchIn(appScope)
}

private fun observeDateTimeChanges() {
dateTimeObserver.changes
.onEach { event ->
when (event) {
is DateTimeObserver.Event.TimeZoneChanged -> rebuildAllRoomSummaries()
is DateTimeObserver.Event.DateChanged -> rebuildAllRoomSummaries()
}
}
.launchIn(appScope)
}

private suspend fun replaceWith(roomSummaries: List<RoomSummary>) = withContext(coroutineDispatchers.computation) {
lock.withLock {
diffCacheUpdater.updateWith(roomSummaries)
buildAndEmitAllRooms(roomSummaries)
}
}

private suspend fun buildAndEmitAllRooms(roomSummaries: List<RoomSummary>) {
private suspend fun buildAndEmitAllRooms(roomSummaries: List<RoomSummary>, useCache: Boolean = true) {
val roomListRoomSummaries = diffCache.indices().mapNotNull { index ->
diffCache.get(index) ?: buildAndCacheItem(roomSummaries, index)
if (useCache) {
diffCache.get(index) ?: buildAndCacheItem(roomSummaries, index)
} else {
buildAndCacheItem(roomSummaries, index)
}
}
_allRooms.emit(roomListRoomSummaries.toImmutableList())
}
Expand All @@ -96,4 +114,12 @@ class RoomListDataSource @Inject constructor(
diffCache[index] = roomListSummary
return roomListSummary
}

private suspend fun rebuildAllRoomSummaries() {
lock.withLock {
roomListService.allRooms.summaries.replayCache.firstOrNull()?.let { roomSummaries ->
buildAndEmitAllRooms(roomSummaries, useCache = false)
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import io.element.android.features.roomlist.impl.model.createRoomListRoomSummary
import io.element.android.features.roomlist.impl.search.RoomListSearchEvents
import io.element.android.features.roomlist.impl.search.RoomListSearchState
import io.element.android.features.roomlist.impl.search.aRoomListSearchState
import io.element.android.libraries.androidutils.system.DateTimeObserver
import io.element.android.libraries.architecture.Presenter
import io.element.android.libraries.dateformatter.api.LastMessageTimestampFormatter
import io.element.android.libraries.dateformatter.test.A_FORMATTED_DATE
Expand Down Expand Up @@ -83,6 +84,7 @@ import io.element.android.tests.testutils.lambda.value
import io.element.android.tests.testutils.test
import io.element.android.tests.testutils.testCoroutineDispatchers
import kotlinx.coroutines.ExperimentalCoroutinesApi
import kotlinx.coroutines.flow.MutableSharedFlow
import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.test.TestScope
import kotlinx.coroutines.test.advanceTimeBy
Expand Down Expand Up @@ -655,7 +657,8 @@ class RoomListPresenterTest {
),
coroutineDispatchers = testCoroutineDispatchers(),
notificationSettingsService = client.notificationSettingsService(),
appScope = backgroundScope
appScope = backgroundScope,
dateTimeObserver = FakeDateTimeObserver(),
),
featureFlagService = featureFlagService,
indicatorService = DefaultIndicatorService(
Expand All @@ -672,3 +675,11 @@ class RoomListPresenterTest {
logoutPresenter = { aDirectLogoutState() },
)
}

class FakeDateTimeObserver : DateTimeObserver {
override val changes = MutableSharedFlow<DateTimeObserver.Event>(extraBufferCapacity = 1)

fun given(event: DateTimeObserver.Event) {
bmarty marked this conversation as resolved.
Show resolved Hide resolved
changes.tryEmit(event)
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
/*
* Copyright 2024 New Vector Ltd.
*
* SPDX-License-Identifier: AGPL-3.0-only
* Please see LICENSE in the repository root for full details.
*/

package io.element.android.features.roomlist.impl.datasource

import app.cash.turbine.test
import com.google.common.truth.Truth.assertThat
import io.element.android.features.roomlist.impl.FakeDateTimeObserver
import io.element.android.libraries.androidutils.system.DateTimeObserver
import io.element.android.libraries.core.coroutine.CoroutineDispatchers
import io.element.android.libraries.dateformatter.api.LastMessageTimestampFormatter
import io.element.android.libraries.eventformatter.api.RoomLastMessageFormatter
import io.element.android.libraries.matrix.api.roomlist.RoomListService
import io.element.android.libraries.matrix.test.notificationsettings.FakeNotificationSettingsService
import io.element.android.libraries.matrix.test.room.aRoomSummary
import io.element.android.libraries.matrix.test.roomlist.FakeRoomListService
import io.element.android.tests.testutils.testCoroutineDispatchers
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.test.TestScope
import kotlinx.coroutines.test.runTest
import org.junit.Test
import java.time.Instant

class RoomListDataSourceTest {
@Test
fun `when DateTimeObserver gets a date change, the room summaries are refreshed`() = runTest {
val roomListService = FakeRoomListService().apply {
postState(RoomListService.State.Running)
postAllRooms(listOf(aRoomSummary()))
}
val dateTimeObserver = FakeDateTimeObserver()
val roomListDataSource = createRoomListDataSource(roomListService = roomListService, dateTimeObserver = dateTimeObserver)

roomListDataSource.allRooms.test {
// Observe room list items changes
roomListDataSource.launchIn(backgroundScope)

// Get the initial room list
val initialRoomList = awaitItem()
assertThat(initialRoomList).isNotEmpty()

// Trigger a date change
dateTimeObserver.given(DateTimeObserver.Event.DateChanged(Instant.MIN, Instant.now()))

// Check there is a new list and it's not the same as the previous one (although it has the same content)
val newRoomList = awaitItem()
assertThat(newRoomList).isNotSameInstanceAs(initialRoomList)

cancelAndIgnoreRemainingEvents()
}
}

@Test
fun `when DateTimeObserver gets a time zone change, the room summaries are refreshed`() = runTest {
val roomListService = FakeRoomListService().apply {
postState(RoomListService.State.Running)
postAllRooms(listOf(aRoomSummary()))
}
val dateTimeObserver = FakeDateTimeObserver()
val roomListDataSource = createRoomListDataSource(roomListService = roomListService, dateTimeObserver = dateTimeObserver)

roomListDataSource.allRooms.test {
// Observe room list items changes
roomListDataSource.launchIn(backgroundScope)

// Get the initial room list
val initialRoomList = awaitItem()
assertThat(initialRoomList).isNotEmpty()

// Trigger a timezone change
dateTimeObserver.given(DateTimeObserver.Event.TimeZoneChanged)

// Check there is a new list and it's not the same as the previous one (although it has the same content)
val newRoomList = awaitItem()
assertThat(newRoomList).isNotSameInstanceAs(initialRoomList)

cancelAndIgnoreRemainingEvents()
}
}

private fun TestScope.createRoomListDataSource(
roomListService: FakeRoomListService = FakeRoomListService(),
roomListRoomSummaryFactory: RoomListRoomSummaryFactory =
RoomListRoomSummaryFactory(
lastMessageTimestampFormatter = LastMessageTimestampFormatter { _ -> "Yesterday" },
roomLastMessageFormatter = RoomLastMessageFormatter { _, _ -> "Hey" }
),
dispatchers: CoroutineDispatchers = testCoroutineDispatchers(),
notificationSettingsService: FakeNotificationSettingsService = FakeNotificationSettingsService(),
appScope: CoroutineScope = backgroundScope,
dateTimeObserver: FakeDateTimeObserver = FakeDateTimeObserver(),
) = RoomListDataSource(
roomListService = roomListService,
roomListRoomSummaryFactory = roomListRoomSummaryFactory,
coroutineDispatchers = dispatchers,
notificationSettingsService = notificationSettingsService,
appScope = appScope,
dateTimeObserver = dateTimeObserver,
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/*
* Copyright 2024 New Vector Ltd.
*
* SPDX-License-Identifier: AGPL-3.0-only
* Please see LICENSE in the repository root for full details.
*/

package io.element.android.libraries.androidutils.system

import android.content.BroadcastReceiver
import android.content.Context
import android.content.Intent
import android.content.IntentFilter
import com.squareup.anvil.annotations.ContributesBinding
import io.element.android.libraries.androidutils.system.DateTimeObserver.Event
import io.element.android.libraries.di.AppScope
import io.element.android.libraries.di.ApplicationContext
import io.element.android.libraries.di.SingleIn
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.MutableSharedFlow
import java.time.Instant
import javax.inject.Inject

interface DateTimeObserver {
val changes: Flow<Event>

sealed interface Event {
data object TimeZoneChanged : Event
data class DateChanged(val previous: Instant, val new: Instant) : Event
}
}

@ContributesBinding(AppScope::class)
@SingleIn(AppScope::class)
class DefaultDateTimeObserver @Inject constructor(

Check warning on line 35 in libraries/androidutils/src/main/kotlin/io/element/android/libraries/androidutils/system/DateTimeObserver.kt

View check run for this annotation

Codecov / codecov/patch

libraries/androidutils/src/main/kotlin/io/element/android/libraries/androidutils/system/DateTimeObserver.kt#L35

Added line #L35 was not covered by tests
Copy link
Member

Choose a reason for hiding this comment

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

This need to be a singleton, else you are registering many BroadcastReceiver, and they are never unregistered.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch, I completely forgot to add the @SingleIn annotation.

@ApplicationContext context: Context
) : DateTimeObserver {
private val dateTimeReceiver = object : BroadcastReceiver() {
private var lastTime = Instant.now()

Check warning on line 39 in libraries/androidutils/src/main/kotlin/io/element/android/libraries/androidutils/system/DateTimeObserver.kt

View check run for this annotation

Codecov / codecov/patch

libraries/androidutils/src/main/kotlin/io/element/android/libraries/androidutils/system/DateTimeObserver.kt#L38-L39

Added lines #L38 - L39 were not covered by tests

override fun onReceive(context: Context, intent: Intent) {
val newDate = Instant.now()

Check warning on line 42 in libraries/androidutils/src/main/kotlin/io/element/android/libraries/androidutils/system/DateTimeObserver.kt

View check run for this annotation

Codecov / codecov/patch

libraries/androidutils/src/main/kotlin/io/element/android/libraries/androidutils/system/DateTimeObserver.kt#L42

Added line #L42 was not covered by tests
when (intent.action) {
Intent.ACTION_TIMEZONE_CHANGED -> changes.tryEmit(Event.TimeZoneChanged)
Intent.ACTION_DATE_CHANGED -> changes.tryEmit(Event.DateChanged(lastTime, newDate))
Intent.ACTION_TIME_CHANGED -> changes.tryEmit(Event.DateChanged(lastTime, newDate))

Check warning on line 46 in libraries/androidutils/src/main/kotlin/io/element/android/libraries/androidutils/system/DateTimeObserver.kt

View check run for this annotation

Codecov / codecov/patch

libraries/androidutils/src/main/kotlin/io/element/android/libraries/androidutils/system/DateTimeObserver.kt#L44-L46

Added lines #L44 - L46 were not covered by tests
}
lastTime = newDate

Check warning on line 48 in libraries/androidutils/src/main/kotlin/io/element/android/libraries/androidutils/system/DateTimeObserver.kt

View check run for this annotation

Codecov / codecov/patch

libraries/androidutils/src/main/kotlin/io/element/android/libraries/androidutils/system/DateTimeObserver.kt#L48

Added line #L48 was not covered by tests
Copy link
Member

Choose a reason for hiding this comment

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

Maybe update lastTime only for the cases Intent.ACTION_DATE_CHANGED and Intent.ACTION_TIME_CHANGED? But actually previous and new does not seem to be used.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added them in case we wanted to generate new timestamps only if >= 1 day had elapsed between them, but there's not really a need for that now that I think about it... they might not be worth keeping.

}
}

override val changes = MutableSharedFlow<Event>(extraBufferCapacity = 10)

Check warning on line 52 in libraries/androidutils/src/main/kotlin/io/element/android/libraries/androidutils/system/DateTimeObserver.kt

View check run for this annotation

Codecov / codecov/patch

libraries/androidutils/src/main/kotlin/io/element/android/libraries/androidutils/system/DateTimeObserver.kt#L52

Added line #L52 was not covered by tests

init {
context.registerReceiver(dateTimeReceiver, IntentFilter().apply {
addAction(Intent.ACTION_TIMEZONE_CHANGED)
addAction(Intent.ACTION_DATE_CHANGED)
addAction(Intent.ACTION_TIME_CHANGED)
})

Check warning on line 59 in libraries/androidutils/src/main/kotlin/io/element/android/libraries/androidutils/system/DateTimeObserver.kt

View check run for this annotation

Codecov / codecov/patch

libraries/androidutils/src/main/kotlin/io/element/android/libraries/androidutils/system/DateTimeObserver.kt#L54-L59

Added lines #L54 - L59 were not covered by tests
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,6 @@

package io.element.android.libraries.dateformatter.api

interface LastMessageTimestampFormatter {
fun interface LastMessageTimestampFormatter {
fun format(timestamp: Long?): String
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import android.text.format.DateFormat
import android.text.format.DateUtils
import kotlinx.datetime.Clock
import kotlinx.datetime.LocalDateTime
import kotlinx.datetime.TimeZone
import kotlinx.datetime.toInstant
import kotlinx.datetime.toJavaLocalDate
import kotlinx.datetime.toJavaLocalDateTime
Expand All @@ -25,7 +24,7 @@ import kotlin.math.absoluteValue
class DateFormatters @Inject constructor(
private val locale: Locale,
private val clock: Clock,
private val timeZone: TimeZone,
private val timeZoneProvider: TimezoneProvider,
) {
private val onlyTimeFormatter: DateTimeFormatter by lazy {
DateTimeFormatter.ofLocalizedTime(FormatStyle.SHORT).withLocale(locale)
Expand Down Expand Up @@ -70,7 +69,7 @@ class DateFormatters @Inject constructor(
return if (period.years.absoluteValue >= 1) {
formatDateWithYear(dateToFormat)
} else if (useRelative && period.days.absoluteValue < 2 && period.months.absoluteValue < 1) {
getRelativeDay(dateToFormat.toInstant(timeZone).toEpochMilliseconds())
getRelativeDay(dateToFormat.toInstant(timeZoneProvider.provide()).toEpochMilliseconds())
} else {
formatDateWithMonth(dateToFormat)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,21 +10,20 @@ package io.element.android.libraries.dateformatter.impl
import kotlinx.datetime.Clock
import kotlinx.datetime.Instant
import kotlinx.datetime.LocalDateTime
import kotlinx.datetime.TimeZone
import kotlinx.datetime.toLocalDateTime
import javax.inject.Inject

class LocalDateTimeProvider @Inject constructor(
private val clock: Clock,
private val timezone: TimeZone,
private val timezoneProvider: TimezoneProvider,
) {
fun providesNow(): LocalDateTime {
val now: Instant = clock.now()
return now.toLocalDateTime(timezone)
return now.toLocalDateTime(timezoneProvider.provide())
}

fun providesFromTimestamp(timestamp: Long): LocalDateTime {
val tsInstant = Instant.fromEpochMilliseconds(timestamp)
return tsInstant.toLocalDateTime(timezone)
return tsInstant.toLocalDateTime(timezoneProvider.provide())
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/*
* Copyright 2024 New Vector Ltd.
*
* SPDX-License-Identifier: AGPL-3.0-only
* Please see LICENSE in the repository root for full details.
*/

package io.element.android.libraries.dateformatter.impl

import kotlinx.datetime.TimeZone

fun interface TimezoneProvider {
fun provide(): TimeZone
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import com.squareup.anvil.annotations.ContributesTo
import dagger.Module
import dagger.Provides
import io.element.android.libraries.dateformatter.impl.TimezoneProvider
import io.element.android.libraries.di.AppScope
import kotlinx.datetime.Clock
import kotlinx.datetime.TimeZone
Expand All @@ -25,5 +26,5 @@
fun providesLocale(): Locale = Locale.getDefault()

@Provides
fun providesTimezone(): TimeZone = TimeZone.currentSystemDefault()
fun providesTimezone(): TimezoneProvider = TimezoneProvider { TimeZone.currentSystemDefault() }

Check warning on line 29 in libraries/dateformatter/impl/src/main/kotlin/io/element/android/libraries/dateformatter/impl/di/DateFormatterModule.kt

View check run for this annotation

Codecov / codecov/patch

libraries/dateformatter/impl/src/main/kotlin/io/element/android/libraries/dateformatter/impl/di/DateFormatterModule.kt#L29

Added line #L29 was not covered by tests
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering if requesting TimeZone.currentSystemDefault() for all Events could come at a cost. We could maybe have a flow of TimeZone that emit new item when the timezone change?

Copy link
Member Author

Choose a reason for hiding this comment

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

There might be some tiny overhead from instantiating the value, but I don't think it's noticeable at all. The underlying code is cloning an existing value AFAICT, then just wrapping it in a new object. Reusing the observer for this feels like an overkill, to be honest.

}
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ class DefaultLastMessageTimestampFormatterTest {
val now = "1980-04-06T18:35:24.00Z"
val dat = "1979-04-06T18:35:24.00Z"
val clock = FakeClock().apply { givenInstant(Instant.parse(now)) }
val dateFormatters = DateFormatters(Locale.US, clock, TimeZone.UTC)
val dateFormatters = DateFormatters(Locale.US, clock) { TimeZone.UTC }
assertThat(dateFormatters.formatDateWithFullFormat(Instant.parse(dat).toLocalDateTime(TimeZone.UTC))).isEqualTo("Friday, April 6, 1979")
}

Expand All @@ -102,8 +102,8 @@ class DefaultLastMessageTimestampFormatterTest {
*/
private fun createFormatter(@Suppress("SameParameterValue") currentDate: String): LastMessageTimestampFormatter {
val clock = FakeClock().apply { givenInstant(Instant.parse(currentDate)) }
val localDateTimeProvider = LocalDateTimeProvider(clock, TimeZone.UTC)
val dateFormatters = DateFormatters(Locale.US, clock, TimeZone.UTC)
val localDateTimeProvider = LocalDateTimeProvider(clock) { TimeZone.UTC }
val dateFormatters = DateFormatters(Locale.US, clock) { TimeZone.UTC }
return DefaultLastMessageTimestampFormatter(localDateTimeProvider, dateFormatters)
}
}
Loading
Loading