fix: recalculate grid span count after rotation

[himu-gupta]

Summary

• Recalculate the file-list grid span count after configuration changes.
• Keep the fragment's selected view type in sync when toggling between list and grid.
• Invalidate staggered-grid span assignments and request a relayout after the mode/orientation update.
• Add an unreleased changelog entry.

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:compileOriginalDebugKotlin
• git diff --check

[CLAassistant]

All committers have signed the CLA.

[joragua]

Hi @himu-gupta! Thanks for opening this PR! 🙌🏻

Some improvements here before the CR:

1. Do not to forget to use Conventional commits for the message
2. Commits must be GPG/PGP signed, and it'd be great if each commit include a Signed-off-by line
3. I'd separate the calens into a different commit with the message: chore: add calens file

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 🍻

[himu-gupta]

Hi @himu-gupta! Thanks for opening this PR! 🙌🏻

Some improvements here before the CR:

1. Do not to forget to use Conventional commits for the message
2. Commits must be GPG/PGP signed, and it'd be great if each commit include a Signed-off-by line
3. I'd separate the calens into a different commit with the message: chore: add calens file

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 🍻

Thanks @joragua, I have done the requested changes.

[jesmrec]

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...):

1. Code review: team will review the code you pushed and make suggestions for improvement. Usually, @joragua takes care of it.

2. QA: Once code review is approved, the feature/fix will be tested to detect defects or unexpected behaviors. I will take care of that.

For sure, all CI checks must be green as well, otherwise merging will be blocked.

Code review will start soon, keep tuned!!

[himu-gupta]

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...):

1. Code review: team will review the code you pushed and make suggestions for improvement. Usually, @joragua takes care of it.
2. QA: Once code review is approved, the feature/fix will be tested to detect defects or unexpected behaviors. I will take care of that.

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

[himu-gupta]

@joragua is anything pending from my end?

[joragua]

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]

CR finished! 🚀 Some comments here @himu-gupta

[himu-gupta]

Thanks @joragua, addressed the CR in two new commits on top without force-pushing:

• afe11e5 updates the calens file name/content to match the template and adds the PR link.
• 8f8534b removes the extra initialization guards and the explicit invalidateSpanAssignments() / requestLayout() calls, relying on notifyItemRangeChanged() for the refresh.

[joragua]

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:

1. fix: recalculate grid span count after rotation
2. chore: add calens file
3. refactor: remove unnecessary grid recalculation logic

[joragua]

I'd re-write the description like this:

Suggested change

	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]

Maintainer review (automated, on behalf of @DeepDiver1975) — fix looks correct and addresses #4884 well.

Correctness — verified

• FileDisplayActivity and FolderPickerActivity both declare android:configChanges="orientation|screenSize" in the manifest, so on rotation the activity is not recreated and onConfigurationChanged is the right (and only) hook. Calling updateRecyclerViewLayoutForCurrentViewType() there is the correct fix.
• Setting this.viewType = viewType in onViewTypeListener is more than a refactor — it fixes a real latent bug: the viewType field was previously only set in initViews() 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.
• viewType is lateinit but is always initialized in initViews() before the view exists, so reading it in onConfigurationChanged is safe (config changes can't precede view creation). No lifecycle/NPE risk.
• The extracted when (viewType) over the two-value enum is exhaustive (no else needed) and idiomatic Kotlin. Behavior in onViewTypeListener is unchanged vs. the previous inline if.

Nit (non-blocking)

• For a StaggeredGridLayoutManager span-count change, notifyItemRangeChanged(0, itemCount) works, but layoutManager.invalidateSpanAssignments() (optionally followed by requestLayout()) 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 confirmed notifyItemRangeChanged is 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 on notifyItemRangeChanged. 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.

[jesmrec]

ey @himu-gupta how are you? don't hesitate to ping us if you need nothing. The current PR is almost ready (just a couple of tiny comments that @joragua suggested) to move to QA. Let us know! same for the other PR you opened.

[dj4oC]

Fixes issue #4884: recalculates the RecyclerView grid span count on onConfigurationChanged (not just on view-type toggle) so grid layout stays correct after device rotation.

Findings

• Security: no findings (no network/IPC/credential/deserialization surface touched — see inline discussion)
• Stability: no findings (viewType is always initialized before any reachable onConfigurationChanged call, per manifest configChanges + fragment lifecycle ordering; no threading issues)
• Performance: 1 finding — see inline suggestion (unconditional adapter rebind fires on more than just rotation)
• Test coverage: 0% on touched lines, below the 85% bar — test proposed inline

Verdict

Changes requested (test coverage gap; performance nit is non-blocking but recommended)

Note: I did not take the existing "automated maintainer review" comment from @DeepDiver1975 on this thread at face value — each of the four passes above (security/stability/performance/coverage) formed its own independent judgment from the diff and surrounding code.

🤖 Automated review by Claude Code (security · stability · performance · coverage)

---

Generated by Claude Code

[dj4oC]

Performance (minor-moderate, non-blocking): this now runs on every onConfigurationChanged, not just rotation — FileDisplayActivity/FolderPickerActivity declare configChanges covering keyboard visibility and uiMode (dark/light) too. In VIEW_TYPE_LIST, spanCount is always 1 and never actually changes, so those triggers still force a full notifyItemRangeChanged(0, itemCount) rebind. FileListAdapter.onBindViewHolder does a synchronous ThumbnailsCacheManager.getBitmapFromDiskCache() disk read per row, so this is real (if currently small) main-thread I/O cost on every qualifying config change, not just rotation.

Suggest guarding the rebind on an actual span-count change:

Suggested change

	layoutManager.spanCount = when (viewType) {
	ViewType.VIEW_TYPE_LIST -> 1
	ViewType.VIEW_TYPE_GRID -> ColumnQuantity(requireContext(), R.layout.grid_item).calculateNoOfColumns()
	}
	fileListAdapter.notifyItemRangeChanged(0, fileListAdapter.itemCount)
	val newSpanCount = when (viewType) {
	ViewType.VIEW_TYPE_LIST -> 1
	ViewType.VIEW_TYPE_GRID -> ColumnQuantity(requireContext(), R.layout.grid_item).calculateNoOfColumns()
	}
	if (newSpanCount != layoutManager.spanCount) {
	layoutManager.spanCount = newSpanCount
	fileListAdapter.notifyItemRangeChanged(0, fileListAdapter.itemCount)
	}

---

Generated by Claude Code

[dj4oC]

Test coverage: 0% on the lines this PR touches (onConfigurationChanged, onViewTypeListener, the new updateRecyclerViewLayoutForCurrentViewType()). No test file exists for MainFileListFragment at all (checked src/test and src/androidTest), and the PR's "Verification" section only lists manual compileDebugKotlin/assembleDebug runs — that proves it compiles, not that the rotation fix behaves or stays fixed. This is UI-visible behavior (the bug being fixed, #4884, is itself a rotation/layout regression), so per house rules it needs Espresso/Robolectric coverage — below the 85% bar required to approve.

Proposed test (Espresso, matching this repo's existing androidTest Koin + mockk(relaxed = true) + ActivityScenario conventions used for sibling fragments):

package com.owncloud.android.presentation.files.filelist

import android.content.res.Configuration
import androidx.recyclerview.widget.RecyclerView
import androidx.recyclerview.widget.StaggeredGridLayoutManager
import androidx.test.core.app.ActivityScenario.launch
import androidx.test.core.app.ApplicationProvider
import com.owncloud.android.R
import com.owncloud.android.domain.files.model.FileListOption
import com.owncloud.android.presentation.files.ViewType
import com.owncloud.android.presentation.files.operations.FileOperationsViewModel
import com.owncloud.android.sharing.shares.ui.TestShareFileActivity
import com.owncloud.android.testutil.OC_ACCOUNT
import com.owncloud.android.testutil.OC_FOLDER
import io.mockk.mockk
import org.junit.Assert.assertEquals
import org.junit.Assert.assertTrue
import org.junit.Before
import org.junit.Test
import org.koin.androidx.viewmodel.dsl.viewModel
import org.koin.core.context.startKoin
import org.koin.core.context.stopKoin
import org.koin.dsl.module

class MainFileListFragmentRotationTest {

    private lateinit var fragment: MainFileListFragment
    private lateinit var mainFileListViewModel: MainFileListViewModel

    @Before
    fun setUp() {
        val context = ApplicationProvider.getApplicationContext<android.content.Context>()
        mainFileListViewModel = mockk(relaxed = true)
        stopKoin()
        startKoin {
            context
            allowOverride(override = true)
            modules(module {
                viewModel { mainFileListViewModel }
                viewModel { mockk<FileOperationsViewModel>(relaxed = true) }
            })
        }

        fragment = MainFileListFragment.newInstance(
            initialFolderToDisplay = OC_FOLDER,
            fileListOption = FileListOption.ALL_FILES,
            accountName = OC_ACCOUNT.name,
        )

        launch(TestShareFileActivity::class.java).onActivity { it.startFragment(fragment) }
    }

    private fun currentSpanCount(): Int =
        (fragment.view!!.findViewById<RecyclerView>(R.id.recyclerViewMainFileList)
            .layoutManager as StaggeredGridLayoutManager).spanCount

    @Test
    fun rotating_in_grid_mode_recalculates_span_count() {
        fragment.onViewTypeListener(ViewType.VIEW_TYPE_GRID)
        fragment.onConfigurationChanged(Configuration(fragment.resources.configuration).apply {
            orientation = Configuration.ORIENTATION_LANDSCAPE
        })
        assertTrue("expected grid span > 1 after rotation", currentSpanCount() > 1)
    }

    @Test
    fun rotating_in_list_mode_keeps_span_count_at_one() {
        fragment.onViewTypeListener(ViewType.VIEW_TYPE_LIST)
        fragment.onConfigurationChanged(Configuration(fragment.resources.configuration).apply {
            orientation = Configuration.ORIENTATION_LANDSCAPE
        })
        assertEquals(1, currentSpanCount())
    }

    @Test
    fun toggling_view_type_is_remembered_across_subsequent_rotation() {
        fragment.onViewTypeListener(ViewType.VIEW_TYPE_LIST)
        fragment.onViewTypeListener(ViewType.VIEW_TYPE_GRID)
        fragment.onConfigurationChanged(fragment.resources.configuration)
        assertTrue("view type toggle should persist and drive grid span on rotation", currentSpanCount() > 1)
    }
}

Security and stability passes found nothing blocking (see review summary).

---

Generated by Claude Code

[dj4oC]

• Security: no findings
• Stability: no findings
• Performance: 1 finding — see review
• Test coverage: 0% on touched lines, below 85% — test proposed in review
• TODOs found: none
• Dependency touched: no
• CI status: all green
• Stale review: no

🤖 Automated review by Claude Code (security · stability · performance · coverage)

---

Generated by Claude Code