Bring selected filter to front

[frebib]

It looked weird before to always have the selected chip disappear and slide behind the deselected filters.

Type of change

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

Content

Selected filters in the room list filter bar now slide in front instead of behind when moving to the left

Motivation and context

It looks nicer

Screenshots / GIFs

Note that I have my animation scale set to 5x here. After:

https://github.com/element-hq/element-x-android/assets/775104/63ec9ec9-0ee8-4aa0-afc1-76c15ab4360f

Before:

https://github.com/element-hq/element-x-android/assets/775104/81ea2b1d-3e6b-4fe6-b894-2f8b3ee38da8

Tests

• Step 1: Open app
• Step 2: Click buttons
• Step 3: Observe

Tested devices

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

Checklist

• [x] Changes have been tested on an Android device or Android emulator with API 23
• [x] UI change has been tested on both light and dark themes
• [ ] Accessibility has been taken into account. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#accessibility
• [x] Pull request is based on the develop branch
• [ ] Pull request includes a new file under ./changelog.d. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#changelog
• [x] 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

[github-actions[bot]]

Thank you for your contribution! Here are a few things to check in the PR to ensure it's reviewed as quickly as possible:

• Your branch should be based on origin/develop, at least when it was created.
• There is a changelog entry in the changelog.d folder with the Towncrier format.
• The test pass locally running ./gradlew test.
• The code quality check suite pass locally running ./gradlew runQualityChecks.
• If you modified anything related to the UI, including previews, you'll have to run the Record screenshots GH action in your forked repo: that will generate compatible new screenshots. However, given Github Actions limitations, it will prevent the CI from running temporarily, until you upload a new commit after that one. To do so, just pull the latest changes and push an empty commit.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 95.83333% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 74.22%. Comparing base (7bda5fd) to head (b852578).

Files	Patch %	Lines
...tures/roomlist/impl/filters/RoomListFiltersView.kt	95.83%	0 Missing and 1 partial :warning:

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #2809   +/-   ##
========================================
  Coverage    74.21%   74.22%           
========================================
  Files         1540     1540           
  Lines        36892    36905   +13     
  Branches      7149     7150    +1     
========================================
+ Hits         27381    27394   +13     
  Misses        5779     5779           
  Partials      3732     3732           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[frebib]

Yeah, you're right, that does look a little weird. I'm not sure how we'd go about doing that though. Maybe base the zindex on the order that the buttons are last touched?

[bmarty]

It's actually maybe OK, since the filter which is moving to the left is always visible 🤷‍♂️

[frebib]

Yeah I think it looks weird in either case. For comparison here is the old behaviour

https://github.com/element-hq/element-x-android/assets/775104/81ea2b1d-3e6b-4fe6-b894-2f8b3ee38da8

[frebib]

I compared this to how it works on iOS and it's not quite the same. iOS seems to always bring just-deselected filters to the front too. Otherwise it's very similar to the newer behaviour. It's not as easy to tell because I don't know how to slow the animations down, and they seem to be faster anyway (I guess to fudge any remaining visual error?)

As a side note: I tried to tweak the animations to match iOS more closely, because that looks waay more fluid. The chip text/background colour fades smoothly which is easy enough to add. The only missing thing is that on iOS excluded filters fade in/out, but we can't do that until Jetpack Compose 1.7.0 is released and EXA is using it: https://issuetracker.google.com/issues/150812265#comment50

[frebib]

That looks nicer indeed, thanks.

When deselecting a filter, shouldn't the deselected filter also stay on top?

With this change I have this:

Please let me know what you think.

I managed to implement this, although it's a bit clunky. I'm not too good with Kotlin. Either way, I think this does what you want. There's also one more commit on this branch to animate colours too. Let me know what you think

https://github.com/element-hq/element-x-android/assets/775104/2be5edfc-500b-4cc7-bf7c-e729d42724ee

https://github.com/frebib/element-x-android/compare/frebib/filter-in-front...frebib:element-x-android:frebib/filter-in-front-improved

[jmartinesp]

I think I'll take over reviewing this PR since @bmarty is off for a few days. This last iteration looks really good to me and it does match iOS way more closely than the current one, good job! The code doesn't seem too complex, which is great too.

[frebib]

I reinstated the LaunchedEffect that makes the filters snap back into position when clearing them (seeing as it was added on purpose), but due to the fading of the colours now, it looks kinda glitchy, especially at 1x speed. I wonder if we should get rid of it?

[jmartinesp]

I think that can be fixed by specifying an animationSpec to the animateColorAsState like this:

val background = animateColorAsState(
    targetValue = if (selected) ElementTheme.colors.bgActionPrimaryRest else ElementTheme.colors.bgCanvasDefault,
    animationSpec = spring(stiffness = Spring.StiffnessMediumLow),
    label = "chip background colour",
)

The animation takes longer to apply so it doesn't look that sudden. We can play with that value until we find the right stiffness, but I think medium low looks good.

[jmartinesp]

@frebib could you add the AnimationSpec changes to your branch? I'd do it myself, but I don't have permissions.

[frebib]

As discussed in chat, this needs more work

[frebib]

I managed to fix the zindex ordering as Benoit pointed out. I also added the stiffness changes. Here is how it looks now

https://github.com/element-hq/element-x-android/assets/775104/99fdfe50-53bd-42e1-b57b-2919a006ba37