Fix search results always being empty when using a backend that doesn't return highlights #8729
Conversation
mlaily
force-pushed
the
fix-search-without-highlights
branch
from
9180675
to
9e64cb7
Compare
…urn highlights This notably happens with Synapse on SQLite.
mlaily
force-pushed
the
fix-search-without-highlights
branch
from
9e64cb7
to
342d46b
Compare
There was a problem hiding this comment.
Merci pour le PR @mlaily !
The method setHighLightedText may return null to mitigate this issue: element-hq/synapse#8686. It's maybe worth checking the status of this Synapse issue before being able to merge this change.
| // In this case, don't attempt to filter based on the highlights, to avoid erasing all the results... | ||
| if (highlights.isEmpty()) { | ||
| return wordToSpan | ||
| } |
There was a problem hiding this comment.
This will work, hence another possible fix would have been to just remove the var found below.
But it's maybe clearer like this.
|
Hi @bmarty, thanks for the review! I noticed the issue you mentioned, but it's still open and I'm not knowledgeable enough about synapse to be certain it's solved or not myself. I felt it was better to be conservative and keep the existing filtering behavior as is and just add a check for the specific situation where the highlights are not returned at all. Does the PR looks okay to you? If not I'm sorry but I'm not sure what I should change. Could you point me in the direction you think is correct? Thanks! |
|
Hi @mlaily , Thanks for the feedback. This project is in maintenance mode now, we are focusing on Element X now (https://github.com/element-hq/element-x-android). So we would merge only PRs that fixes critical issues, or PRs which do not contain any doubt. I am afraid that we do not want to go further with the proposal changes. |
|
Hi, Oh ok I didn't know that. Is the switch to Element X planned soon though? It is still in pre-alpha according to its readme, and I just tried to use it with my home server but it doesn't work because I don't have the sliding sync feature enabled...
The search feature not working at all seems like a pretty big issue to me, even though it only happens with some backends... I don't understand your doubts about my 3 lines of code fix so it's a bit disappointing to hear. I hope you can somehow revisit this decision. |
…thing (#17000) Fixes #16999 and element-hq/element-android#8729 by returning the search terms as search highlights.
|
|
Fix search results always being empty when using a backend that doesn't return highlights.
This notably happens with Synapse on SQLite.
Type of change
Content
Search results are filtered to make sure there is at least one match with the
highlightsreturned by the backend.The issue is that some backends — in my case Synapse with SQLite instead of PostGreSQL — return an empty array for highlights.
The result is that search results always appear empty from the Android app. (The web element app does something different because search results look ok there with the same server)
As a minimal workaround I simply bypassed the filter in case the highlights are empty.
This should preserve the existing behaviour in most situation while allowing the search page to work when highlights are not returned by the backend.
Motivation and context
No open issue AFAIK
Screenshots / GIFs
Tests
I tested my changes on my own homeserver to make sure it works.
Tested devices
Checklist
Signed-off-by: Melvyn Laïly melvyn.laily@gmail.com