-
Notifications
You must be signed in to change notification settings - Fork 4
fix: stringFormatter suffix 애니메이션 안정화 #11
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
Changes from all commits
5ffc753
cb62b3f
3f9f3b1
b04a6fb
ed44ebf
1866883
ad01a22
3919256
cfe5018
995b091
324c626
3bef6a1
b0afed8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,7 +12,7 @@ enum TextType: Equatable { | |
| case number(Int) | ||
|
|
||
| init(_ value: Character) { | ||
| if let number = value.wholeNumberValue { | ||
| if let number = value.asciiDigitValue { | ||
| self = .number(number) | ||
| } else { | ||
| self = .string(String(value)) | ||
|
|
@@ -27,6 +27,15 @@ enum TextType: Equatable { | |
| return false | ||
| } | ||
| } | ||
|
|
||
| var digitValue: Int? { | ||
| switch self { | ||
| case .number(let number): | ||
| return number | ||
| case .string: | ||
| return nil | ||
| } | ||
| } | ||
| } | ||
|
|
||
| struct TextColumn: Identifiable, Equatable { | ||
|
|
@@ -39,7 +48,7 @@ struct TextColumn: Identifiable, Equatable { | |
| } | ||
|
|
||
| init(placeholderFor value: Character) { | ||
| if value.wholeNumberValue != nil { | ||
| if value.asciiDigitValue != nil { | ||
| self.init(value: .number(0)) | ||
| } else { | ||
| self.init(value: TextType(value)) | ||
|
|
@@ -49,16 +58,61 @@ struct TextColumn: Identifiable, Equatable { | |
|
|
||
| extension Array where Element == TextColumn { | ||
| mutating func resizeForAnimation(to string: String) { | ||
| let extra = string.count - count | ||
| guard extra != 0 else { return } | ||
| let targetCharacters = Swift.Array<Character>(string) | ||
| // Equal-length updates keep their columns; settingAnimationRange handles value changes. | ||
| guard targetCharacters.count != count else { return } | ||
|
|
||
| if extra > 0 { | ||
| append(contentsOf: string.suffix(extra).map { | ||
| TextColumn(placeholderFor: $0) | ||
| }) | ||
| } else { | ||
| removeLast(-extra) | ||
| let currentColumns = self | ||
| var preservedDigitColumns = currentColumns.filter(\.value.isNumber) | ||
| var preservedTextColumns = currentColumns.filter { !$0.value.isNumber } | ||
| var targetColumns = targetCharacters.map { TextColumn(placeholderFor: $0) } | ||
|
|
||
| for targetIndex in targetCharacters.indices.reversed() { | ||
| guard targetColumns[targetIndex].value.isNumber else { continue } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Minor] Problem: non-digit 문자의 보존을 위해 현재는 역방향 루프로 매칭을 시도하지만, 자릿수가 증가할 때 suffix의 인덱스 위치가 달라져 기존의 TextColumn(새 UUID)이 생성되며 SwiftUI 뷰 노드 보존이 실패할 수 있습니다. Why it matters: 이로 인해 형식 문자열(예: 'ms')의 위치가 일관되지 않게 보일 수 있으며, 애니메이션 중 뷰가 재생성되어 깜빡임이 발생할 수 있습니다. Suggested fix: digit 컬럼처럼 우측 정렬 기준으로 매칭하거나, suffix의 위치를 고정된 인덱스 기준으로 매칭하는 방식으로 구조를 개선해야 합니다. Confidence: Medium Confidence score: 0.85 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 최신 코드의 resizeForAnimation(to:)는 preservedTextColumns에서 lastIndex(where: { $0.value == targetValue })로 값 기반 매칭을 수행하므로, 자릿수가 증가하여 suffix 인덱스가 밀리더라도 동일 값의 기존 TextColumn UUID가 정확히 보존됩니다. 추가된 테스트(resizeForAnimationUsesStablePlaceholders, KeepsFormattedSuffixAligned, WhenShrinking)에서도 suffix(3).map(.id)가 resize 전후로 일치하는 것이 검증되어 있으므로, 이 지적은 현재 코드에는 해당하지 않는 것으로 보입니다. |
||
| guard let preservedColumn = preservedDigitColumns.popLast() else { break } | ||
|
|
||
| targetColumns[targetIndex] = preservedColumn | ||
| } | ||
|
|
||
| for targetIndex in targetCharacters.indices.reversed() { | ||
| let targetValue = TextType(targetCharacters[targetIndex]) | ||
| guard !targetValue.isNumber else { continue } | ||
|
|
||
| // Reverse iteration + lastIndex keeps trailing duplicate separators aligned first. | ||
| if let currentIndex = preservedTextColumns.lastIndex(where: { $0.value == targetValue }) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] preservedTextColumns에서 동일 값의 non-digit 문자가 여러 개일 때(예: 천 단위 쉼표 "," 다수), lastIndex(where:)와 역순 순회 조합이 우측 정렬 매칭을 자연스럽게 수행합니다. 현재 동작은 정확하지만, digit 컬럼의 popLast 기반 우측 정렬과 달리 non-digit 매칭의 우측 정렬 의도가 코드만으로는 파악하기 어렵습니다. 간단한 인라인 주석(예: "// Right-to-left: reverse iteration + lastIndex preserves trailing duplicates first")을 추가해 주시면 유지보수에 도움이 됩니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 동의 — digit 컬럼이 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] preservedTextColumns에서 역순 매칭을 위해 lastIndex(where:)를 사용하는 우측 정렬 매칭 의도가 명확히 이해되도록 주석을 추가하면 향후 코드 이해와 유지보수에 도움이 됩니다. |
||
| targetColumns[targetIndex] = preservedTextColumns.remove(at: currentIndex) | ||
| } else { | ||
| targetColumns[targetIndex] = TextColumn(value: targetValue) | ||
| } | ||
| } | ||
|
|
||
| self = targetColumns | ||
| } | ||
|
|
||
| var digitCount: Int { | ||
| filter(\.value.isNumber).count | ||
| } | ||
|
|
||
| func preservingReelPositions(_ positions: [UUID: Double]) -> [UUID: Double] { | ||
| var nextPositions: [UUID: Double] = [:] | ||
|
|
||
| for column in self { | ||
| guard let digit = column.value.digitValue else { continue } | ||
| nextPositions[column.id] = positions[column.id] ?? Double(digit) | ||
| } | ||
|
|
||
| return nextPositions | ||
| } | ||
|
|
||
| func currentDigitPositions() -> [UUID: Double] { | ||
| var nextPositions: [UUID: Double] = [:] | ||
|
|
||
| for column in self { | ||
| guard let digit = column.value.digitValue else { continue } | ||
| nextPositions[column.id] = Double(digit) | ||
| } | ||
|
|
||
| return nextPositions | ||
| } | ||
|
|
||
| func canAnimateDigitChange(to value: Character, index: Int) -> Bool { | ||
|
|
@@ -67,6 +121,18 @@ extension Array where Element == TextColumn { | |
| return self[index].value.isNumber && TextType(value).isNumber | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] TextType(value).isNumber를 판정하기 위해 매번 TextType 인스턴스를 초기화하고 있습니다. value.asciiDigitValue != nil을 직접 평가하여 불필요한 인스턴스 생성을 피하는 편이 더 효율적입니다. |
||
| } | ||
|
|
||
| func needsUpdate(to value: Character, index: Int) -> Bool { | ||
| guard indices.contains(index) else { return false } | ||
|
|
||
| return self[index].value != TextType(value) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] self[index].value != TextType(value) 비교 시에도 매번 Character를 기반으로 TextType 객체를 생성합니다. TextType 내부에 Character와의 동등성 비교 연산자(==)를 오버로딩하거나 패턴 매칭을 활용해 객체 생성을 생략할 수 있습니다. |
||
| } | ||
|
|
||
| func digitOrdinal(at index: Int) -> Int? { | ||
| guard indices.contains(index), self[index].value.isNumber else { return nil } | ||
|
|
||
| return self[..<index].filter(\.value.isNumber).count | ||
| } | ||
|
|
||
| mutating func set(_ value: Character, index: Int) { | ||
| set(TextType(value), index: index) | ||
| } | ||
|
|
@@ -77,3 +143,16 @@ extension Array where Element == TextColumn { | |
| self[index].value = value | ||
| } | ||
| } | ||
|
|
||
| private extension Character { | ||
| var asciiDigitValue: Int? { | ||
| guard unicodeScalars.count == 1, | ||
| let scalar = unicodeScalars.first, | ||
| scalar.value >= 48, | ||
| scalar.value <= 57 else { | ||
| return nil | ||
| } | ||
|
|
||
| return Int(scalar.value - 48) | ||
| } | ||
| } | ||
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.
[Suggestion] 현재 guard는 문자 수가 같으면 즉시 반환하므로, 같은 길이에서 digit/non-digit 패턴이 달라지는 경우(예: 소수점 위치 이동) 컬럼 재배치가 생략됩니다. 실제 숫자 포맷팅에서 이런 상황은 극히 드물고, 값 업데이트는
settingAnimationRange에서 처리되므로 당장 문제가 되지는 않지만, 향후 edge case를 고려하여 이 가정을 주석으로 남겨 두시면 유지보수에 도움이 될 수 있습니다.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.
수정 완료: 같은 길이 업데이트는 컬럼 재배치 없이
settingAnimationRange에서 값/type 변경을 처리한다는 의도를 주석으로 남겼습니다. 이 PR의 깜빡임 원인인 길이 증감 경로는 별도 테스트로 고정했습니다. 검증:swift test.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.
동의합니다. 문자 개수가 같더라도 소수점의 위치가 바뀌거나 숫자와 문자의 배열 패턴이 달라지는 특수한 환경에서는 각 칸의 속성이 맞지 않아 화면이 잘못 그려질 수 있습니다. 지금 당장은 발생하기 어려운 상황이더라도, 이 동작 방식에 대한 명확한 주석을 남기거나 추후 문자 패턴의 일치 여부까지 검사하도록 보완하는 것이 유지보수에 도움이 됩니다.