-
Notifications
You must be signed in to change notification settings - Fork 31
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.
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
fix: Issue with blank conversations [WPB-14276] #3624
Conversation
@@ -55,7 +59,7 @@ import com.wire.kalium.logic.data.user.UserId | |||
import kotlinx.collections.immutable.toImmutableList | |||
import kotlinx.coroutines.flow.flowOf | |||
|
|||
@Suppress("LongParameterList") | |||
@Suppress("LongParameterList", "CyclomaticComplexMethod") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we supress it? I've seen we have it around the code, but Im not sure 🤔
@@ -71,6 +75,15 @@ fun ConversationList( | |||
onAudioPermissionPermanentlyDenied: () -> Unit = {} | |||
) { | |||
val context = LocalContext.current | |||
var listScrollState by remember { mutableStateOf(false to 0) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is your take on the Pair
? I'd say its enough for our intermediate state here, but we can always change it to a dedicated data class if we need some more context
|
Built wire-android-staging-compat-pr-3624.apk is available for download |
Built wire-android-dev-debug-pr-3624.apk is available for download |
Now the whole list disappears when you move your finger down as if to scroll up and blinks when you actually start scrolling: Screen_recording_20241113_125357.mp4 |
Interesting, it indeed happens for me too when I enter the screen through conversations 🤔 I will check it out, thanks! |
I quicky checked and the issue is there even without any of my changes, it started to happen after latest changes to develop where we have the deprecated |
Deprecated It looks like there are three different scenarios:
I managed to catch all three on this video and this is the version from before the pagination, from 6 weeks ago: Screen_recording_20241113_151711.mp4So it looks like fixing the first one makes it so that the other two happen more frequently. There were some issues with transition animation and disappearing list and it was related to |
I investigated that, since I was the one making changes (and bugs 😅) to it recently, and found out that the issue is with the usage of |
https://wearezeta.atlassian.net/browse/WPB-14276
What's new in this PR?
Issues
When we navigated between screens sometimes the conversation list was empty until we manually scrolled, also sometimes there were those weird jumps.
Causes (Optional)
Turns out out current
keepOnTopWhenNotScrolled
is not working properly with animating lazy items.Solutions
Right now we're still using
Snapshot.withoutReadObservation
but we only use it to get current state, to make sure we're on top - without this we would end up being on the second place and not scrolling the list.The difference is that we create a tmp state which has the flag
isAtTheTop
with the number of items in the list, this state is out distinction of when we should try to animate to top.The next big difference is that we're not relying on
requestScrollToItem
but rather onanimateScrollToItem
which is a suspending function, which seems to work together with theanimateItem
Testing
How to Test
Repeat it multiple times
Also might be worth to checkout scrolling to top:
New activity
is properly visible and not under the search or toolbarAttachments (Optional)
Attachments like images, videos, etc. (drag and drop in the text box)
PR Post Submission Checklist for internal contributors (Optional)
PR Post Merge Checklist for internal contributors
References
feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764
.