Skip to content
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

Unexpected stop of first item moving in intermediate state. #32

Closed
zhelenskiy opened this issue May 5, 2024 · 60 comments
Closed

Unexpected stop of first item moving in intermediate state. #32

zhelenskiy opened this issue May 5, 2024 · 60 comments
Labels
Waiting for Compose v1.7.0 (try 2.4.0-beta01) Waiting for Compose Foundation v1.7.0 to be in Compose Multiplatform

Comments

@zhelenskiy
Copy link

zhelenskiy commented May 5, 2024

I have issues with LazyRow when I add more items. The issue happens only when I add items and only when I move the first item.

Screen.Recording.2024-05-05.at.03.24.05.mov

Here is working example when I move other items.

Screen.Recording.2024-05-05.at.03.32.24.mov

I'll try to figure out what happens or minimize the example later today, but if you already have any idea what happens, let me now.

The version is 2.0.1, the platform is Desktop. I test it on Mac OS 14.4.1.

@Calvin-LL
Copy link
Owner

Oh this is very strange. I have no clue how this is happening. I will wait for your minimized example. Is this on the latest version of reorderable?

@zhelenskiy
Copy link
Author

Yes

@Calvin-LL
Copy link
Owner

Did this work with an older version of the library? Also does this have a invisible item before the first item like in #4? I'm assuming so because I don't see a jump after moving the first item.

@zhelenskiy
Copy link
Author

No, I don't have any invisible items. I didn't test for older versions. But I reproduced it successfully with a smaller example.

Screen.Recording.2024-05-05.at.04.15.51.mov
import androidx.compose.animation.*
import androidx.compose.animation.core.animateDpAsState
import androidx.compose.desktop.ui.tooling.preview.Preview
import androidx.compose.foundation.*
import androidx.compose.foundation.layout.*
import androidx.compose.foundation.lazy.LazyRow
import androidx.compose.foundation.lazy.itemsIndexed
import androidx.compose.foundation.lazy.rememberLazyListState
import androidx.compose.foundation.shape.RoundedCornerShape
import androidx.compose.material.*
import androidx.compose.material.MaterialTheme.colors
import androidx.compose.material.icons.Icons
import androidx.compose.material.icons.filled.Add
import androidx.compose.material.icons.filled.Close
import androidx.compose.runtime.*
import androidx.compose.ui.Alignment
import androidx.compose.ui.Modifier
import androidx.compose.ui.draw.clip
import androidx.compose.ui.graphics.Color
import androidx.compose.ui.graphics.SolidColor
import androidx.compose.ui.platform.LocalDensity
import androidx.compose.ui.text.TextStyle
import androidx.compose.ui.text.input.TextFieldValue
import androidx.compose.ui.text.rememberTextMeasurer
import androidx.compose.ui.unit.dp
import androidx.compose.ui.unit.sp
import androidx.compose.ui.window.Window
import androidx.compose.ui.window.application
import kotlinx.coroutines.launch
import sh.calvin.reorderable.ReorderableItem
import sh.calvin.reorderable.rememberReorderableLazyListState

@Composable
@Preview
fun App() {
    MaterialTheme {
        TabRow()
    }
}

fun main() = application {
    Window(onCloseRequest = ::exitApplication) {
        App()
    }
}

typealias Id = Long

data class Tab(val id: Id)

@OptIn(ExperimentalFoundationApi::class)
@Composable
private fun TabRow() {
    var tabModels: List<Tab> by remember { mutableStateOf(listOf(Tab(-1))) }
    val coroutineScope = rememberCoroutineScope()
    val tabRowScrollState = rememberLazyListState()
    val reorderableLazyColumnState =
        rememberReorderableLazyListState(tabRowScrollState) { from, to ->
            coroutineScope.launch {
                tabModels = tabModels.toMutableList().apply {
                    add(to.index, removeAt(from.index))
                }
            }
        }
    var chosenId by remember { mutableStateOf(tabModels.first().id) }
    BoxWithConstraints {
        AnimatedVisibility(
            visible = maxHeight > 300.dp,
            enter = expandVertically(),
            exit = shrinkVertically(),
        ) {
            LazyRow(
                state = tabRowScrollState,
                verticalAlignment = Alignment.CenterVertically,
                modifier = Modifier.fillMaxWidth()
            ) {
                itemsIndexed(
                    tabModels,
                    key = { _, model -> model.id }) { index, tabModel ->
                    ReorderableItem(
                        reorderableLazyColumnState,
                        key = tabModel.id
                    ) { isDragging ->
                        val isSelected = tabModel.id == chosenId
                        val chooseThis = { coroutineScope.launch { chosenId = tabModel.id } }
                        Row(
                            Modifier
                                .height(IntrinsicSize.Min).draggableHandle()
                        ) {
                            Tab(
                                selected = isSelected,
                                enabled = !isSelected,
                                modifier = Modifier
                                    .padding(horizontal = 2.dp)
                                    .clip(RoundedCornerShape(topEnd = 4.dp, topStart = 4.dp)),
                                onClick = { chooseThis() },
                            ) {
                                Box(Modifier.width(IntrinsicSize.Min)) {
                                    val textColor = if (isSelected) Color.Black else Color.Blue
                                    val maxTabWidth = 300.dp
                                    Row(
                                        verticalAlignment = Alignment.CenterVertically,
                                        modifier = Modifier.widthIn(max = maxTabWidth)
                                            .padding(vertical = 4.dp)
                                            .padding(start = 12.dp),
                                    ) {
                                        Spacer(Modifier.width(6.dp))
                                        val textStyle =
                                            TextStyle(color = textColor, fontSize = 18.sp)
                                        Box(Modifier.weight(1f)) {
                                            Text(
                                                text = tabModel.id.toString(),
                                                style = textStyle,
                                                maxLines = 1,
                                                softWrap = false,
                                                modifier = Modifier
                                                    .horizontalScroll(rememberScrollState())
                                            )
                                        }
                                        val expectedCloseSize by animateDpAsState(if (tabModels.size > 1) 48.dp else 8.dp)
                                        Box(Modifier.width(expectedCloseSize)) {
                                            androidx.compose.animation.AnimatedVisibility(
                                                tabModels.size > 1,
                                                enter = scaleIn() + fadeIn(),
                                                exit = scaleOut() + fadeOut(),
                                            ) {
                                                IconButton(
                                                    onClick = {
                                                    },
                                                    enabled = tabModels.size > 1,
                                                ) {
                                                    Icon(
                                                        imageVector = Icons.Default.Close,
                                                        tint = textColor,
                                                        contentDescription = "Close tab"
                                                    )
                                                }
                                            }
                                        }
                                    }
                                }
                            }
                        }
                    }
                }
                item {
                    IconButton(
                        onClick = {
                            coroutineScope.launch {
                                tabModels += Tab(tabModels.size.toLong())
                            }
                        }
                    ) {
                        Icon(
                            Icons.Default.Add,
                            contentDescription = "Add tab",
                        )
                    }
                }
            }
        }
    }
}

@zhelenskiy
Copy link
Author

For 2.0.0 The library seems not to be working properly at all.

Screen.Recording.2024-05-05.at.04.45.34.mov

@zhelenskiy
Copy link
Author

With 2.0.1, the previous case works well.

Screen.Recording.2024-05-05.at.04.47.24.mov

@Calvin-LL
Copy link
Owner

Thank you! I will take a look.

@zhelenskiy
Copy link
Author

Just for my internal planning, when are you going to take a look at the issue?

@Calvin-LL
Copy link
Owner

Just got up. I think I see what was causing it last night, something in my library is causing your composable to constantly recompose. I will fix it in the next 48 hours but likely sooner. Sorry about the long timeline, I'm playing DEF CON CTF Qualifier this weekend.

@zhelenskiy
Copy link
Author

Well, my experience shows that average ETA of response in GitHub issues is several days in the best case and several weeks or months in the others. So, having responses within several minutes (7 minutes for the first reply and several for the rest) is insanely fast. Your eta for the fix is also tens of times less than the average. So there're no complaints!

Good luck at the Qualifier! Let all the flags be with you 🙏

@Calvin-LL
Copy link
Owner

Just released v2.0.3! Let me know if that fixes it. Thank you again for reporting this issue 🙏.

@zhelenskiy
Copy link
Author

It partially fixes. It is not stuck at the point anymore, but it stops after exchanging with the next element.

Screen.Recording.2024-05-07.at.16.30.35.mov

@Calvin-LL
Copy link
Owner

It partially fixes. It is not stuck at the point anymore, but it stops after exchanging with the next element.

Screen.Recording.2024-05-07.at.16.30.35.mov

Oh strange I'll take a look.

@liltof
Copy link

liltof commented May 7, 2024

Hi,
I also have the same issue (but with a vertical lazylist), and another one, I'm not sure if it's linked to this issue so I just comment here, maybe it can help:
When I try to move an item to the last position, it's impossible, the item doesn't move, there is no unexpected stop but maybe the problem is the same.
I can drag the last item to above, but not the before last to bellow
Thank you for your time, this library is the best drag and drop lib I found, despite these issues 👍

@Calvin-LL
Copy link
Owner

Hi, I also have the same issue (but with a vertical lazylist), and another one, I'm not sure if it's linked to this issue so I just comment here, maybe it can help: When I try to move an item to the last position, it's impossible, the item doesn't move, there is no unexpected stop but maybe the problem is the same. I can drag the last item to above, but not the before last to bellow Thank you for your time, this library is the best drag and drop lib I found, despite these issues 👍

That sounds like a different issue. Can you post a screen recording?

@liltof
Copy link

liltof commented May 7, 2024

Here it is
(the true/false) at the begining is just debug, I was checking "isDraging" to see if I did something wrong

Screen_recording_20240507_193401.mp4

@Calvin-LL
Copy link
Owner

Here it is (the true/false) at the begining is just debug, I was checking "isDraging" to see if I did something wrong

Screen_recording_20240507_193401.mp4

That looks like a different issue. Does the ReorderableItem on the last item have (enable = true)? It would help me debug this if you can provide a simplified version of your code that can reproduce this bug in a new issue 🙏.

@liltof
Copy link

liltof commented May 7, 2024

No the last one is like the other ones, nothin special:
ReorderableItem(reorderableLazyListState, key = item.metaId.hashCode()) {

I will try to make a simple example and come back here

@Calvin-LL
Copy link
Owner

It partially fixes. It is not stuck at the point anymore, but it stops after exchanging with the next element.

Screen.Recording.2024-05-07.at.16.30.35.mov

Should work in v2.0.4

@LOOHP
Copy link

LOOHP commented May 7, 2024

Here it is (the true/false) at the begining is just debug, I was checking "isDraging" to see if I did something wrong

Screen_recording_20240507_193401.mp4

I'm also having this problem where an item couldn't push the bottom item up and when the first item swaps position with the second item, the first item loses the drag. Both in a vertical lazy list. And I've just tried with 2.0.4.

(Upgraded from 1.5.2, which the first item losing drag problem already existed)

@liltof
Copy link

liltof commented May 7, 2024

Ok I made an example, AND making it I found a workaround
So the bottom issue seems to only happen when the last item is at the bottom of the screen (at least on android)
So what I did is add a contentPadding to the list (which is exactly what I need in the specs I have so it's ok for me)
the contentPadding is commented in the code below
But I still have the issue with the top item, even in 2.0.4

package com.example.testsapplication

import android.os.Bundle
import androidx.activity.ComponentActivity
import androidx.activity.compose.setContent
import androidx.compose.foundation.ExperimentalFoundationApi
import androidx.compose.foundation.layout.Row
import androidx.compose.foundation.layout.fillMaxSize
import androidx.compose.foundation.layout.fillMaxWidth
import androidx.compose.foundation.layout.height
import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.lazy.LazyColumn
import androidx.compose.foundation.lazy.items
import androidx.compose.foundation.lazy.rememberLazyListState
import androidx.compose.material3.Scaffold
import androidx.compose.material3.Surface
import androidx.compose.material3.Text
import androidx.compose.runtime.getValue
import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.remember
import androidx.compose.runtime.setValue
import androidx.compose.runtime.toMutableStateList
import androidx.compose.ui.Modifier
import androidx.compose.ui.unit.dp
import com.example.testsapplication.ui.theme.TestsApplicationTheme
import sh.calvin.reorderable.ReorderableItem
import sh.calvin.reorderable.rememberReorderableLazyListState

class MainActivity : ComponentActivity() {
    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        setContent {
            TestsApplicationTheme {
                Scaffold(modifier = Modifier.fillMaxSize()) { innerPadding ->
                    val listState = rememberLazyListState()
                    var itms by remember {
                        mutableStateOf(
                            listOf(
                            "a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l", "m", "n", "o")
                        )
                    }
                    val reorderableLazyListState = rememberReorderableLazyListState(lazyListState = listState) { from, to ->
                        val fromObject = itms[from.index]
                        val toObject = itms[to.index]
                        val newlist = itms.toMutableStateList()
                        newlist[from.index] = toObject
                        newlist[to.index] = fromObject
                        itms = newlist.toList()
                    }


                    LazyColumn(
                        modifier = Modifier
                            .padding(innerPadding)
                            .fillMaxSize(),
                        state = listState, // contentPadding = PaddingValues(bottom = 50.dp)
                    ) {

                        items(items = itms, key = {  item ->
                            item.hashCode()
                        }){ item->
                            ReorderableItem(reorderableLazyListState, key = item.hashCode()) { isDragging ->
                                val elevation = if (isDragging) 4.dp else 0.dp
                                Surface(shadowElevation = elevation) {
                                    Row(modifier = Modifier.fillMaxWidth().longPressDraggableHandle()) {
                                        Text(text = "$isDragging")
                                        Text(text = item, modifier = Modifier
                                            .padding(10.dp)
                                            .height(50.dp)
                                            )
                                    }
                                }

                            }
                        }
                    }
                }
            }
        }
    }
}

@zhelenskiy
Copy link
Author

Nothing changed for me with 2.0.4. @Calvin-LL

Screen.Recording.2024-05-07.at.22.39.39.mov

@Calvin-LL
Copy link
Owner

Calvin-LL commented May 7, 2024

@zhelenskiy I finally manage to reliably reproduce this bug. I have some mildly bad news. As this library currently works right now there's no way for me to fix it until Compose Foundation v1.7.0 gets into Compose Multiplatform (See #4 and #26). But you can add contentPadding = PaddingValues(horizontal = 1.dp) to the LazyRow as a workaround.

Compose Foundation 1.6.0-alpha08 to 1.6.0 took almost 3 months, the latest Compose Multiplatform 1.6.2 uses Compose Foundation 1.6.4 which is a 20-day gap. So expect Compose Foundation v1.7.0 to be in Compose Multiplatform in around 4 months at the current speed.

Unnecessary Details

When the first item swaps with the second, because of the small size difference in this case, the first item is pushed off the screen. Once the item is off screen, LazyRow disposes that item, dragging state is lost.

The Workaround

Adding a small content padding with contentPadding = PaddingValues(horizontal = 1.dp) keeps a little bit of the dragging item on screen so the dragging state is not lost.

@liltof
Copy link

liltof commented May 8, 2024

That solution is OK for me, it also works with my LazyColumn (with a top contentPadding to 1.dp), I'll wait for the new Compose Foundation just to have the final solution but for the moment that's good. thanks! 👍

@LOOHP
Copy link

LOOHP commented May 8, 2024

The workaround works great, solved both top and bottom issues. Would be nice if I knew this sooner. Thanks!

@zhelenskiy
Copy link
Author

Thank you for the workaround! However, I have another issue. When I start moving a tab backwards and forwards a lot, it disappears at some point in time. And find it always being placed at position 1.

Screen.Recording.2024-05-08.at.03.43.24.mov

@Calvin-LL
Copy link
Owner

I never considered the case where an item could be dragged off the viewport. (The gesture detector keeps reporting the items position even when it's outside the window) I will fix that.

@zhelenskiy
Copy link
Author

Again, just for internal planning, is there any ETA?

@Calvin-LL
Copy link
Owner

So, I'm looking for the update for the version of Foundation to become available! Thanks.
Are you going to try using alpha versions for the library?

As far as I can't tell there's no way to use any other versions of Compose Foundation than the one that is bundled into Compose Multiplatform 😭. If this library was Android only then I would be able to release an alpha version with the foundation alpha.

@zhelenskiy
Copy link
Author

I'm not sure, but it seems you can add the Compose dependencies explicitly, specifying their version in version catalogs. However, if there are no multiplatform artifacts, then we can do nothing 😿.

@Calvin-LL Calvin-LL added the Waiting for Compose v1.7.0 (try 2.4.0-beta01) Waiting for Compose Foundation v1.7.0 to be in Compose Multiplatform label May 9, 2024
@Calvin-LL
Copy link
Owner

Good news! v1.7.0 is being released in June according to https://android-developers.googleblog.com/2024/05/whats-new-in-jetpack-compose-at-io-24.html, we should see v1.7.0 land in Compose Multiplatform some time in June/July.

@Calvin-LL
Copy link
Owner

Calvin-LL commented Jul 29, 2024

I just released 2.4.0-alpha01 yesterday. It uses v1.7.0 so it should fix this issue. I'm waiting for Compose Multiplatform v1.7.0 to be stable to release a stable version.

@zhelenskiy
Copy link
Author

Hi! I see the same issue reproducible in 2.3.0.

@Calvin-LL
Copy link
Owner

Can you share a screen recording?

@zhelenskiy
Copy link
Author

Screen.Recording.2024-07-30.at.03.24.32.mov

@Calvin-LL
Copy link
Owner

Calvin-LL commented Jul 30, 2024

Any chance you're using launch in onMove? Try removing the launch.

https://github.com/Calvin-LL/Reorderable?tab=readme-ov-file#faq

@zhelenskiy
Copy link
Author

Seems no chance.

@Calvin-LL
Copy link
Owner

Which version did this start happeneing? I haven't changed the core functions for a while. If this is recent, it might be an issue with compose.

@zhelenskiy
Copy link
Author

I don't know, in this project I started with 2.3.0.

@Calvin-LL
Copy link
Owner

Calvin-LL commented Jul 30, 2024

I'm so sorry to have to ask you once again for a minimal reproducible example 😢 . I can't seem to trigger this issue on my end.

@zhelenskiy
Copy link
Author

One second, I'm polishing the rest, will supply within 30 minutes.

@zhelenskiy
Copy link
Author

It took more time than expected, so I will send it later today.

@Calvin-LL
Copy link
Owner

Thank you ❤️

@zhelenskiy
Copy link
Author

zhelenskiy commented Jul 31, 2024

Check https://github.com/zhelenskiy/PersonalVault there are actually two artifacts of using reorderable. One is this, the second one is that some items start jumping vertically on appearance. If I remove reorderable, everything works fine.

Screen.Recording.2024-07-31.at.06.37.02.mov

The more exact place is this one. To reproduce it, you need to add press '+' and add some folders and files in them.

@Calvin-LL
Copy link
Owner

Thanks. I will try to figure this out tomorrow.

@Calvin-LL
Copy link
Owner

Ah I see what's going on, if you print the key passed to the ReorderableItem, you can see that the keys change depending on the order. That's why the dragging item jumps during reordering. Consider using some sort of id as key, something that's unique to each item.

@zhelenskiy
Copy link
Author

Oh, hm... I see. I'll try, thanks!

@zhelenskiy
Copy link
Author

zhelenskiy commented Aug 3, 2024

@Calvin-LL Thank you! This really worked for the first problem but not for the last one shown in the video above. I checked that it is still reproducible after fixing the IDs. Furthermore, if I remove reorderable, everything works fine. Check current master branch`, please.

@zhelenskiy
Copy link
Author

Apply the following to master to remove the library usage:

diff --git a/composeApp/src/commonMain/kotlin/spaceScreen/SpaceScreen.kt b/composeApp/src/commonMain/kotlin/spaceScreen/SpaceScreen.kt
index 8016b31..20cac4c 100644
--- a/composeApp/src/commonMain/kotlin/spaceScreen/SpaceScreen.kt
+++ b/composeApp/src/commonMain/kotlin/spaceScreen/SpaceScreen.kt
@@ -388,7 +388,7 @@ fun FileSystem(
                         }
                     }
                     itemsIndexed(children, key = { _, child -> child.fileId }) { index, child ->
-                        ReorderableItem(reorderableLazyListState, key = child.fileId) { isDragging ->
+//                        ReorderableItem(reorderableLazyListState, key = child.fileId) { isDragging ->
                             fun moveToShort(newPath: List<Int>) {
                                 val success = moveTo(
                                     spaceStructure = spaceStructure, oldPath = path,
@@ -416,7 +416,7 @@ fun FileSystem(
                                 }
                             }
 
-                            val elevation by animateDpAsState(if (isDragging) 4.dp else 0.dp)
+//                            val elevation by animateDpAsState(if (isDragging) 4.dp else 0.dp)
 
                             when (child) {
                                 is Directory -> DirectoryRow(
@@ -437,8 +437,8 @@ fun FileSystem(
                                     copyTo = ::copyToShort,
                                     isNewDirectory = index == children.lastIndex && isLastElementNew,
                                     onCreationFinished = { isLastElementNew = false },
-                                    modifier = Modifier.draggableHandle(enabled = blockingFiles.isEmpty() && !viewOnly),
-                                    elevation = elevation,
+//                                    modifier = Modifier.draggableHandle(enabled = blockingFiles.isEmpty() && !viewOnly),
+//                                    elevation = elevation,
                                     onChange = { element, mapping ->
                                         changeRootByChangesInsideCurrentFolder(mapping) {
                                             if (element != null) set(index, element) else removeAt(index)
@@ -459,8 +459,8 @@ fun FileSystem(
                                     copyTo = ::copyToShort,
                                     isNewFile = index == children.lastIndex && isLastElementNew,
                                     onCreationFinished = { isLastElementNew = false },
-                                    modifier = Modifier.draggableHandle(enabled = blockingFiles.isEmpty() && !viewOnly),
-                                    elevation = elevation,
+//                                    modifier = Modifier.draggableHandle(enabled = blockingFiles.isEmpty() && !viewOnly),
+//                                    elevation = elevation,
                                     onChange = { element, mapping ->
                                         changeRootByChangesInsideCurrentFolder(mapping) {
                                             if (element != null) set(index, element) else removeAt(index)
@@ -468,7 +468,7 @@ fun FileSystem(
                                     }
                                 )
                             }
-                        }
+//                        }
                     }
                 }
                 VerticalScrollbar(directoryContentListState)

@Calvin-LL
Copy link
Owner

Calvin-LL commented Aug 3, 2024

I'll have a look when I get up tomorrow morning US central time

@zhelenskiy
Copy link
Author

I think there might be some issue with animatePlacement done at the start.

@Calvin-LL
Copy link
Owner

Yeah that's what I'm thinking. But it should be fine if all the IDs are different?

@Calvin-LL
Copy link
Owner

I see what's happening. So when you first open a the folder, Because the width in RowScope.CardTextField is 0 in the first frame, it looks like this:

image

Then after the text width is measured, it looks like this:

image

Because animatePlacement also animate item resizing.

I would either not show the row until after the text width is measured or disable animatePlacement just for the first frame.

@zhelenskiy
Copy link
Author

Thanks, I fixed jumping by not showing the row until the text is measured.

@Calvin-LL
Copy link
Owner

v2.4.0 is out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Waiting for Compose v1.7.0 (try 2.4.0-beta01) Waiting for Compose Foundation v1.7.0 to be in Compose Multiplatform
Projects
None yet
Development

No branches or pull requests

4 participants