From d017774cef508e7af8d0135ff899b960b18dc390 Mon Sep 17 00:00:00 2001 From: OxygenCobalt Date: Thu, 3 Sep 2020 18:13:20 -0600 Subject: [PATCH] Fix bug with permission checking Fix a bug with the permission checking where the MusicRepository would return NO_PERMS to the LoadingFragment faster than the creation of the view, causing the error message to never display. Permission checks are now handled by LoadingFragment instead. --- .../oxycblt/auxio/loading/LoadingFragment.kt | 86 ++++++++++++------- .../oxycblt/auxio/loading/LoadingViewModel.kt | 4 +- .../oxycblt/auxio/music/MusicRepository.kt | 15 ---- .../auxio/music/processing/MusicLoader.kt | 2 +- app/src/main/res/drawable/ripple.xml | 4 +- app/src/main/res/layout/fragment_main.xml | 2 +- 6 files changed, 60 insertions(+), 53 deletions(-) diff --git a/app/src/main/java/org/oxycblt/auxio/loading/LoadingFragment.kt b/app/src/main/java/org/oxycblt/auxio/loading/LoadingFragment.kt index 857263233..06b166770 100644 --- a/app/src/main/java/org/oxycblt/auxio/loading/LoadingFragment.kt +++ b/app/src/main/java/org/oxycblt/auxio/loading/LoadingFragment.kt @@ -1,6 +1,7 @@ package org.oxycblt.auxio.loading import android.Manifest +import android.content.pm.PackageManager import android.os.Bundle import android.util.Log import android.view.LayoutInflater @@ -8,6 +9,7 @@ import android.view.View import android.view.ViewGroup import androidx.activity.result.ActivityResultLauncher import androidx.activity.result.contract.ActivityResultContracts +import androidx.core.content.ContextCompat import androidx.databinding.DataBindingUtil import androidx.fragment.app.Fragment import androidx.lifecycle.ViewModelProvider @@ -16,7 +18,7 @@ import org.oxycblt.auxio.R import org.oxycblt.auxio.databinding.FragmentLoadingBinding import org.oxycblt.auxio.music.processing.MusicLoaderResponse -class LoadingFragment : Fragment() { +class LoadingFragment : Fragment(R.layout.fragment_loading) { private val loadingModel: LoadingViewModel by lazy { ViewModelProvider( @@ -27,6 +29,8 @@ class LoadingFragment : Fragment() { ).get(LoadingViewModel::class.java) } + private var noPerms = false + private lateinit var binding: FragmentLoadingBinding private lateinit var permLauncher: ActivityResultLauncher @@ -69,72 +73,81 @@ class LoadingFragment : Fragment() { // If its actually granted, restart the loading process again. if (granted) { - binding.loadingBar.visibility = View.VISIBLE - binding.errorText.visibility = View.GONE - binding.statusIcon.visibility = View.GONE - binding.retryButton.visibility = View.GONE - binding.grantButton.visibility = View.GONE + wipeViews() + + noPerms = false loadingModel.retry() } } - // This never seems to return true but Im apparently supposed to use it so - if (shouldShowRequestPermissionRationale(Manifest.permission.READ_EXTERNAL_STORAGE)) { - onMusicLoadResponse(MusicLoaderResponse.NO_PERMS) + // Force an error screen if the permissions are denied or the prompt needs to be shown. + // This should be in MusicRepository, but the response comes faster than the view creation + // itself and therefore causes the error screen to not appear. + if (checkPerms()) { + noPerms = true + onNoPerms() } else { loadingModel.go() } - Log.d(this::class.simpleName, "Fragment created.") return binding.root } + // Check for permissions. God help us all. + private fun checkPerms(): Boolean { + return shouldShowRequestPermissionRationale( + Manifest.permission.READ_EXTERNAL_STORAGE + ) || ContextCompat.checkSelfPermission( + requireContext(), Manifest.permission.READ_EXTERNAL_STORAGE + ) == PackageManager.PERMISSION_DENIED || noPerms + } + private fun onMusicLoadResponse(repoResponse: MusicLoaderResponse?) { // Don't run this if the value is null, Which is what the value changes to after // this is run. repoResponse?.let { response -> + if (response == MusicLoaderResponse.DONE) { this.findNavController().navigate( LoadingFragmentDirections.actionToMain() ) } else { // If the response wasn't a success, then show the specific error message - // depending on which error response was given, along with a retry or grant button + // depending on which error response was given, along with a retry button binding.loadingBar.visibility = View.GONE binding.errorText.visibility = View.VISIBLE binding.statusIcon.visibility = View.VISIBLE + binding.retryButton.visibility = View.VISIBLE - when (response) { - MusicLoaderResponse.NO_PERMS -> { - binding.grantButton.visibility = View.VISIBLE - binding.errorText.text = getString(R.string.error_no_perms) - } - - MusicLoaderResponse.NO_MUSIC -> { - binding.retryButton.visibility = View.VISIBLE - binding.errorText.text = getString(R.string.error_no_music) - } - - else -> { - binding.retryButton.visibility = View.VISIBLE - binding.errorText.text = getString(R.string.error_music_load_failed) - } - } + binding.errorText.text = + if (response == MusicLoaderResponse.NO_MUSIC) + getString(R.string.error_no_music) + else + getString(R.string.error_music_load_failed) } loadingModel.doneWithResponse() } } + private fun onNoPerms() { + // If there are no perms, switch out the view elements as if an error screen was being + // shown, but show the label that Auxio needs to read external storage to function, + // along with a GRANT button + + binding.loadingBar.visibility = View.GONE + binding.errorText.visibility = View.VISIBLE + binding.statusIcon.visibility = View.VISIBLE + binding.grantButton.visibility = View.VISIBLE + + binding.errorText.text = getString(R.string.error_no_perms) + } + private fun onRetry(retry: Boolean) { if (retry) { - binding.loadingBar.visibility = View.VISIBLE - binding.errorText.visibility = View.GONE - binding.statusIcon.visibility = View.GONE - binding.retryButton.visibility = View.GONE - binding.grantButton.visibility = View.GONE + wipeViews() loadingModel.doneWithRetry() } @@ -147,4 +160,13 @@ class LoadingFragment : Fragment() { loadingModel.doneWithGrant() } } + + // Wipe views and switch back to the plain LoadingBar + private fun wipeViews() { + binding.loadingBar.visibility = View.VISIBLE + binding.errorText.visibility = View.GONE + binding.statusIcon.visibility = View.GONE + binding.retryButton.visibility = View.GONE + binding.grantButton.visibility = View.GONE + } } diff --git a/app/src/main/java/org/oxycblt/auxio/loading/LoadingViewModel.kt b/app/src/main/java/org/oxycblt/auxio/loading/LoadingViewModel.kt index 5e9375a32..3c2bc8413 100644 --- a/app/src/main/java/org/oxycblt/auxio/loading/LoadingViewModel.kt +++ b/app/src/main/java/org/oxycblt/auxio/loading/LoadingViewModel.kt @@ -34,6 +34,8 @@ class LoadingViewModel(private val app: Application) : ViewModel() { // Start the music loading. It has already been called, one needs to call retry() instead. fun go() { if (!started) { + started = true + startMusicRepo() } } @@ -49,8 +51,6 @@ class LoadingViewModel(private val app: Application) : ViewModel() { // Then actually notify listeners of the response in the Main thread withContext(Dispatchers.Main) { mMusicRepoResponse.value = response - - started = true } } } diff --git a/app/src/main/java/org/oxycblt/auxio/music/MusicRepository.kt b/app/src/main/java/org/oxycblt/auxio/music/MusicRepository.kt index ba5e5bf3b..75c4aa6e7 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/MusicRepository.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/MusicRepository.kt @@ -1,10 +1,7 @@ package org.oxycblt.auxio.music -import android.Manifest import android.app.Application -import android.content.pm.PackageManager import android.util.Log -import androidx.core.content.ContextCompat import org.oxycblt.auxio.R import org.oxycblt.auxio.music.models.Album import org.oxycblt.auxio.music.models.Artist @@ -23,12 +20,6 @@ class MusicRepository { lateinit var songs: List fun init(app: Application): MusicLoaderResponse { - if (!checkPerms(app)) { - Log.i(this::class.simpleName, "No permissions, aborting...") - - return MusicLoaderResponse.NO_PERMS - } - Log.i(this::class.simpleName, "Starting initial music load...") val start = System.currentTimeMillis() @@ -64,12 +55,6 @@ class MusicRepository { return loader.response } - private fun checkPerms(app: Application): Boolean { - return ContextCompat.checkSelfPermission( - app.applicationContext, Manifest.permission.READ_EXTERNAL_STORAGE - ) == PackageManager.PERMISSION_GRANTED - } - companion object { @Volatile private var INSTANCE: MusicRepository? = null diff --git a/app/src/main/java/org/oxycblt/auxio/music/processing/MusicLoader.kt b/app/src/main/java/org/oxycblt/auxio/music/processing/MusicLoader.kt index a3c81e0f1..c71fbcdfb 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/processing/MusicLoader.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/processing/MusicLoader.kt @@ -15,7 +15,7 @@ import org.oxycblt.auxio.music.toAlbumArtURI import org.oxycblt.auxio.music.toNamedGenre enum class MusicLoaderResponse { - DONE, FAILURE, NO_MUSIC, NO_PERMS + DONE, FAILURE, NO_MUSIC } // Class that loads music from the FileSystem. diff --git a/app/src/main/res/drawable/ripple.xml b/app/src/main/res/drawable/ripple.xml index 1747f0bc6..9ecc8eb07 100644 --- a/app/src/main/res/drawable/ripple.xml +++ b/app/src/main/res/drawable/ripple.xml @@ -1,10 +1,10 @@ + android:color="@color/selection_color"> - + \ No newline at end of file diff --git a/app/src/main/res/layout/fragment_main.xml b/app/src/main/res/layout/fragment_main.xml index 5f5699002..770b66337 100644 --- a/app/src/main/res/layout/fragment_main.xml +++ b/app/src/main/res/layout/fragment_main.xml @@ -27,7 +27,7 @@ app:tabIconTint="?android:attr/colorPrimary" app:tabIconTintMode="src_in" app:tabIndicator="@drawable/indicator" - app:tabRippleColor="?android:attr/colorPrimary" /> + app:tabRippleColor="@color/selection_color" /> \ No newline at end of file