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.

Sign up for GitHub

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

Jump to bottom

Fix issue 4600 that search results sometimes are not displayed properly #4601

Merged
merged 2 commits into from
Dec 13, 2021
Merged

Fix issue 4600 that search results sometimes are not displayed properly #4601

merged 2 commits into from
Dec 13, 2021

Conversation

ariskotsomitopoulos
Copy link
Contributor

Fixing issue #4600

@ariskotsomitopoulos ariskotsomitopoulos linked an issue Dec 1, 2021 that may be closed by this pull request
The very first search in a room sometimes fails. #4600
Closed
bmarty
bmarty reviewed Dec 1, 2021
View reviewed changes
vector/src/main/java/im/vector/app/features/home/room/detail/search/SearchResultController.kt
@@ -54,10 +54,6 @@ class SearchResultController @Inject constructor(
fun loadMore()
}

init {
setData(null)
}
Copy link
Member

Choose a reason for hiding this comment

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

I count 9 other controllers with the same code, do you think the same fix should be applied?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think yes, they should be removed. I will update the PR and test those screens

@github-actions
Copy link

github-actions bot commented Dec 1, 2021 •
edited
Loading

Unit Test Results

  66 files  ±0    66 suites  ±0   1m 3s ⏱️ +15s
135 tests ±0  135 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
418 runs  ±0  418 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit f8f5e7e. ± Comparison against base commit 2e52f42.

♻️ This comment has been updated with latest results.

@bmarty
Copy link
Member

bmarty commented Dec 2, 2021

After a discussion with @ganfra this code was added for performance reason (it should have deserve some more comments), to not block the UI thread if the controller has many items to compute. It is maybe not necessary anymore, but how can we be certain of that?

@ariskotsomitopoulos
Copy link
Contributor Author

After a discussion with @ganfra this code was added for performance reason (it should have deserve some more comments), to not block the UI thread if the controller has many items to compute. It is maybe not necessary anymore, but how can we be certain of that?

Yes that will make the buildModels run on background. However, this is equivalent to that #4591. We are certain that the first model build should be in UI thread otherwise the views will not be properly restored. Both the documentation and the code doc explain that. Also, this search result bug is a demonstration that this can produce problems

@bmarty bmarty merged commit 2c81e7e into develop Dec 13, 2021
@bmarty bmarty deleted the feature/aris/search_no_data_displayed_4600 branch December 13, 2021 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bmarty bmarty bmarty left review comments

@ouchadam ouchadam ouchadam approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The very first search in a room sometimes fails.
3 participants
@ariskotsomitopoulos @bmarty @ouchadam