Conversation
📝 WalkthroughWalkthroughEditOptionPrice.kt의 레이아웃을 단순화하고 입력 정규화 및 통화 포맷 변환 매핑을 원본 텍스트 기준으로 재작성했습니다. 하단 구분선은 drawWithCache로 그려진 커스텀 언더라인으로 대체되었고, 입력값은 정수화된 문자열(또는 빈 문자열)로 전달됩니다. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8분 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
cmj7271
left a comment
cmj7271
left a comment
There was a problem hiding this comment.
스트링 객체 재생성의 경우는 안드로이드 런타임이랑 컴파일러의 가비지 컬렉터가 잘 처리해준다네요
블로그 에 나와있는 최적화 등을 사용한다카더라..
그리고 컴파일러 입장에서 1초에 5~6자 생성하는건 ms 단위로 동작하는 입장에서는 무지 느리겠다는 생각도 들었어요
아래 내용은 PR 과는 무관하긴 합니다.
p2: textWidth 라는 변수값이 매번 변하고, 이 값을 OptionTextField, HorizontalDivider 가 모두 사용하고 있어서, UI 를 그리는 과정에서 Layout 과정을 다시 수행하게 돼요.
이에 대해서 BasicTextField 가 입력받은 텍스트의 길이에 따라 알아서 늘어난다는 점,
drawBehind 을 사용하면, Layout 과정을 생략한다는 점에서 최적화가 가능할거 같아요.
decorationBox 가 textField 주변을 꾸미는 역할을 한다고 나와있다는 점에서 여기서 처리하면 딱일거 같아요~
| val adjusted = when { | ||
| newValue.isEmpty() -> "" | ||
| newValue.all { it == '0' } -> "0" | ||
| newValue.startsWith("0") -> newValue.dropWhile { it == '0' } | ||
| else -> newValue | ||
| } |
There was a problem hiding this comment.
P1: 복잡한 분기 처리 대신, 입력값을 Int로 변환 후 다시 String으로 바꾸면 앞부분의 0 처리를 훨씬 직관적이고 간단하게 구현할 수 있을 것 같아욤
| val adjusted = when { | |
| newValue.isEmpty() -> "" | |
| newValue.all { it == '0' } -> "0" | |
| newValue.startsWith("0") -> newValue.dropWhile { it == '0' } | |
| else -> newValue | |
| } | |
| val adjusted = newValue.toIntOrNull()?.toString() ?: "" |
좋아용~
연산량이 많은 추가로 |
- as-is : 필드 입력 한 번이라도 발생 시 true - to-be : 각 필드 입력값과 초기값 일치하는지 비교
| textStyle = textStyle, | ||
| onFocusChanged = onFocusChanged, | ||
| enabled = enabled, | ||
| modifier = Modifier.width(textWidth), |
There was a problem hiding this comment.
P1: 근데 OptionTextField 너비를 Modifier.widthIn(42.dp) 주고 텍스필드 정렬을 textAlign = TextAlign.End 해준 다음, 지금 방식처럼 drawWithCache를 통해 언더라인 그리도록 하면 density와 measurer를 통해 textWidth를 계산하지 않아도 최소 너비를 유지하면서 텍스트 길이에 늘어나도록 할 수 있을 것 같은데 지금 방식을 사용하신 이유가 있나용??
There was a problem hiding this comment.
헐랭 정렬 쓰는거 넘 좋은 방법같은데 몰라서 못썼씁니다!!!
근데 적용해보니 문제가 있어요
기존 : textWidth로 OptionTextField width 제한
OptionTextField의 width는 무조건 textWidth와 일치drawWithCache로 그리는 언더라인의 길이는OptionTextField의 width를 따라감 > 텍스트 폭과 일치
변경 : OptionTextField width 제한 없이, 텍스트만 우측 정렬
OptionTextField의 width는BasicTextField의 최소 너비 (최소 너비가 꽤 길어요 숫자 9자리 입력한 것 훨씬 초과)drawWithCache로 그리는 언더라인의 길이가 입력된 텍스트 길이와 맞춰지지 않음
결국은 BasicTextField의 최소 너비 때문에 textWidth을 구하는 건 불가피한 것 같아요.
다만 구한 textWidth를 OptionTextField 자체에 적용하기 보다는
OptionTextField에 파라미터로 전달 > 내부에서 drawWithCache에 적용해 언더라인 그릴 때에만 사용되게 하는 편이 비용이 적을 것 같아서 해봤슴니다!
OptionTextField에 폭 제한 제거 +textStyle에TextAlign.End적용drawWithCache에서 underline 길이 구할 때만textWidth사용
@Composable
private fun OptionTextField(
value: String,
onValueChanged: (String) -> Unit,
imeAction: ImeAction,
transformation: VisualTransformation,
textStyle: TextStyle,
onFocusChanged: (Boolean) -> Unit,
enabled: Boolean,
textWidth: Dp,
modifier: Modifier = Modifier,
) {
val focusManager = LocalFocusManager.current
val keyboardController = LocalSoftwareKeyboardController.current
val colors = PotiTheme.colors
BasicTextField(
value = value,
onValueChange = onValueChanged,
modifier = modifier
.onFocusChanged { focusState ->
onFocusChanged(focusState.isFocused)
}
.drawWithCache {
val minPx = 42.dp.toPx()
val textPx = textWidth.toPx()
val underlinePx = max(minPx, textPx)
val strokePx = 2.dp.toPx()
val yOffset = strokePx / 2 - 4.dp.toPx()
onDrawBehind {
val y = size.height - yOffset
drawLine(
color = colors.gray300,
start = Offset(size.width - underlinePx, y),
end = Offset(size.width, y),
strokeWidth = strokePx,
cap = StrokeCap.Round,
)
}
},
textStyle = textStyle.copy(
color = PotiTheme.colors.black,
textAlign = TextAlign.End,
),
There was a problem hiding this comment.
텍스트필드에 IntrinsicSize.Min 적용해줘도 최소 너비 설정이 안되나욤??
There was a problem hiding this comment.
헐 된다..! 입력값 없을 때 클릭이 안되어버려서 widthIn 2dp로 커서 위치만 잡아줫습니다!!
There was a problem hiding this comment.
P3: 이 방법도 괜찮은데 여기서도 text.toIntOrNull()?.toMoneyString() ?: text 사용해줄 수도 있을듯?? 근데 귀찮다면.. 굳이 고치진 않아도 될 것 같아여
There was a problem hiding this comment.
P3: offset이 text.length보다 커질 일은 없을 것 같지만 만약의 상황을 대비해 >=를 통해 방어 코드를 넣는 것도 괜찮을 것 같아요. 이것도 그냥 추천..
There was a problem hiding this comment.
P3: 파라미터 text랑 헷갈릴수도 있어서 originalText 라던가.. 그렇게 바꾸는 것도 나쁘지 않을 것 같아욤
|
|
||
| return offset - commasBeforeCursor | ||
| } | ||
| } |
There was a problem hiding this comment.
P3: 요기도 전에 반복문 사용해서 콤마 개수 세도 성능상 큰 문제 없을 것 같다고 코리 달았던 것 같은데,, 최적화를 좀 더 해보자면 반복문 대신 위에 originalToTransformed처럼 수학 계산식을 사용할 수 있을 것 같은 느낌,,!!
override fun transformedToOriginal(offset: Int): Int {
if (offset >= transformedText.length) return originalText.length
val rightOffset = transformedText.length - offset
val commas = rightOffset / 4
return originalText.length - (rightOffset - commas)
}예를 들어서 콤마는 뒤에서부터 4글자마다 하나씩 포함되기 때문에 이런 식으로 콤마 개수를 한 번에 계산할 수도 있을 것 같아여
There was a problem hiding this comment.
헐랭 4로 나눠버리다니 대박 짱이에여.. 방어코드도 !!!
- as-is : OptionTextField에 modifier width로 textWidth 전달 - to-be : OptionTextField 폭제한 제거, TextAlign.End 적용, textWidth는 파라미터로 전달해 underline 그리는 데만 사용
- originalText/transformedText으로 변수명 변경 - 얼리 리턴 조건식 방어적으로 작성 - transformedToOriginal 계산식 단순화
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@app/src/main/java/com/poti/android/presentation/party/create/component/EditOptionPrice.kt`:
- Around line 204-221: In the filter implementation of PriceVisualTransformation
(method filter in EditOptionPrice.kt) there is a typo in the variable name
numbersAtferCursor — rename it to numbersAfterCursor everywhere it's declared
and referenced (including the calculation for numbersAfterCursor and the later
usage in originalToTransformed and any other offsets) so the identifier is
consistent and compiles.
🧹 Nitpick comments (2)
app/src/main/java/com/poti/android/presentation/party/create/component/EditOptionPrice.kt (2)
155-174: 언더라인y좌표 계산이 컴포저블 영역 밖으로 그려질 수 있습니다.Line 161에서
yOffset = strokePx / 2 - 4.dp.toPx()는 약-3dp가 되고, Line 164에서y = size.height - yOffset는size.height + 3dp가 됩니다. 즉 언더라인이 컴포저블 하단 경계 아래 약 3dp 지점에 그려집니다.Compose에서는 기본적으로
clipToBounds가 false이므로 시각적으로는 보이겠지만, 부모 레이아웃의 클리핑 정책에 따라 잘릴 수 있습니다. 의도된 동작이라면 괜찮으나,yOffset계산의 의미가 직관적이지 않아 간단한 주석을 추가하시는 것을 권장드립니다.또한
42.dp,2.dp,4.dp등 하드코딩된 수치가 여러 개 있는데, 의미 있는 상수(private val UNDERLINE_MIN_WIDTH = 42.dp등)로 추출하면 가독성이 좋아질 것 같습니다.
196-198:decorationBox가 단순히innerTextField()만 호출하고 있습니다.이 경우
decorationBox파라미터를 아예 생략하셔도 기본 동작이 동일합니다. 불필요한 코드를 제거하면 가독성이 살짝 향상됩니다.♻️ 불필요한 decorationBox 제거
singleLine = true, visualTransformation = transformation, - decorationBox = { innerTextField -> - innerTextField() - }, enabled = enabled,
- 텍스트필드에 widthIn 적용
| onValueChange = onValueChanged, | ||
| modifier = modifier | ||
| .width(IntrinsicSize.Min) | ||
| .widthIn(2.dp) |
There was a problem hiding this comment.
P1: 텍스트필드에는 최소 너비를 2.dp로 하고 밑줄 최소 길이를 42.dp로 준 이유가 있나요?? 뭔가 UX 상 밑줄이 있는 영역까지 다 터치가 되어야 자연스러울 것 같은데 지금은 밑줄이 42.dp지만 2.dp에 해당하는 부분만 터치가 돼서 조금 어색한 느낌이 있는 것 같아요.
또한 이렇게 했을 때 만약 옵션 이름이 길다면 텍스트필드 너비는 2.dp일 뿐이라 밑줄 위까지 넘어올수가 있어요.
그래서 제 생각에는 그냥 텍스트필드 자체에 widthIn(42.dp)를 주고 밑줄은 딱 텍스트필드 길이 만큼만 생기도록 하는게 나을 것 같은데 어떻게 생각하시나요??
There was a problem hiding this comment.
맞아요ㅠㅠ 최소 42로 주는게 맞습니다!! 제가 좀 꼬아서 생각했던거같아요
There was a problem hiding this comment.
P1: 혹시 가격 입력하고 지워보셨나욤,, singleLine = true 설정에서 가로 스크롤을 지원하다보니 지울 때 텍스트 앞에 여백 때문에 자꾸 텍스트가 스크롤되어서 잘려보이는 문제가 있네요ㅠㅠ 그래서 singleLine말고 maxLines = 1을 사용해야 할 것 같아요
There was a problem hiding this comment.
IntrinsicSize.Min을 추가하며 생긴 이슈 같은데 제가 제대로 확인을 못했어요
꼼꼼히 봐주셔서 감사합니다!! ㅠㅠㅠ
* [Refactor/#143] UiState 수정 * [Refactor/#143] 뷰모델 수정 * [Chore/#143] import문, 기호 수정 * [Chore/#143] ktlint 적용 * [Chore/#143] 미사용 코드 삭제 * [Chore/#143] 오탈자 수정 * [Fix/#143] launchedEffect 키 변경 * [Fix/#143] 불필요한 modifier 속성 제거 및 기기 대응 높이 제한 * [Fix/#143] showHint 플래그 제거 * [Refactor/#143] isEditBtnInScreen 판단 로직 변경 - as-is : screenHeight, bottomButtonHeight 활용 - to-be : layoutBottom 활용 * [Refactor/#143] artistQuery 변수명 변경 - artistQuery to artistSearchKeyword * [Refactor/#143] 등록 뷰모델 초기화 로직 변경 - as-is : Route에서 수동 초기화 - to-be : PartyCreate 중첩 그래프 추가로 Create 뷰와 뷰모델 생명주기 연동 > 자동 초기화 * [Refactor/#142] 다이얼로그 노출 조건 변경 - as-is : 필드 입력 한 번이라도 발생 시 true - to-be : 각 필드 입력값과 초기값 일치하는지 비교 * [Refactor/#143] isTouched 프로퍼티 삭제 * [Chore/#143] ktlint 적용 * [Fix/#143] 멤버 가격 검사 로직 수정 - as-is : 새 입력값을 검사해야 하는데, 기존값을 검사해 에러상태 업데이트 - to-be : 새 입력값으로 에러 검사
3 participants
Related issue 🛠️
Work Description ✏️
Screenshot 📸
default.mp4
Uncompleted Tasks 😅
To Reviewers 📢
입력마다 스트링 객체 생성되는 점이 걸리긴 합니다만 다른 방법이 떠오르지 않네욤,, UX를 위해 희생해도 되는 부분인지 아님 더 좋은 방법이 있을까욤?
Summary by CodeRabbit
버그 수정
UI 개선
행동 변경