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

Threads P0 Release #4746

Merged
merged 144 commits into from
Feb 1, 2022
Merged

Threads P0 Release #4746

merged 144 commits into from
Feb 1, 2022

Conversation

ariskotsomitopoulos
Copy link
Contributor

@ariskotsomitopoulos ariskotsomitopoulos commented Dec 17, 2021

Local Threads implementation for P0 release

 - 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)
Copy link
Member

@bmarty bmarty Jan 31, 2022

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.

Copy link
Contributor Author

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!

Copy link
Contributor Author

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

Copy link
Member

Choose a reason for hiding this comment

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

OK

Copy link
Member

@bmarty bmarty left a 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

fun mapEventsWithEdition(threads: List<TimelineEvent>): List<TimelineEvent>

/**
* Marks the current thread as read. This is a local implementation
Copy link
Member

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
}
Copy link
Member

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)
Copy link
Member

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))
Copy link
Member

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

Copy link
Contributor Author

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 :)

Copy link
Member

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) {
Copy link
Member

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

Copy link
Contributor Author

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

Copy link
Member

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

Copy link
Contributor Author

@ariskotsomitopoulos ariskotsomitopoulos Feb 1, 2022

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

@ariskotsomitopoulos ariskotsomitopoulos changed the title Threads local implementation Threads P0 Release Feb 1, 2022
Copy link
Member

@bmarty bmarty left a 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
Copy link
Member

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.

Copy link
Contributor Author

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) {
Copy link
Member

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

Copy link
Contributor Author

@ariskotsomitopoulos ariskotsomitopoulos Feb 1, 2022

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

Copy link
Member

@bmarty bmarty left a 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.

@ariskotsomitopoulos ariskotsomitopoulos merged commit 3d5f8ed into develop Feb 1, 2022
@ariskotsomitopoulos ariskotsomitopoulos deleted the feature/aris/threads branch February 1, 2022 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants