element-android icon indicating copy to clipboard operation
element-android copied to clipboard

Fix search results always being empty when using a backend that doesn't return highlights

Open mlaily opened this issue 5 months ago • 3 comments

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

  • [ ] Feature
  • [x] Bugfix
  • [ ] Technical
  • [ ] Other :

Content

Search results are filtered to make sure there is at least one match with the highlights returned 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

  • [x] Physical
  • [ ] Emulator
  • OS version(s): Android 13 (33)

Checklist

  • [ ] Changes has been tested on an Android device or Android emulator with API 21
  • [ ] UI change has been tested on both light and dark themes
  • [ ] Accessibility has been taken into account. See https://github.com/element-hq/element-android/blob/develop/CONTRIBUTING.md#accessibility
  • [x] Pull request is based on the develop branch
  • [x] Pull request includes a new file under ./changelog.d. See https://github.com/element-hq/element-android/blob/develop/CONTRIBUTING.md#changelog
  • [ ] Pull request includes screenshots or videos if containing UI changes
  • [x] Pull request includes a sign off
  • [x] You've made a self review of your PR
  • [ ] If you have modified the screen flow, or added new screens to the application, you have updated the test UiAllScreensSanityTest.allScreensTest()

Signed-off-by: Melvyn Laïly [email protected]

mlaily avatar Jan 07 '24 21:01 mlaily

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!

mlaily avatar Feb 05 '24 18:02 mlaily

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.

bmarty avatar Feb 16 '24 14:02 bmarty

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...

So we would merge only PRs that fixes critical issues, or PRs which do not contain any doubt.

The search feature not working at all seems like a pretty big issue to me, even though it only happens with some backends... It is nonetheless a real annoyance my users are facing right now.

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.

mlaily avatar Feb 16 '24 14:02 mlaily