-
Notifications
You must be signed in to change notification settings - Fork 739
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
Threads P0 Release #4746
Threads P0 Release #4746
Conversation
- Create new thread & reply to thread
** Important while this feature depends on local echo, should be added local echo support in threads to work 100%
- Renamed RoomDetailFragment to TimelineFragment while it should be reused from threads - View root thread message in room functionality
Add user friendly message thread summary on the SDK side Fix not encrypted rooms thread summaries
UI Improvements on threads summary Add View In Room bottom sheet action from within thread timeline root message
Add thread creation from photos, files, images, audios, replies etc Add slide animation to view threads from thread list Add click listener on room summary to handle cases like ( there is an image and the user upon click should view the image instead of navigating to the thread, so he can click the thread summary )
Add animation to fragment transition with offset for recyclerview initialization Support threads on deleted events
…hunk - Local notification mentioning system - Fix/Improve thread list filtering
# Conflicts: # library/ui-styles/src/main/res/values/dimens.xml # matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/events/model/RelationType.kt # matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/RealmSessionStoreMigration.kt # matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/model/EventEntity.kt # matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/send/LocalEchoEventFactory.kt # vector/src/main/java/im/vector/app/core/di/FragmentModule.kt # vector/src/main/java/im/vector/app/core/di/ScreenComponent.kt # vector/src/main/java/im/vector/app/features/command/Command.kt # vector/src/main/java/im/vector/app/features/home/room/detail/RoomDetailActivity.kt # vector/src/main/java/im/vector/app/features/home/room/detail/RoomDetailViewModel.kt # vector/src/main/java/im/vector/app/features/home/room/detail/TimelineFragment.kt # vector/src/main/java/im/vector/app/features/home/room/detail/composer/MessageComposerViewEvents.kt # vector/src/main/java/im/vector/app/features/home/room/detail/composer/MessageComposerViewModel.kt # vector/src/main/java/im/vector/app/features/home/room/detail/composer/MessageComposerViewState.kt # vector/src/main/java/im/vector/app/features/home/room/detail/timeline/action/MessageActionsViewModel.kt # vector/src/main/java/im/vector/app/features/home/room/detail/timeline/factory/MessageItemFactory.kt # vector/src/main/java/im/vector/app/features/navigation/DefaultNavigator.kt # vector/src/main/java/im/vector/app/features/navigation/Navigator.kt # vector/src/main/java/im/vector/app/features/notifications/NotificationUtils.kt # vector/src/main/java/im/vector/app/features/permalink/PermalinkHandler.kt # vector/src/main/res/layout/fragment_room_detail.xml
- Disable thread awareness when threads are enabled
# Conflicts: # matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/model/relation/RelationService.kt # tools/check/forbidden_strings_in_code.txt # vector/build.gradle # vector/src/main/java/im/vector/app/core/di/FragmentModule.kt # vector/src/main/java/im/vector/app/core/resources/UserPreferencesProvider.kt # vector/src/main/java/im/vector/app/features/command/Command.kt # vector/src/main/java/im/vector/app/features/command/CommandParser.kt # vector/src/main/java/im/vector/app/features/home/room/detail/RoomDetailViewState.kt # vector/src/main/java/im/vector/app/features/home/room/detail/TimelineFragment.kt # vector/src/main/java/im/vector/app/features/home/room/detail/TimelineViewModel.kt # vector/src/main/java/im/vector/app/features/home/room/detail/composer/MessageComposerViewModel.kt # vector/src/main/java/im/vector/app/features/home/room/detail/search/SearchResultItem.kt # vector/src/main/java/im/vector/app/features/home/room/detail/timeline/TimelineEventController.kt # vector/src/main/java/im/vector/app/features/home/room/detail/timeline/factory/MessageItemFactory.kt # vector/src/main/java/im/vector/app/features/navigation/Navigator.kt # vector/src/main/java/im/vector/app/features/notifications/NotificationUtils.kt # vector/src/main/java/im/vector/app/features/settings/VectorPreferences.kt # vector/src/main/res/layout/fragment_timeline.xml # vector/src/main/res/xml/vector_settings_labs.xml
- Update a text resource
val indexOfPrefetchForward = DEFAULT_PREFETCH_THRESHOLD.coerceAtMost(size - 1) | ||
val indexOfPrefetchForward = DEFAULT_PREFETCH_THRESHOLD | ||
.coerceAtMost(size - 1) | ||
.coerceAtLeast(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just on the form: coerceIn(minimumValue: Int, maximumValue: Int)
exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, let's use that, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bmarty Actually, coerceIn
is not exactly the same. So I will revert using coerceIn
. When the range is empty or like [0 to -5] coerceIn
will not work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update!
I added more comments
...-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/threads/ThreadsService.kt
Outdated
Show resolved
Hide resolved
...-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/threads/ThreadsService.kt
Outdated
Show resolved
Hide resolved
...-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/threads/ThreadsService.kt
Outdated
Show resolved
Hide resolved
...-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/threads/ThreadsService.kt
Outdated
Show resolved
Hide resolved
...-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/threads/ThreadsService.kt
Outdated
Show resolved
Hide resolved
fun mapEventsWithEdition(threads: List<TimelineEvent>): List<TimelineEvent> | ||
|
||
/** | ||
* Marks the current thread as read. This is a local implementation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you mean by This is a local implementation
? I think I understand, but for a SDK user who may read this documentation, it will be maybe not clear enough.
} | ||
} | ||
return null | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: could be re-write with
private fun getNotSupportedByThreads(isInThreadTimeline: Boolean, slashCommand: String): Command? {
return if (isInThreadTimeline) {
notSupportedThreadsCommands.firstOrNull { it.command == slashCommand }
} else {
null
}
}
is MessageLocationContent -> { | ||
handleShowLocationPreview(messageContent, informationData.senderId) | ||
} | ||
else -> { | ||
Timber.d("No click action defined for this message content") | ||
onThreadSummaryClicked(informationData.eventId, isRootThreadEvent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is not ideal (but I know it's due to a recent merge), I would prefer to read:
if (isRootThreadEvent) {
onThreadSummaryClicked(informationData.eventId)
} else {
Timber.d("No click action defined for this message content")
}
if (isRootThreadEvent) { | ||
onThreadSummaryClicked(informationData.eventId, isRootThreadEvent) | ||
} else { | ||
timelineViewModel.handle(RoomDetailAction.TapOnFailedToDecrypt(informationData.eventId)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this change, there is no way to get UISI details if the event is a root thread event. But when we have a UISI, can the app knows that this is a root thread event? If yes, I think we must change that.
CC @BillCarsonFr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to investigate that I can remove that to be sure, it will still be clickable the thread summary so we will have both actions :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
} | ||
.show() | ||
} | ||
} | ||
|
||
override fun onThreadSummaryClicked(eventId: String, isRootThreadEvent: Boolean) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove the second param, since it will always be true at the call sites
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually when called from within the TimelineFragment it is not always true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading it twice I think it will always be true when called from the TimelineFragment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line will be true if this is a root thread event and false if this is a normal message, and will act accordignly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update. I will do some test today
@@ -168,6 +171,11 @@ class MessageItemFactory @Inject constructor( | |||
return noticeItemFactory.create(params) | |||
} | |||
|
|||
// This is a thread event and we will [debug] display it when we are in the main timeline |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have move the comment inside the if
block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, updated
@@ -117,7 +117,7 @@ class TimelineEventVisibilityHelper @Inject constructor(private val userPreferen | |||
rootThreadEventId: String? | |||
): Boolean { | |||
// If show hidden events is true we should always display something | |||
if (userPreferencesProvider.shouldShowHiddenEvents()) { | |||
if (userPreferencesProvider.shouldShowHiddenEvents() && !isFromThreadTimeline) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it means that a message edition in a thread will not be displayed as a debug event in the thread? This is fine, just want to confirm. I will do some test when the APK will be ready
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly! It will be displayed in the main timeline. We can of course further improve all those things when threads are more stable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick smoke test and it seems to work fine! Thank!
We can merge if CI is happy.
I will create an issue to test the new screens in the sanity test.
Local Threads implementation for P0 release
Start a new Thread by replying to a message in Timeline #4328
Respond to a Thread #4329
See/Read previous responses in a Thread in the right order (Thread history)
See/Read all Threads in a Room & Thread List view
See Thread Summary from Timeline
Send a link to a Thread
Search results from Threads
Message action bar for events in Thread
Create Thread deleted state UI for when root message removed
See Threads as Reply if don’t have Threads
Disable/Enable thread feature from build configuration while some customers may not want to have threads