Refresh room summaries when date or time changes in the device #3683
Conversation
|
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3683 +/- ##
===========================================
- Coverage 82.81% 82.79% -0.03%
===========================================
Files 1748 1749 +1
Lines 41777 41808 +31
Branches 5108 5113 +5
===========================================
+ Hits 34597 34614 +17
- Misses 5365 5379 +14
Partials 1815 1815 ☔ View full report in Codecov by Sentry. |
| } | ||
|
|
||
| @ContributesBinding(AppScope::class) | ||
| class DefaultDateTimeObserver @Inject constructor( |
There was a problem hiding this comment.
This need to be a singleton, else you are registering many BroadcastReceiver, and they are never unregistered.
There was a problem hiding this comment.
Nice catch, I completely forgot to add the @SingleIn annotation.
| Intent.ACTION_DATE_CHANGED -> changes.tryEmit(Event.DateChanged(lastTime, newDate)) | ||
| Intent.ACTION_TIME_CHANGED -> changes.tryEmit(Event.DateChanged(lastTime, newDate)) | ||
| } | ||
| lastTime = newDate |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -24,6 +24,7 @@ interface MutableDiffCache<E> : DiffCache<E> { | |||
| fun removeAt(index: Int): E? | |||
| fun add(index: Int, element: E?) | |||
| operator fun set(index: Int, element: E?) | |||
| fun clear() | |||
There was a problem hiding this comment.
It's a leftover from previous tests that I forgot to remove, thanks!
| @@ -25,5 +26,5 @@ object DateFormatterModule { | |||
| fun providesLocale(): Locale = Locale.getDefault() | |||
|
|
|||
| @Provides | |||
| fun providesTimezone(): TimeZone = TimeZone.currentSystemDefault() | |||
| fun providesTimezone(): TimezoneProvider = TimezoneProvider { TimeZone.currentSystemDefault() } | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
...list/impl/src/test/kotlin/io/element/android/features/roomlist/impl/RoomListPresenterTest.kt
Show resolved
Hide resolved
jmartinesp
force-pushed
the
fix/jme/add-datetime-observer-to-update-the-room-list
branch
from
e63d5cb
to
b3be170
Compare
jmartinesp
added
the
Run-Maestro
github-actions
bot
removed
the
Run-Maestro
|
jmartinesp
deleted the
fix/jme/add-datetime-observer-to-update-the-room-list
branch
Content
DateTimeObserver, which will register a receiver for date and time changes in the devices and emit updates.RoomListDataSourcelisten to those updates.TimezoneProviderinstead of injecting theTimezoneused when the app was opened.Alternative to #3650.
Motivation and context
There is a bug where in the room list you might be able to see rooms whose state hasn't changed since yesterday still displaying the time at which some event happened instead of 'Yesterday', making you think they happened at some time today and they're out of order.
If you open one of these rooms, the room list item is also refreshed and the right 'Yesterday' formatted timestamp is used.
The same happens with the time if you change the timezone of your device.
Screenshots / GIFs
They should be the same.
Tests
Tested devices
Checklist