IPC 717 Refactor Payment Review Screen for not recreating after orientation change #835
Conversation
… after orientation change IPC-717
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the payment review screen to prevent recreation after orientation changes, improving user experience by maintaining the screen state during device rotation.
Key changes:
- Implement state preservation for ViewPager position and height across orientation changes
- Add proper handling for keyboard state restoration after rotation
- Update ViewPager2 and RecyclerView dependencies to newer versions
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| ghs_fragment_review.xml | Adds constraint height attribute to improve layout behavior |
| ReviewViewModel.kt | Adds properties to store pager position and height state |
| ReviewFragment.kt | Implements comprehensive orientation change handling with state preservation |
| libs.versions.toml | Updates ViewPager2 and RecyclerView dependency versions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| val heightDiff = root.rootView.height - visible | ||
| imeVisibleNow = heightDiff > root.rootView.height * KEYBOARD_VISIBILITY_RATIO // keyboard threshold | ||
| imeWasVisible = | ||
| heightDiff > root.rootView.height * KEYBOARD_VISIBILITY_RATIO // keyboard threshold |
There was a problem hiding this comment.
The assignment to imeWasVisible on line 470-471 appears to only track the current state but doesn't preserve the previous value. Consider using a logical OR to maintain the visibility state: imeWasVisible = imeWasVisible || (heightDiff > root.rootView.height * KEYBOARD_VISIBILITY_RATIO)
| heightDiff > root.rootView.height * KEYBOARD_VISIBILITY_RATIO // keyboard threshold | |
| imeWasVisible || (heightDiff > root.rootView.height * KEYBOARD_VISIBILITY_RATIO) // keyboard threshold |
Copilot uses AI. Check for mistakes.
| val h = when { | ||
| viewModel.pagerHeight > 0 -> viewModel.pagerHeight | ||
| binding.pager.height > 0 -> binding.pager.height | ||
| else -> 1 |
There was a problem hiding this comment.
Using a magic number 1 as fallback height could cause layout issues. Consider using a more meaningful fallback value like ViewGroup.LayoutParams.WRAP_CONTENT or defining a named constant for the minimum height.
| else -> 1 | |
| else -> ViewGroup.LayoutParams.WRAP_CONTENT |
Copilot uses AI. Check for mistakes.
| constrainHeight( | ||
| binding.pager.id, | ||
| abs( | ||
| binding.pager.measuredHeight - ((dragHandle?.height | ||
| ?: 0) + bottomLayout.height) | ||
| ) |
There was a problem hiding this comment.
Using abs() on the height calculation could mask negative values that indicate layout issues. If dragHandle.height + bottomLayout.height exceeds pager.measuredHeight, this suggests a problem that should be handled explicitly rather than hidden with absolute value.
| constrainHeight( | |
| binding.pager.id, | |
| abs( | |
| binding.pager.measuredHeight - ((dragHandle?.height | |
| ?: 0) + bottomLayout.height) | |
| ) | |
| val calculatedHeight = binding.pager.measuredHeight - ((dragHandle?.height ?: 0) + bottomLayout.height) | |
| if (calculatedHeight < 0) { | |
| android.util.Log.w("ReviewFragment", "Calculated pager height is negative: $calculatedHeight. This may indicate a layout issue.") | |
| } | |
| constrainHeight( | |
| binding.pager.id, | |
| maxOf(0, calculatedHeight) |
Copilot uses AI. Check for mistakes.
1 participant
… after orientation change
IPC-717