-
Notifications
You must be signed in to change notification settings - Fork 0
[Fix/#146] 3차 QA #151
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/#146] 3차 QA #151
Conversation
…ID-MEMENTO into fix/#146-hyoeun-qa # Conflicts: # app/src/main/java/org/memento/presentation/component/MementoDialog.kt # app/src/main/java/org/memento/presentation/setting/SettingViewModel.kt # app/src/main/java/org/memento/presentation/today/TodayScreen.kt # app/src/main/java/org/memento/presentation/today/TodayViewModel.kt # app/src/main/java/org/memento/presentation/todo/TodoViewModel.kt
imtaejugkim
left a comment
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.
드앤드 미쳤다~~ 고생했습니다
| override suspend fun patchDragAndDrop( | ||
| toDoId: Int, | ||
| dragAndDrop: DragAndDrop, | ||
| ): Result<Unit> { | ||
| return runCatching { | ||
| Log.d("drag", "📡 Sending PATCH to /todo/$toDoId/reorder with body = $dragAndDrop") | ||
|
|
||
| todoDataSource.patchDragAndDrop( | ||
| toDoId = toDoId, | ||
| requestDragAndDropDto = dragAndDrop.toData(), | ||
| ).handleBaseResponse().getOrThrow() | ||
| } | ||
| } |
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.
디버깅용 로그죠?
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.
넹.. 서버 잘 연결되면 지우겠습니다!
| is UiState.Failure -> { | ||
| showExistDialog = false | ||
| showExistDialog = true | ||
| settingViewModel.resetTagUiState() | ||
| } |
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.
요것은 왜 false로 초기화됐다 다시 true가 된건가요?
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.
그거 아직 tag patch되는 api로 이것저것 테스트해보다가 안지워서 저렇게 작성된 것 같습니다
지금 해당 api가 조금 이상해서 서버쌤들께 말씀드렸어요! 추후 해결되면 분기처리 제대로 하려구요ㅠㅠ
| colorHex = selectedColorCode.toString(), | ||
| ) | ||
| } else { | ||
| Timber.tag("D").d(text) |
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.
나중에 지우기
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.
ㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋ넵!!!!!!!!!!
| itemsIndexed(combinedItems, key = { _, item -> | ||
| when (item) { | ||
| is MementoItem.TodoItem -> "todo_${item.id}" | ||
| is MementoItem.ScheduleItem -> "schedule_${item.id}" | ||
| } | ||
| }) { index, item -> | ||
|
|
||
| val isDragging = | ||
| when (item) { | ||
| is MementoItem.TodoItem -> draggedItemId == item.id | ||
| is MementoItem.ScheduleItem -> false | ||
| } | ||
| val scale by animateFloatAsState(if (isDragging) 1.1f else 1f) | ||
|
|
||
| val canDrag = item is MementoItem.TodoItem | ||
|
|
||
| Box( | ||
| modifier = | ||
| Modifier | ||
| .fillMaxWidth() | ||
| .onGloballyPositioned { coordinates -> | ||
| if (index == 0 && actualItemHeight == 0f) { | ||
| actualItemHeight = coordinates.size.height.toFloat() | ||
| } | ||
| } | ||
| .graphicsLayer( | ||
| scaleX = scale, | ||
| scaleY = scale, | ||
| translationY = if (isDragging) draggedOffsetY else 0f, | ||
| ) | ||
| .zIndex(if (isDragging) 1f else 0f) | ||
| .pointerInput(Unit) { | ||
| detectDragGesturesAfterLongPress( | ||
| onDragStart = { | ||
| draggedItemIndex = index | ||
| isDraggingEnabled = true | ||
| }, | ||
| onDrag = { change, dragAmount -> | ||
| if (isDraggingEnabled) { | ||
| change.consume() | ||
| draggedOffsetY += dragAmount.y | ||
|
|
||
| val itemHeightPx = with(density) { 60.dp.toPx() } | ||
| while (true) { | ||
| val deltaIndices = (draggedOffsetY / itemHeightPx).toInt() | ||
| if (deltaIndices == 0) break | ||
|
|
||
| val targetIndex = | ||
| (draggedItemIndex + deltaIndices) | ||
| .coerceIn(0, combinedItems.size - 1) | ||
|
|
||
| if (targetIndex != draggedItemIndex) { | ||
| viewModel.reorderItems(draggedItemIndex, targetIndex) | ||
| draggedItemIndex = targetIndex | ||
| draggedOffsetY %= itemHeightPx | ||
| } else { | ||
| draggedOffsetY = 0f | ||
| break | ||
| .then( | ||
| if (canDrag) { | ||
| // 캐스팅 | ||
| val todoItem = item as MementoItem.TodoItem | ||
|
|
||
| Modifier.pointerInput(item.id) { | ||
| detectDragGesturesAfterLongPress( | ||
| onDragStart = { | ||
| // ID로 추적 추가 | ||
| draggedItemId = (item as? MementoItem.TodoItem)?.id ?: -1 | ||
| draggedItemIndex = index | ||
| isDraggingEnabled = true | ||
| }, |
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.
드래그 앤 드롭 정말 고생많으셨습니다!
하나 생각해보자면, 현재는 드래그 중에 아이템이 한칸씩 넘어갈 때마다 viewmodel.reorderItems가 투두랑 스케쥴 아이템을 갱신하고 combineItems가 재계산 되서 전체리스트가 계속 리컴포지션이 되는 것 같아요
과도한 리컴포지션이 반복될 수도 있다고 생각되서, 이런 방법은 혹시 어떨지 질문드립니다.
생각한 방법은, 드롭이 완료되기 전에는 임시 temp 데이터를 만들어서 UI상에만 변경시키고, drag end시에 reorder데이터를 반영하는 것은 어떨까 싶습니다.
var tempCombinedItems by remember { mutableStateOf<List<MementoItem>?>(null) }
val uiItems = tempCombinedItems ?: combinedItems이런식으로 uiItems를 임의로 만들어주고,
nDragStart = {
draggedItemId = (item as? MementoItem.TodoItem)?.id ?: -1
draggedItemIndex = index
isDraggingEnabled = true
// temp로 복사
tempCombinedItems = combinedItems.toList()
}데이터 복사하고,
while (kotlin.math.abs(draggedOffsetY) >= totalItemHeight) {
val direction = if (draggedOffsetY > 0) 1 else -1
val currentList = (tempCombinedItems ?: combinedItems).toMutableList()
// 현재 드래그 중인 아이템 index를 temp에서 찾게
val currentDraggedIndex = currentList.indexOfFirst {
it is MementoItem.TodoItem && it.id == draggedItemId
}
if (currentDraggedIndex == -1) break이런식으로 ui에서 사용되고, dragEnd가 되면 patch후에 다시 해당 temp데이터를 비울수도 있을 것 같아요.
한번 확인해주시고 알려주시면 좋을 것 같아요~!
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.
정말 너무 좋은..!!! 성능을 진짜 많이 개선할 수 있을 것 같아요
사실 저도 이 부분이 좋은 로직이라고는 생각하지 않았거든요. 드래그할 때마다 ViewModel의 reorderItems를 호출해서 _todoItems와 _scheduleItems가 매번 업데이트되고, 이게 combinedItems StateFlow를 다시 계산하게 만들어서 전체 LazyColumn이 계속 리컴포지션되는 문제가 있었기때문이죠. 투두 아이템이 많아지면 분명 많이 버벅거릴거라고 생각했어요
해당 방식을 사용하면 드래그 중에는 tempCombinedItems라는 임시 리스트에서만 순서를 바꾸고, 실제 ViewModel은 건드리지 않습니다. 드래그가 완전히 끝났을 때 한 번만 ViewModel을 업데이트 하면 드래그 중에는 UI 레벨에서만 변경이 일어나서 StateFlow 재계산이나 불필요한 리컴포지션이 발생하지 않는다는 성능상 큰 장점이 있을 것 같습니다.
좋은 리뷰 감사합니다! 역시 애햄이!
hwidung
left a comment
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.
이 복잡한 로직을 완성하느라 너무너무 고생 많앗오요 짱!
| .fillMaxWidth(fraction = fraction) | ||
| .aspectRatio(3f / 68f) |
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.
오 ㅎ 급 감탄 ui 잘짠당
| val movedItem = currentList[fromIndex] | ||
|
|
||
| val movedItem = currentList.removeAt(fromIndex) | ||
| if (movedItem !is MementoItem.TodoItem) return |
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.
스케줄은 드앤드 안되고 투두만 드앤드 되는 군뇨~~
✅ 𝗖𝗵𝗲𝗰𝗸-𝗟𝗶𝘀𝘁
📌 𝗜𝘀𝘀𝘂𝗲𝘀
📎𝗪𝗼𝗿𝗸 𝗗𝗲𝘀𝗰𝗿𝗶𝗽𝘁𝗶𝗼𝗻
📷 𝗦𝗰𝗿𝗲𝗲𝗻𝘀𝗵𝗼𝘁
KakaoTalk_20250901_020915047.mp4
💬 𝗧𝗼 𝗥𝗲𝘃𝗶𝗲𝘄𝗲𝗿𝘀
+) 드래그 앤 드롭은 현재 서버응답 문제로 반영은 안됩니다. 아마 리프레시 하시면 원래 상태로 돌아올거에요! 감안하고 확인해주시면 감사하겠습니다
+) 화살표 로직에 대해서 추가 설명 드리겠습니다.
이를 참고하셔서 엣지케이스 찾아주시면 감사하겠습니다.
p.s. 점점 끝이 보인다 화이팅 메둥이들아 ❤️