Pinned messages list : hide reactions #3430
Conversation
|
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3430 +/- ##
========================================
Coverage 82.61% 82.61%
========================================
Files 1699 1700 +1
Lines 39989 40003 +14
Branches 4866 4868 +2
========================================
+ Hits 33036 33050 +14
Misses 5233 5233
Partials 1720 1720 ☔ View full report in Codecov by Sentry. |
| private val dispatchers: CoroutineDispatchers, | ||
| private val eventItemFactory: TimelineItemEventFactory, | ||
| private val virtualItemFactory: TimelineItemVirtualFactory, | ||
| private val timelineItemGrouper: TimelineItemGrouper, | ||
| private val timelineItemIndexer: TimelineItemIndexer, | ||
| ) { | ||
| @AssistedFactory | ||
| interface Creator { | ||
| fun create(context: TimelineItemsFactoryContext): TimelineItemsFactory |
There was a problem hiding this comment.
We now have creator of factories :)
| * @param computeReadReceipts when false, read receipts will be empty. | ||
| * @param computeReactions when false, reactions will be empty. | ||
| */ | ||
| data class TimelineItemsFactoryContext( |
There was a problem hiding this comment.
Maybe rename to TimelineItemsFactoryParams? Context is sort of a reserved word in Android dev...
There was a problem hiding this comment.
I find Params a bit weird here, do you have other ideas? I wasn't sure how to name it..
There was a problem hiding this comment.
I can only think of either Params or Config.
There was a problem hiding this comment.
Do we need to have a wrapper data class?
Can't we just put the 2 members computeReadReceipts: Boolean and computeReactions: Boolean as parameters of the factory?
Also when trying to implement this ^, I found the line lateinit var context: TimelineItemsFactoryContext a bit not nice... We should pass the parameter to the TimelineItemEventFactory.create method
There was a problem hiding this comment.
It's done on init, so it's not that bad, although it's true a refactor could easily break it. The alternative might be creating a TimelineItemsFactoryContextBuilder and I think none of us want to reach that level of design pattern overload 😅 .
There was a problem hiding this comment.
I don't like creating several bools for config, it should be in one wrapper IMO.
So either I pass the wrapper to the create function, but it's a bit stupid because it won't change overtime, either I also use an AssistedFactory 😅
There was a problem hiding this comment.
Yes, to be clean we will need an assisted factory.
There was a problem hiding this comment.
LGTM, but I agree with @bmarty in some renaming for the Context class.
| @@ -55,7 +56,7 @@ import kotlin.time.Duration.Companion.milliseconds | |||
| class PinnedMessagesListPresenter @AssistedInject constructor( | |||
| @Assisted private val navigator: PinnedMessagesListNavigator, | |||
| private val room: MatrixRoom, | |||
| private val timelineItemsFactory: TimelineItemsFactory, | |||
| timelineItemsFactoryCreator: TimelineItemsFactory.Creator, | |||
There was a problem hiding this comment.
TimelineItemsFactory.Factory was a bit redundant, I guess 😅 .
There was a problem hiding this comment.
Yeah, same, wasn't sure of the name 😅
ganfra
force-pushed
the
feature/fga/pinned_messages_list_remove_reaction
branch
from
cab8860
to
73bbd1e
Compare
ganfra
added
the
Run-Maestro
github-actions
bot
removed
the
Run-Maestro
|
|
Maestro is still failing at different places, including ANR and issue with the creation flow. I merged as it's not related to this PR. |
Content
Allow disabling computation of reaction and read receipts from the timelineItemFactory.
Motivation and context
Reactions should be hidden in the pinned messages list.
Screenshots / GIFs
Tests
Tested devices
Checklist