Skip to content

fix: recalculate grid span count after rotation #4894

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.

Sign up for GitHub

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

Closed
himu-gupta wants to merge 4 commits into from
Closed

fix: recalculate grid span count after rotation #4894

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
89fa4da
fix: recalculate grid span count after rotation
himu-gupta Jun 18, 2026
c889d0b
chore: add calens file
himu-gupta Jun 18, 2026
afe11e5
chore: update calens file
himu-gupta Jun 24, 2026
8f8534b
fix: address grid span review comments
himu-gupta Jun 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions changelog/unreleased/4884
View file
Open in desktop
Comment thread
joragua marked this conversation as resolved.
Outdated
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
Bugfix: Grid mode after rotation

Grid mode now recalculates its column count after device rotation, keeping
file tiles in the correct layout for the new orientation.

https://github.com/owncloud/android/issues/4884
21 changes: 18 additions & 3 deletions ...pp/src/main/java/com/owncloud/android/presentation/files/filelist/MainFileListFragment.kt
View file
Open in desktop

@dj4oC dj4oC Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Original file line number Diff line number Diff line change
Expand Up @@ -335,6 +335,9 @@ class MainFileListFragment : Fragment(),
override fun onConfigurationChanged(newConfig: Configuration) {
super.onConfigurationChanged(newConfig)
updateConfigDependentSizes()
if (::viewType.isInitialized && viewType == ViewType.VIEW_TYPE_GRID) {
updateRecyclerViewLayoutForCurrentViewType()
}
Comment thread
joragua marked this conversation as resolved.
Outdated
}

private fun updateConfigDependentSizes() {
Expand Down Expand Up @@ -998,17 +1001,30 @@ class MainFileListFragment : Fragment(),
}

override fun onViewTypeListener(viewType: ViewType) {
this.viewType = viewType
binding.optionsLayout.viewTypeSelected = viewType

if (viewType == ViewType.VIEW_TYPE_LIST) {
mainFileListViewModel.setListModeAsPreferred()
layoutManager.spanCount = 1

} else {
mainFileListViewModel.setGridModeAsPreferred()
layoutManager.spanCount = ColumnQuantity(requireContext(), R.layout.grid_item).calculateNoOfColumns()
}

updateRecyclerViewLayoutForCurrentViewType()
}

private fun updateRecyclerViewLayoutForCurrentViewType() {
if (!::layoutManager.isInitialized || !::fileListAdapter.isInitialized) {
return
}
Comment thread
joragua marked this conversation as resolved.
Outdated

layoutManager.spanCount = when (viewType) {
ViewType.VIEW_TYPE_LIST -> 1
ViewType.VIEW_TYPE_GRID -> ColumnQuantity(requireContext(), R.layout.grid_item).calculateNoOfColumns()
}
layoutManager.invalidateSpanAssignments()
binding.recyclerViewMainFileList.requestLayout()
Comment thread
joragua marked this conversation as resolved.
Outdated
fileListAdapter.notifyItemRangeChanged(0, fileListAdapter.itemCount)
Comment on lines +1016 to 1020

@dj4oC dj4oC Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

Expand Down Expand Up @@ -1675,4 +1691,3 @@ class MainFileListFragment : Fragment(),
}
}
}