saving sync filter changed #7627
Conversation
|
|
||
| package org.matrix.android.sdk.api.session.sync.filter | ||
|
|
||
| internal data class SyncFilterParams( |
There was a problem hiding this comment.
if it's internal it should be moved to the internal package.
| listOfSupportedEventTypes = params.listOfSupportedEventTypes.toRealmList(), | ||
| listOfSupportedEventTypesHasSet = params.listOfSupportedEventTypes != null, | ||
| listOfSupportedStateEventTypes = params.listOfSupportedStateEventTypes.toRealmList(), | ||
| listOfSupportedStateEventTypesHasSet = params.listOfSupportedEventTypes != null, |
There was a problem hiding this comment.
Copy paste bug here:
| listOfSupportedStateEventTypesHasSet = params.listOfSupportedEventTypes != null, | |
| listOfSupportedStateEventTypesHasSet = params.listOfSupportedStateEventTypes != null, |
| var lazyLoadMembersForMessageEvents: Boolean? = null, | ||
| var useThreadNotifications: Boolean? = null, | ||
| var listOfSupportedEventTypes: RealmList<String>? = null, | ||
| var listOfSupportedEventTypesHasSet: Boolean = false, |
There was a problem hiding this comment.
Maybe rename to
| var listOfSupportedEventTypesHasSet: Boolean = false, | |
| var listOfSupportedEventTypesHasBeenSet: Boolean = false, |
| var listOfSupportedEventTypes: RealmList<String>? = null, | ||
| var listOfSupportedEventTypesHasSet: Boolean = false, | ||
| var listOfSupportedStateEventTypes: RealmList<String>? = null, | ||
| var listOfSupportedStateEventTypesHasSet: Boolean = false, |
There was a problem hiding this comment.
Maybe rename to
| var listOfSupportedStateEventTypesHasSet: Boolean = false, | |
| var listOfSupportedStateEventTypesHasBeenSet: Boolean = false, |
| return monarchy.awaitTransaction { realm -> | ||
| realm.where<SyncFilterParamsEntity>().findFirst()?.let { | ||
| val params = FilterParamsMapper().map(it) | ||
| params |
There was a problem hiding this comment.
maybe simplify those two lines to
FilterParamsMapper().map(it)
(will also remove the indentation issue)
|
|
||
| override suspend fun storeFilterParams(params: SyncFilterParams) { | ||
| return monarchy.awaitTransaction { realm -> | ||
| val entity = FilterParamsMapper().map(params) |
| saveFilterTask | ||
| .configureWith(SaveFilterTask.Params(filterPreset)) | ||
| .executeBy(taskExecutor) | ||
| override fun setSyncFilter(filterBuilder: SyncFilterBuilder) { |
There was a problem hiding this comment.
Maybe take the opportunity to make this fun suspendable?
| filterRepository.storeSyncFilter( | ||
| filter = filter, | ||
| filterId = filterResponse.filterId, | ||
| roomEventFilter = FilterFactory.createDefaultRoomFilter() |
There was a problem hiding this comment.
It's weird to call createDefaultRoomFilter. Can you explain please?
There was a problem hiding this comment.
There is currently no difference between ElementRoomFilter and DefaultRoomFilter so basically there is no need to introduce any options here. It's also looks like we could utilise Filter.room.timeline filter for the purpose of roomEventFilter, which will mean that we don't need this separate filter at all. But currently no one can say for sure if we could/should do so or no, so for now I decided to do as little change to this area as possible, since the task was mostly about SyncFilter. I just wanted to remove referencing Element from the sdk, since there was no real difference between Default and Element room filters
| ) | ||
| } | ||
|
|
||
| internal fun build(homeServerCapabilities: HomeServerCapabilities): Filter { |
There was a problem hiding this comment.
I understand the logic, but from an app developer, it's weird to provide a builder without calling build().
So maybe refactor a bit and create a SyncFilterParam with a SyncFilterParamBuilder in the API.
then we can have an internal SyncFilterBuilder which takes a SyncFilterParam and build with HomeServerCapabilities, retrieved using an injected HomeServerCapabilitiesDataSource in the constructor.
WDYT?
There was a problem hiding this comment.
I also had doubts making it internal, but then I decided that introducing extra entities could make it more difficult to understand the flow and maintain it in the future. I don't have any strong opinion on that though, more like personal preference, so if you think it will be better to separate to two builders - I'll do the change
| ) | ||
|
|
||
| @ExperimentalCoroutinesApi | ||
| class DefaultGetCurrentFilterTaskTest { |
There was a problem hiding this comment.
Please rename the file to match this class name.
copyright fixed for sdk
| filter = filterBuilder.build(homeServerCapabilities) | ||
| ) | ||
| ) | ||
| .executeBy(taskExecutor) |
There was a problem hiding this comment.
Since you are in a suspend fun, I think you do not need to use the taskExecutor (it can be removed from the constructor).
Just call
saveFilterTask.execute(
SaveFilterTask.Params(
filter = filterBuilder.build(homeServerCapabilities)
)
)Should be fine
# Conflicts: # matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/migration/MigrateSessionTo043.kt
# Conflicts: # matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/migration/MigrateSessionTo044.kt
|
SonarCloud Quality Gate failed. |
Type of change
Content
Currently sync filter doesn't take in account home server capability, so we can't add params which depends on homeserver version like
unread_thread_notifications, also we need to update filter when those capabilities changing and so we need to store filter parameters to be able to build filter with new capabilities and compare with stored oneMotivation and context
closes #7626