From a912417c898c82f678da8b83dd6c257c0ed1a7ea Mon Sep 17 00:00:00 2001 From: Gibran Chevalley Date: Wed, 28 Feb 2024 16:36:53 +0100 Subject: [PATCH 1/3] Fix GalleryFragment missing context crash When navigating between fragment, the MenuGalleryFragment sometimes had a reference on the bad GalleryFragment instance. That instance did not have access to a context and therefore crashed when doing certain actions on it --- .../drive/ui/menu/MenuGalleryFragment.kt | 76 ++++++++++--------- 1 file changed, 39 insertions(+), 37 deletions(-) diff --git a/app/src/main/java/com/infomaniak/drive/ui/menu/MenuGalleryFragment.kt b/app/src/main/java/com/infomaniak/drive/ui/menu/MenuGalleryFragment.kt index 09e786f05f..fb6062f7f5 100644 --- a/app/src/main/java/com/infomaniak/drive/ui/menu/MenuGalleryFragment.kt +++ b/app/src/main/java/com/infomaniak/drive/ui/menu/MenuGalleryFragment.kt @@ -27,7 +27,6 @@ import androidx.core.view.marginBottom import androidx.core.view.marginTop import androidx.fragment.app.Fragment import androidx.fragment.app.activityViewModels -import com.google.android.material.appbar.AppBarLayout import com.infomaniak.drive.R import com.infomaniak.drive.databinding.FragmentMenuGalleryBinding import com.infomaniak.drive.databinding.MultiSelectLayoutBinding @@ -43,60 +42,63 @@ class MenuGalleryFragment : Fragment() { private var binding: FragmentMenuGalleryBinding by safeBinding() private val mainViewModel: MainViewModel by activityViewModels() - private var galleryFragment = GalleryFragment() + private var galleryFragment: GalleryFragment? = null override fun onCreateView(inflater: LayoutInflater, container: ViewGroup?, savedInstanceState: Bundle?): View { - binding = FragmentMenuGalleryBinding.inflate(inflater, container, false).apply { - swipeRefreshLayout.setOnRefreshListener { galleryFragment.onRefreshGallery() } - } + return FragmentMenuGalleryBinding.inflate(inflater, container, false).also { binding = it }.root + } + + override fun onViewCreated(view: View, savedInstanceState: Bundle?) = with(binding) { + super.onViewCreated(view, savedInstanceState) + + ViewCompat.requestApplyInsets(galleryListCoordinator) - binding.multiSelectLayout.apply { + instantiateGalleryFragment() + + swipeRefreshLayout.setOnRefreshListener { galleryFragment?.onRefreshGallery() } + + multiSelectLayout.apply { selectAllButton.isGone = true setMultiSelectClickListeners() } - return binding.root - } + galleryFragment!!.menuGalleryBinding = binding - private fun MultiSelectLayoutBinding.setMultiSelectClickListeners() = with(galleryFragment) { - toolbarMultiSelect.setNavigationOnClickListener { closeMultiSelect() } - moveButtonMultiSelect.setOnClickListener { onMoveButtonClicked() } - deleteButtonMultiSelect.setOnClickListener { deleteFiles() } - menuButtonMultiSelect.setOnClickListener { - onMenuButtonClicked( - multiSelectBottomSheet = GalleryMultiSelectActionsBottomSheetDialog(), - areAllFromTheSameFolder = false, - ) + appBar.addOnOffsetChangedListener { _, verticalOffset -> + galleryFragment!!.setScrollbarTrackOffset(appBar.totalScrollRange + verticalOffset) } - } - - override fun onViewCreated(view: View, savedInstanceState: Bundle?) { - super.onViewCreated(view, savedInstanceState) - ViewCompat.requestApplyInsets(binding.galleryListCoordinator) + adjustFastScrollBarScrollRange() + observeAndDisplayNetworkAvailability( + mainViewModel = mainViewModel, + noNetworkBinding = noNetworkInclude, + noNetworkBindingDirectParent = galleryContentLinearLayout, + ) + } + private fun instantiateGalleryFragment() { with(childFragmentManager) { (findFragmentByTag(GalleryFragment.TAG) as? GalleryFragment)?.let { galleryFragment = it } ?: run { + val fragment = galleryFragment ?: GalleryFragment().also { galleryFragment = it } beginTransaction() - .replace(R.id.galleryFragmentView, galleryFragment, GalleryFragment.TAG) - .commit() + .replace(R.id.galleryFragmentView, fragment, GalleryFragment.TAG) + .commitNow() } } + } - galleryFragment.menuGalleryBinding = binding - - binding.appBar.addOnOffsetChangedListener(AppBarLayout.OnOffsetChangedListener { _, verticalOffset -> - galleryFragment.setScrollbarTrackOffset(binding.appBar.totalScrollRange + verticalOffset) - }) - - adjustFastScrollBarScrollRange() - observeAndDisplayNetworkAvailability( - mainViewModel = mainViewModel, - noNetworkBinding = binding.noNetworkInclude, - noNetworkBindingDirectParent = binding.galleryContentLinearLayout, - ) + private fun MultiSelectLayoutBinding.setMultiSelectClickListeners() = with(galleryFragment) { + toolbarMultiSelect.setNavigationOnClickListener { galleryFragment?.closeMultiSelect() } + moveButtonMultiSelect.setOnClickListener { galleryFragment?.onMoveButtonClicked() } + deleteButtonMultiSelect.setOnClickListener { galleryFragment?.deleteFiles() } + menuButtonMultiSelect.setOnClickListener { + galleryFragment?.onMenuButtonClicked( + multiSelectBottomSheet = GalleryMultiSelectActionsBottomSheetDialog(), + areAllFromTheSameFolder = false, + ) + } } private fun adjustFastScrollBarScrollRange() = with(binding) { @@ -106,7 +108,7 @@ class MenuGalleryFragment : Fragment() { appBar.addOnOffsetChangedListener { _, verticalOffset -> val margin = appBar.totalScrollRange + verticalOffset + bottomNavigationOffset - galleryFragment.setScrollbarTrackOffset(margin) + galleryFragment!!.setScrollbarTrackOffset(margin) } } } From 0675a47feb61de99d8632a68845e4aaf6b2000ed Mon Sep 17 00:00:00 2001 From: Gibran Chevalley Date: Thu, 29 Feb 2024 10:38:38 +0100 Subject: [PATCH 2/3] Limit side effects by avoiding the useless global scope galleryFragment instance --- .../drive/ui/menu/MenuGalleryFragment.kt | 62 +++++++++---------- 1 file changed, 31 insertions(+), 31 deletions(-) diff --git a/app/src/main/java/com/infomaniak/drive/ui/menu/MenuGalleryFragment.kt b/app/src/main/java/com/infomaniak/drive/ui/menu/MenuGalleryFragment.kt index fb6062f7f5..3b3e6dd8f4 100644 --- a/app/src/main/java/com/infomaniak/drive/ui/menu/MenuGalleryFragment.kt +++ b/app/src/main/java/com/infomaniak/drive/ui/menu/MenuGalleryFragment.kt @@ -42,33 +42,46 @@ class MenuGalleryFragment : Fragment() { private var binding: FragmentMenuGalleryBinding by safeBinding() private val mainViewModel: MainViewModel by activityViewModels() - private var galleryFragment: GalleryFragment? = null - override fun onCreateView(inflater: LayoutInflater, container: ViewGroup?, savedInstanceState: Bundle?): View { return FragmentMenuGalleryBinding.inflate(inflater, container, false).also { binding = it }.root } - override fun onViewCreated(view: View, savedInstanceState: Bundle?) = with(binding) { + override fun onViewCreated(view: View, savedInstanceState: Bundle?) { super.onViewCreated(view, savedInstanceState) + ViewCompat.requestApplyInsets(binding.galleryListCoordinator) + val galleryFragment = addGalleryFragment() + setUi(galleryFragment) + } - ViewCompat.requestApplyInsets(galleryListCoordinator) - - instantiateGalleryFragment() + private fun addGalleryFragment(): GalleryFragment { + with(childFragmentManager) { + (findFragmentByTag(GalleryFragment.TAG) as? GalleryFragment)?.let { + return it + } ?: run { + val galleryFragment = GalleryFragment() + beginTransaction() + .replace(R.id.galleryFragmentView, galleryFragment, GalleryFragment.TAG) + .commit() + return galleryFragment + } + } + } - swipeRefreshLayout.setOnRefreshListener { galleryFragment?.onRefreshGallery() } + private fun setUi(galleryFragment: GalleryFragment) = with(binding) { + swipeRefreshLayout.setOnRefreshListener { galleryFragment.onRefreshGallery() } multiSelectLayout.apply { selectAllButton.isGone = true - setMultiSelectClickListeners() + setMultiSelectClickListeners(galleryFragment) } - galleryFragment!!.menuGalleryBinding = binding + galleryFragment.menuGalleryBinding = binding appBar.addOnOffsetChangedListener { _, verticalOffset -> - galleryFragment!!.setScrollbarTrackOffset(appBar.totalScrollRange + verticalOffset) + galleryFragment.setScrollbarTrackOffset(appBar.totalScrollRange + verticalOffset) } - adjustFastScrollBarScrollRange() + adjustFastScrollBarScrollRange(galleryFragment) observeAndDisplayNetworkAvailability( mainViewModel = mainViewModel, noNetworkBinding = noNetworkInclude, @@ -76,39 +89,26 @@ class MenuGalleryFragment : Fragment() { ) } - private fun instantiateGalleryFragment() { - with(childFragmentManager) { - (findFragmentByTag(GalleryFragment.TAG) as? GalleryFragment)?.let { - galleryFragment = it - } ?: run { - val fragment = galleryFragment ?: GalleryFragment().also { galleryFragment = it } - beginTransaction() - .replace(R.id.galleryFragmentView, fragment, GalleryFragment.TAG) - .commitNow() - } - } - } - - private fun MultiSelectLayoutBinding.setMultiSelectClickListeners() = with(galleryFragment) { - toolbarMultiSelect.setNavigationOnClickListener { galleryFragment?.closeMultiSelect() } - moveButtonMultiSelect.setOnClickListener { galleryFragment?.onMoveButtonClicked() } - deleteButtonMultiSelect.setOnClickListener { galleryFragment?.deleteFiles() } + private fun MultiSelectLayoutBinding.setMultiSelectClickListeners(galleryFragment: GalleryFragment) = with(galleryFragment) { + toolbarMultiSelect.setNavigationOnClickListener { closeMultiSelect() } + moveButtonMultiSelect.setOnClickListener { onMoveButtonClicked() } + deleteButtonMultiSelect.setOnClickListener { deleteFiles() } menuButtonMultiSelect.setOnClickListener { - galleryFragment?.onMenuButtonClicked( + onMenuButtonClicked( multiSelectBottomSheet = GalleryMultiSelectActionsBottomSheetDialog(), areAllFromTheSameFolder = false, ) } } - private fun adjustFastScrollBarScrollRange() = with(binding) { + private fun adjustFastScrollBarScrollRange(galleryFragment: GalleryFragment) = with(binding) { val bottomNavigationOffset = with((activity as MainActivity).getBottomNavigation()) { layoutParams.height + marginBottom + marginTop + 10.toPx() } appBar.addOnOffsetChangedListener { _, verticalOffset -> val margin = appBar.totalScrollRange + verticalOffset + bottomNavigationOffset - galleryFragment!!.setScrollbarTrackOffset(margin) + galleryFragment.setScrollbarTrackOffset(margin) } } } From 2dad63ed59963fc35a6e41014b9e5dac08232a39 Mon Sep 17 00:00:00 2001 From: Kevin Boulongne Date: Tue, 5 Mar 2024 14:45:52 +0100 Subject: [PATCH 3/3] =?UTF-8?q?Simplify=20`fun=20addGalleryFragment(?= =?UTF-8?q?=E2=80=A6)`=20code?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../drive/ui/menu/MenuGalleryFragment.kt | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/app/src/main/java/com/infomaniak/drive/ui/menu/MenuGalleryFragment.kt b/app/src/main/java/com/infomaniak/drive/ui/menu/MenuGalleryFragment.kt index 3b3e6dd8f4..65edde1eba 100644 --- a/app/src/main/java/com/infomaniak/drive/ui/menu/MenuGalleryFragment.kt +++ b/app/src/main/java/com/infomaniak/drive/ui/menu/MenuGalleryFragment.kt @@ -53,22 +53,19 @@ class MenuGalleryFragment : Fragment() { setUi(galleryFragment) } - private fun addGalleryFragment(): GalleryFragment { - with(childFragmentManager) { - (findFragmentByTag(GalleryFragment.TAG) as? GalleryFragment)?.let { - return it - } ?: run { - val galleryFragment = GalleryFragment() + private fun addGalleryFragment(): GalleryFragment = with(childFragmentManager) { + return@with (findFragmentByTag(GalleryFragment.TAG) as? GalleryFragment) ?: run { + GalleryFragment().also { beginTransaction() - .replace(R.id.galleryFragmentView, galleryFragment, GalleryFragment.TAG) + .replace(R.id.galleryFragmentView, it, GalleryFragment.TAG) .commit() - return galleryFragment } } } private fun setUi(galleryFragment: GalleryFragment) = with(binding) { - swipeRefreshLayout.setOnRefreshListener { galleryFragment.onRefreshGallery() } + + swipeRefreshLayout.setOnRefreshListener(galleryFragment::onRefreshGallery) multiSelectLayout.apply { selectAllButton.isGone = true @@ -82,6 +79,7 @@ class MenuGalleryFragment : Fragment() { } adjustFastScrollBarScrollRange(galleryFragment) + observeAndDisplayNetworkAvailability( mainViewModel = mainViewModel, noNetworkBinding = noNetworkInclude,