Code review

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/search/SearchResult.kt

@@ -23,10 +23,17 @@ import org.matrix.android.sdk.api.session.events.model.Event
  * Domain class to represent the response of a search request in a room.
  */
 data class SearchResult(
-        // Token that can be used to get the next batch of results, by passing as the next_batch parameter to the next call. If this field is absent, there are no more results.
+        /**
+         * Token that can be used to get the next batch of results, by passing as the next_batch parameter to the next call.
+         * If this field is null, there are no more results.
+         */
         val nextBatch: String? = null,
-        // List of words which should be highlighted, useful for stemming which may change the query terms.
-        var highlights: List<String>? = null,
-        // List of results in the requested order.
-        var results: List<Event>? = null
+        /**
+         *  List of words which should be highlighted, useful for stemming which may change the query terms.
+         */
+        val highlights: List<String>? = null,
+        /**
+         * List of results in the requested order.
+         */
+        val results: List<Event>? = null
 )

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/DefaultSession.kt

@@ -96,6 +96,7 @@ internal class DefaultSession @Inject constructor(
         private val pushRuleService: Lazy<PushRuleService>,
         private val pushersService: Lazy<PushersService>,
         private val termsService: Lazy<TermsService>,
+        private val searchService: Lazy<SearchService>,
         private val cryptoService: Lazy<DefaultCryptoService>,
         private val defaultFileService: Lazy<FileService>,
         private val permalinkService: Lazy<PermalinkService>,
@@ -121,8 +122,7 @@ internal class DefaultSession @Inject constructor(
         private val taskExecutor: TaskExecutor,
         private val callSignalingService: Lazy<CallSignalingService>,
         @UnauthenticatedWithCertificate
-        private val unauthenticatedWithCertificateOkHttpClient: Lazy<OkHttpClient>,
-        private val searchService: Lazy<SearchService>
+        private val unauthenticatedWithCertificateOkHttpClient: Lazy<OkHttpClient>
 ) : Session,
         RoomService by roomService.get(),
         RoomDirectoryService by roomDirectoryService.get(),

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/search/SearchAPI.kt

@@ -28,9 +28,9 @@ internal interface SearchAPI {
 
     /**
      * Performs a full text search across different categories.
+     * Ref: https://matrix.org/docs/spec/client_server/r0.6.1#post-matrix-client-r0-search
      */
     @POST(NetworkConstants.URI_API_PREFIX_PATH_R0 + "search")
-    fun search(
-            @Query("next_batch") nextBatch: String?,
-            @Body body: SearchRequestBody): Call<SearchResponse>
+    fun search(@Query("next_batch") nextBatch: String?,
+               @Body body: SearchRequestBody): Call<SearchResponse>
 }

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/search/SearchTask.kt

@@ -69,14 +69,14 @@ internal class DefaultSearchTask @Inject constructor(
                     )
             )
             apiCall = searchAPI.search(params.nextBatch, searchRequestBody)
-        }.toDomain().apply { results = results?.reversed() }
+        }.toDomain()
     }
 
     private fun SearchResponse.toDomain(): SearchResult {
         return SearchResult(
                 nextBatch = searchCategories.roomEvents?.nextBatch,
                 highlights = searchCategories.roomEvents?.highlights,
-                results = searchCategories.roomEvents?.results?.map { it.event }
+                results = searchCategories.roomEvents?.results?.map { it.event }?.reversed()
         )
     }
 }

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/search/request/SearchRequestBody.kt

@@ -22,6 +22,9 @@ import com.squareup.moshi.JsonClass
 
 @JsonClass(generateAdapter = true)
 internal data class SearchRequestBody(
+        /**
+         * Required. Describes which categories to search in and their criteria.
+         */
         @Json(name = "search_categories")
         val searchCategories: SearchRequestCategories
 )

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/search/request/SearchRequestCategories.kt

@@ -22,7 +22,9 @@ import com.squareup.moshi.JsonClass
 
 @JsonClass(generateAdapter = true)
 internal data class SearchRequestCategories(
-        // Mapping of category name to search criteria.
+        /**
+         * Mapping of category name to search criteria.
+         */
         @Json(name = "room_events")
         val roomEvents: SearchRequestRoomEvents? = null
 )

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/search/request/SearchRequestRoomEvents.kt

@@ -22,15 +22,44 @@ import com.squareup.moshi.JsonClass
 
 @JsonClass(generateAdapter = true)
 internal data class SearchRequestRoomEvents(
-        // Required. The string to search events for.
+        /**
+         * Required. The string to search events for.
+         */
         @Json(name = "search_term")
         val searchTerm: String,
+
+        /**
+         * The keys to search. Defaults to all. One of: ["content.body", "content.name", "content.topic"]
+         */
+        @Json(name = "keys")
+        val keys: Any? = null,
+
+        /**
+         * This takes a filter.
+         */
         @Json(name = "filter")
         val filter: SearchRequestFilter? = null,
-        // By default, this is "rank". One of: ["recent", "rank"]
+
+        /**
+         * The order in which to search for results. By default, this is "rank". One of: ["recent", "rank"]
+         */
         @Json(name = "order_by")
         val orderBy: SearchRequestOrder? = null,
-        // Configures whether any context for the events returned are included in the response.
+
+        /**
+         * Configures whether any context for the events returned are included in the response.
+         */
         @Json(name = "event_context")
-        val eventContext: SearchRequestEventContext? = null
+        val eventContext: SearchRequestEventContext? = null,
+
+        /**
+         * Requests the server return the current state for each room returned.
+         */
+        @Json(name = "include_state")
+        val include_state: Boolean? = null
+
+        /**
+         * Requests that the server partitions the result set based on the provided list of keys.
+         */
+        // val groupings: SearchRequestGroupings? = null
 )

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/search/response/SearchResponse.kt

@@ -21,8 +21,10 @@ import com.squareup.moshi.Json
 import com.squareup.moshi.JsonClass
 
 @JsonClass(generateAdapter = true)
-data class SearchResponse(
-        // Required. Describes which categories to search in and their criteria.
+internal data class SearchResponse(
+        /**
+         * Required. Describes which categories to search in and their criteria.
+         */
         @Json(name = "search_categories")
         val searchCategories: SearchResponseCategories
 )

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/search/response/SearchResponseCategories.kt

@@ -21,7 +21,10 @@ import com.squareup.moshi.Json
 import com.squareup.moshi.JsonClass
 
 @JsonClass(generateAdapter = true)
-data class SearchResponseCategories(
+internal data class SearchResponseCategories(
+        /**
+         * Mapping of category name to search criteria.
+         */
         @Json(name = "room_events")
         val roomEvents: SearchResponseRoomEvents? = null
 )

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/search/response/SearchResponseItem.kt

@@ -22,11 +22,22 @@ import com.squareup.moshi.JsonClass
 import org.matrix.android.sdk.api.session.events.model.Event
 
 @JsonClass(generateAdapter = true)
-data class SearchResponseItem(
-        // A number that describes how closely this result matches the search. Higher is closer.
+internal data class SearchResponseItem(
+        /**
+         *  A number that describes how closely this result matches the search. Higher is closer.
+         */
         @Json(name = "rank")
         val rank: Double? = null,
-        // The event that matched.
+
+        /**
+         * The event that matched.
+         */
         @Json(name = "result")
-        val event: Event
+        val event: Event,
+
+        /**
+         * Context for result, if requested.
+         */
+        @Json(name = "context")
+        val context: SearchResponseEventContext? = null
 )

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/search/response/SearchResponseRoomEvents.kt

@@ -21,7 +21,7 @@ import com.squareup.moshi.Json
 import com.squareup.moshi.JsonClass
 
 @JsonClass(generateAdapter = true)
-class SearchResponseRoomEvents(
+internal data class SearchResponseRoomEvents(
         // List of results in the requested order.
         @Json(name = "results")
         val results: List<SearchResponseItem>? = null,

vector/src/main/java/im/vector/app/features/home/room/detail/search/SearchFragment.kt

@@ -46,7 +46,7 @@ data class SearchArgs(
 
 class SearchFragment @Inject constructor(
         val viewModelFactory: SearchViewModel.Factory,
-        val controller: SearchResultController
+        private val controller: SearchResultController
 ) : VectorBaseFragment(), StateView.EventCallback, SearchResultController.Listener {
 
     private val fragmentArgs: SearchArgs by args()
@@ -85,13 +85,13 @@ class SearchFragment @Inject constructor(
     }
 
     override fun invalidate() = withState(searchViewModel) { state ->
-        if (state.searchResult?.results.isNullOrEmpty()) {
-            when (state.asyncEventsRequest) {
+        if (state.searchResult.isNullOrEmpty()) {
+            when (state.asyncSearchRequest) {
                 is Loading -> {
                     stateView.state = StateView.State.Loading
                 }
                 is Fail    -> {
-                    stateView.state = StateView.State.Error(errorFormatter.toHumanReadable(state.asyncEventsRequest.error))
+                    stateView.state = StateView.State.Error(errorFormatter.toHumanReadable(state.asyncSearchRequest.error))
                 }
                 is Success -> {
                     stateView.state = StateView.State.Empty(
@@ -100,7 +100,7 @@ class SearchFragment @Inject constructor(
                 }
             }
         } else {
-            val lastBatchSize = state.lastBatch?.results?.size ?: 0
+            val lastBatchSize = state.lastBatch?.size ?: 0
             pendingScrollToPosition = if (lastBatchSize > 0) lastBatchSize - 1 else 0
 
             stateView.state = StateView.State.Content

vector/src/main/java/im/vector/app/features/home/room/detail/search/SearchResultController.kt

@@ -48,7 +48,9 @@ class SearchResultController @Inject constructor(
     }
 
     override fun buildModels(data: SearchViewState?) {
-        if (!data?.searchResult?.nextBatch.isNullOrEmpty()) {
+        data ?: return
+
+        if (data.hasMoreResult) {
             loadingItem {
                 // Always use a different id, because we can be notified several times of visibility state changed
                 id("loadMore${idx++}")
@@ -60,7 +62,7 @@ class SearchResultController @Inject constructor(
             }
         }
 
-        buildSearchResultItems(data?.searchResult?.results.orEmpty())
+        buildSearchResultItems(data.searchResult.orEmpty())
     }
 
     private fun buildSearchResultItems(events: List<Event>) {
@@ -83,12 +85,9 @@ class SearchResultController @Inject constructor(
                 avatarRenderer(avatarRenderer)
                 dateFormatter(dateFormatter)
                 event(event)
+                // I think we should use the data returned by the server?
                 sender(event.senderId?.let { session.getUser(it) })
-                listener(object : SearchResultItem.Listener {
-                    override fun onItemClicked() {
-                        listener?.onItemClicked(event)
-                    }
-                })
+                listener { listener?.onItemClicked(event) }
             }
         }
     }

vector/src/main/java/im/vector/app/features/home/room/detail/search/SearchResultItem.kt

@@ -23,8 +23,10 @@ import com.airbnb.epoxy.EpoxyModelClass
 import im.vector.app.R
 import im.vector.app.core.date.DateFormatKind
 import im.vector.app.core.date.VectorDateFormatter
+import im.vector.app.core.epoxy.ClickListener
 import im.vector.app.core.epoxy.VectorEpoxyHolder
 import im.vector.app.core.epoxy.VectorEpoxyModel
+import im.vector.app.core.epoxy.onClick
 import im.vector.app.features.home.AvatarRenderer
 import org.matrix.android.sdk.api.session.events.model.Event
 import org.matrix.android.sdk.api.session.user.model.User
@@ -33,21 +35,21 @@ import org.matrix.android.sdk.api.util.toMatrixItem
 @EpoxyModelClass(layout = R.layout.item_search_result)
 abstract class SearchResultItem : VectorEpoxyModel<SearchResultItem.Holder>() {
 
-    @EpoxyAttribute var avatarRenderer: AvatarRenderer? = null
+    @EpoxyAttribute lateinit var avatarRenderer: AvatarRenderer
     @EpoxyAttribute var dateFormatter: VectorDateFormatter? = null
-    @EpoxyAttribute var event: Event? = null
+    @EpoxyAttribute lateinit var event: Event
     @EpoxyAttribute var sender: User? = null
-    @EpoxyAttribute var listener: Listener? = null
+    @EpoxyAttribute var listener: ClickListener? = null
 
     override fun bind(holder: Holder) {
         super.bind(holder)
-        event ?: return
 
-        holder.view.setOnClickListener { listener?.onItemClicked() }
-        sender?.toMatrixItem()?.let { avatarRenderer?.render(it, holder.avatarImageView) }
-        holder.memberNameView.text = sender?.displayName
-        holder.timeView.text = dateFormatter?.format(event!!.originServerTs, DateFormatKind.MESSAGE_SIMPLE)
-        holder.contentView.text = event?.content?.get("body") as? String
+        holder.view.onClick(listener)
+        sender?.toMatrixItem()?.let { avatarRenderer.render(it, holder.avatarImageView) }
+        holder.memberNameView.text = sender?.getBestName()
+        holder.timeView.text = dateFormatter?.format(event.originServerTs, DateFormatKind.MESSAGE_SIMPLE)
+        // TODO Improve that (use formattedBody, etc.)
+        holder.contentView.text = event.content?.get("body") as? String
     }
 
     class Holder : VectorEpoxyHolder() {
@@ -56,8 +58,4 @@ abstract class SearchResultItem : VectorEpoxyModel<SearchResultItem.Holder>() {
         val timeView by bind<TextView>(R.id.messageTimeView)
         val contentView by bind<TextView>(R.id.messageContentView)
     }
-
-    interface Listener {
-        fun onItemClicked()
-    }
 }

vector/src/main/java/im/vector/app/features/home/room/detail/search/SearchViewModel.kt

@@ -37,15 +37,14 @@ import org.matrix.android.sdk.internal.util.awaitCallback
 
 class SearchViewModel @AssistedInject constructor(
         @Assisted private val initialState: SearchViewState,
-        private val session: Session
+        session: Session
 ) : VectorViewModel<SearchViewState, SearchAction, SearchViewEvents>(initialState) {
 
-    private var room: Room? = null
+    private var room: Room? = session.getRoom(initialState.roomId)
+
     private var currentTask: Cancelable? = null
 
-    init {
-        room = initialState.roomId?.let { session.getRoom(it) }
-    }
+    private var nextBatch: String? = null
 
     @AssistedInject.Factory
     interface Factory {
@@ -64,8 +63,8 @@ class SearchViewModel @AssistedInject constructor(
     override fun handle(action: SearchAction) {
         when (action) {
             is SearchAction.SearchWith -> handleSearchWith(action)
-            is SearchAction.LoadMore   -> handleLoadMore()
-            is SearchAction.Retry      -> handleRetry()
+            is SearchAction.LoadMore -> handleLoadMore()
+            is SearchAction.Retry -> handleRetry()
         }.exhaustive
     }
 
@@ -74,7 +73,7 @@ class SearchViewModel @AssistedInject constructor(
             setState {
                 copy(searchTerm = action.searchTerm)
             }
-            startSearching()
+            startSearching(false)
         }
     }
 
@@ -83,25 +82,25 @@ class SearchViewModel @AssistedInject constructor(
     }
 
     private fun handleRetry() {
-        startSearching()
+        startSearching(false)
     }
 
-    private fun startSearching(isNextBatch: Boolean = false) = withState { state ->
-        if (state.roomId == null || state.searchTerm == null) return@withState
+    private fun startSearching(isNextBatch: Boolean) = withState { state ->
+        if (state.searchTerm == null) return@withState
 
         // There is no batch to retrieve
-        if (isNextBatch && state.searchResult?.nextBatch == null) return@withState
+        if (isNextBatch && nextBatch == null) return@withState
 
         // Show full screen loading just for the clean search
         if (!isNextBatch) {
             setState {
                 copy(
-                        asyncEventsRequest = Loading()
+                        asyncSearchRequest = Loading()
                 )
             }
         }
 
-        if (state.asyncEventsRequest is Loading) {
+        if (state.asyncSearchRequest is Loading) {
             currentTask?.cancel()
         }
 
@@ -110,7 +109,7 @@ class SearchViewModel @AssistedInject constructor(
                 val result = awaitCallback<SearchResult> {
                     currentTask = room?.search(
                             searchTerm = state.searchTerm,
-                            nextBatch = state.searchResult?.nextBatch,
+                            nextBatch = nextBatch,
                             orderByRecent = true,
                             beforeLimit = 0,
                             afterLimit = 0,
@@ -126,7 +125,7 @@ class SearchViewModel @AssistedInject constructor(
                 _viewEvents.post(SearchViewEvents.Failure(failure))
                 setState {
                     copy(
-                            asyncEventsRequest = Fail(failure),
+                            asyncSearchRequest = Fail(failure),
                             searchResult = null
                     )
                 }
@@ -135,27 +134,19 @@ class SearchViewModel @AssistedInject constructor(
     }
 
     private fun onSearchResultSuccess(searchResult: SearchResult, isNextBatch: Boolean) = withState { state ->
-        val accumulatedResult = SearchResult(
-                nextBatch = searchResult.nextBatch,
-                results = searchResult.results,
-                highlights = searchResult.highlights
-        )
-
         // Accumulate results if it is the next batch
-        if (isNextBatch) {
-            if (state.searchResult != null) {
-                accumulatedResult.results = accumulatedResult.results?.plus(state.searchResult.results!!)
-            }
-            if (state.searchResult?.highlights != null) {
-                accumulatedResult.highlights = accumulatedResult.highlights?.plus(state.searchResult.highlights!!)
-            }
-        }
+        val accumulatedResult = searchResult.results.orEmpty().plus(state.searchResult?.takeIf { isNextBatch }.orEmpty())
+
+        // Note: We do not care about the highlights for the moment, but it will be the same algorithm
+
+        nextBatch = searchResult.nextBatch
 
         setState {
             copy(
                     searchResult = accumulatedResult,
-                    lastBatch = searchResult,
-                    asyncEventsRequest = Success(Unit)
+                    hasMoreResult = !nextBatch.isNullOrEmpty(),
+                    lastBatch = searchResult.results,
+                    asyncSearchRequest = Success(Unit)
             )
         }
     }

vector/src/main/java/im/vector/app/features/home/room/detail/search/SearchViewState.kt

@@ -19,17 +19,18 @@ package im.vector.app.features.home.room.detail.search
 import com.airbnb.mvrx.Async
 import com.airbnb.mvrx.MvRxState
 import com.airbnb.mvrx.Uninitialized
-import org.matrix.android.sdk.api.session.search.SearchResult
+import org.matrix.android.sdk.api.session.events.model.Event
 
 data class SearchViewState(
         // Accumulated search result
-        val searchResult: SearchResult? = null,
+        val searchResult: List<Event>? = null,
+        val hasMoreResult: Boolean = false,
         // Last batch result will help RecyclerView to position itself
-        val lastBatch: SearchResult? = null,
+        val lastBatch: List<Event>? = null,
         val searchTerm: String? = null,
-        val roomId: String? = null,
+        val roomId: String = "",
         // Current pagination request
-        val asyncEventsRequest: Async<Unit> = Uninitialized
+        val asyncSearchRequest: Async<Unit> = Uninitialized
 ) : MvRxState {
 
     constructor(args: SearchArgs) : this(roomId = args.roomId)

vector/src/main/res/layout/activity_search.xml

@@ -24,8 +24,8 @@
                 style="@style/VectorSearchView"
                 android:layout_width="match_parent"
                 android:layout_height="wrap_content"
-                app:queryHint="@string/search_hint"
-                android:backgroundTint="@color/base_color" />
+                android:backgroundTint="@color/base_color"
+                app:queryHint="@string/search_hint" />
 
         </androidx.appcompat.widget.Toolbar>
 

vector/src/main/res/layout/fragment_search.xml

@@ -1,5 +1,6 @@
 <?xml version="1.0" encoding="utf-8"?>
 <im.vector.app.core.platform.StateView xmlns:android="http://schemas.android.com/apk/res/android"
+    xmlns:tools="http://schemas.android.com/tools"
     android:id="@+id/stateView"
     android:layout_width="match_parent"
     android:layout_height="match_parent">
@@ -8,6 +9,7 @@
         android:id="@+id/searchResultRecycler"
         android:layout_width="match_parent"
         android:layout_height="match_parent"
-        android:overScrollMode="always" />
+        android:overScrollMode="always"
+        tools:listitem="@layout/item_search_result" />
 
 </im.vector.app.core.platform.StateView>