search

msfjarvis / compose-lobsters

Claw for Android: Unofficial read-only client for https://lobste.rs, built using Jetpack Compose.
https://play.google.com/store/apps/details?id=dev.msfjarvis.claw.android
MIT License
99 stars 8 forks source link

Remember the comment expanded/collapsed state when scrolling. #673

Closed rubenquadros closed 1 month ago

rubenquadros commented 1 month ago

Remember the comment expanded/collapsed state when scrolling. Previously, the state was remembered inside the Node composable. When the users scroll, and Node re-composes, the state was being reset.

In this commit, we add the collapsed state into the CommentNode. This makes sure that the state is retained even when the UI is re-composed.

Fixes #651

msfjarvis commented 1 month ago

I think the general approach looks reasonable, but it really needs to be able to live inside the top-level CommentsPage composable and not leak mutable mutable state belonging to that screen into the CommentNode file.

msfjarvis commented 1 month ago

I think this current version works pretty well, but the scroll position goes completely out of whack when collapsing a comment. Go to any post with a comment tree that has 4-5 nested items and try to collapse the topmost parent, you get scrolled way up far away from where you started.

I don't have a surefire solution at the moment, triggering a delayed scroll back to the comment which was collapsed might be worth looking into but I'm not sure if it's possible.

rubenquadros commented 1 month ago

I think this current version works pretty well, but the scroll position goes completely out of whack when collapsing a comment. Go to any post with a comment tree that has 4-5 nested items and try to collapse the topmost parent, you get scrolled way up far away from where you started.

I don't have a surefire solution at the moment, triggering a delayed scroll back to the comment which was collapsed might be worth looking into but I'm not sure if it's possible.

Yeah i see that. Should be good with AnimatedVisibility.

msfjarvis commented 1 month ago

Oh can you update the title? If not then I'll do it tomorrow

msfjarvis commented 1 month ago

I found a bug while testing, with these changes I can only collapse the topmost comments and not ones under them which works correctly on the main branch. Can you take a look?

rubenquadros commented 1 month ago

I found a bug while testing, with these changes I can only collapse the topmost comments and not ones under them which works correctly on the main branch. Can you take a look?

Yeah i found the issue. Because when updating the expanded/collapsed state I only do it in the parent. I will have to check for the shortId in the children as well. See the updateListNode function.

msfjarvis commented 1 month ago

I found a bug while testing, with these changes I can only collapse the topmost comments and not ones under them which works correctly on the main branch. Can you take a look?

Yeah i found the issue. Because when updating the expanded/collapsed state I only do it in the parent. I will have to check for the shortId in the children as well. See the updateListNode function.

Do you wanna try to fix that? If not I can merge this as-is and fix it later myself.

rubenquadros commented 1 month ago

I found a bug while testing, with these changes I can only collapse the topmost comments and not ones under them which works correctly on the main branch. Can you take a look?

Yeah i found the issue. Because when updating the expanded/collapsed state I only do it in the parent. I will have to check for the shortId in the children as well. See the updateListNode function.

Do you wanna try to fix that? If not I can merge this as-is and fix it later myself.

Yeah maybe a different MR all together. I'm not getting any elegant solution at the moment :P

msfjarvis commented 1 month ago

I've fixed the bug with child comments so everything works perfectly now, thanks again for your PR!