fix: recalculate grid span count after rotation#4894
Conversation
|
Hi @himu-gupta! Thanks for opening this PR! 🙌🏻 Some improvements here before the CR:
Let us know if you have any doubts and we will help you! I will start with the code review when all changes are done 🍻 |
4a35526 to
bc59543
Compare
Signed-off-by: Himanshu Gupta <himu.gupta23@gmail.com>
Signed-off-by: Himanshu Gupta <himu.gupta23@gmail.com>
bc59543 to
c889d0b
Compare
Thanks @joragua, I have done the requested changes. |
|
Thanks for your contribution @himu-gupta!! Let me tell you how the process is. Basically, there are two phases after the PR is correct (signed commits, separated changelog, conventional commits correct...):
For sure, all CI checks must be green as well, otherwise merging will be blocked. Code review will start soon, keep tuned!! |
Thanks for the insight |
|
@joragua is anything pending from my end? |
|
I'll take care of it as soon as possible and I'll ping you once the CR is ready! Stay tuned! 🙌🏻 @himu-gupta |
joragua
left a comment
There was a problem hiding this comment.
CR finished! 🚀 Some comments here @himu-gupta
Signed-off-by: Himanshu Gupta <himu.gupta23@gmail.com>
Signed-off-by: Himanshu Gupta <himu.gupta23@gmail.com>
|
Thanks @joragua, addressed the CR in two new commits on top without force-pushing: |
joragua
left a comment
There was a problem hiding this comment.
Just two easy comments and it's ready! Thanks for your patience @himu-gupta!
Would it be possible to squash the two commits about calens into a single one? We use to have only one commit for the calens entry in each PR. Also, I'd rename the last commit to make it more descriptive. The idea would be to have only three commits in this PR:
fix: recalculate grid span count after rotationchore: add calens filerefactor: remove unnecessary grid recalculation logic
| The grid mode column count has been recalculated after device rotation, keeping | ||
| file tiles in the correct layout for the new orientation. |
There was a problem hiding this comment.
I'd re-write the description like this:
| The grid mode column count has been recalculated after device rotation, keeping | |
| file tiles in the correct layout for the new orientation. | |
| The grid mode column count has been recalculated after device rotation, keeping | |
| file layouts aligned with the new orientation. |
DeepDiver1975
left a comment
There was a problem hiding this comment.
Maintainer review (automated, on behalf of @DeepDiver1975) — fix looks correct and addresses #4884 well.
Correctness — verified
FileDisplayActivityandFolderPickerActivityboth declareandroid:configChanges="orientation|screenSize"in the manifest, so on rotation the activity is not recreated andonConfigurationChangedis the right (and only) hook. CallingupdateRecyclerViewLayoutForCurrentViewType()there is the correct fix.- Setting
this.viewType = viewTypeinonViewTypeListeneris more than a refactor — it fixes a real latent bug: theviewTypefield was previously only set ininitViews()and never updated when the user toggled list/grid. Without this, a rotation after toggling would recompute the span count for the stale view type. Good catch. viewTypeislateinitbut is always initialized ininitViews()before the view exists, so reading it inonConfigurationChangedis safe (config changes can't precede view creation). No lifecycle/NPE risk.- The extracted
when (viewType)over the two-value enum is exhaustive (noelseneeded) and idiomatic Kotlin. Behavior inonViewTypeListeneris unchanged vs. the previous inlineif.
Nit (non-blocking)
- For a
StaggeredGridLayoutManagerspan-count change,notifyItemRangeChanged(0, itemCount)works, butlayoutManager.invalidateSpanAssignments()(optionally followed byrequestLayout()) is the more robust idiom for guaranteeing stale span gaps are cleared after a column-count change. Since the change is verified against the reported overlap and the previous reviewer confirmednotifyItemRangeChangedis sufficient, I'm fine leaving it. Worth a mental note if any residual overlap is ever reported. - The PR description mentions "Invalidate staggered-grid span assignments", but the code does not call
invalidateSpanAssignments()— it relies onnotifyItemRangeChanged. Minor wording mismatch only; no code change needed.
Changelog — changelog/unreleased/4894 is present and matches the calens convention (TEMPLATE.md): Bugfix: type ≤ 80 chars, present-perfect-passive body, issue + PR URLs. ✅
Note: @joragua's earlier change-requests (filename → 4894, present-perfect-passive description, PR link) appear addressed in the current diff. No new blocking issues from my side; this is good to go once the maintainer's outstanding commit-squash request is satisfied.
Summary
Fixes #4884
Verification
JAVA_HOME="/Applications/Android Studio.app/Contents/jbr/Contents/Home" ANDROID_HOME="/Users/himu/Library/Android/sdk" ./gradlew --no-daemon --console=plain :owncloudApp:compileOriginalDebugKotlingit diff --check