refactor: Simplify View{Image,Media,Video}Fragment creation (#175)

Previous code had to distinguish between showing an attachment or showing an image by URL. Simplify this by -- in the image URL case -- creating a fake attachment that references the image URL. Move the code that unmarshalls the Bundle arguments to `ViewMediaFragment` to share between `ViewImageFragment` and `ViewVideoFragment`.
2023-10-15 22:29:18 +02:00 · 2023-10-15 22:29:18 +02:00 · 24fa26c126
commit 24fa26c126
parent c50f10a989
4 changed files with 37 additions and 44 deletions
--- a/app/src/main/java/app/pachli/fragment/ViewImageFragment.kt
+++ b/app/src/main/java/app/pachli/fragment/ViewImageFragment.kt
@ -28,12 +28,10 @@ import android.view.MotionEvent
 import android.view.View
 import android.view.ViewGroup
 import android.widget.ImageView
-import androidx.core.os.BundleCompat
 import androidx.core.view.GestureDetectorCompat
 import app.pachli.R
 import app.pachli.ViewMediaActivity
 import app.pachli.databinding.FragmentViewImageBinding
-import app.pachli.entity.Attachment
 import app.pachli.util.hide
 import app.pachli.util.viewBinding
 import app.pachli.util.visible
@ -59,7 +57,6 @@ class ViewImageFragment : ViewMediaFragment() {
    private lateinit var photoActionsListener: PhotoActionsListener
    private lateinit var toolbar: View
    private var transition = BehaviorSubject.create<Unit>()
-    private var shouldStartTransition = false

    // Volatile: Image requests happen on background thread and we want to see updates to it
    // immediately on another thread. Atomic is an overkill for such thing.
@ -72,15 +69,14 @@ class ViewImageFragment : ViewMediaFragment() {
    }

    override fun setupMediaView(
-        previewUrl: String?,
        showingDescription: Boolean,
    ) {
-        binding.photoView.transitionName = url
-        binding.mediaDescription.text = description
+        binding.photoView.transitionName = attachment.url
+        binding.mediaDescription.text = attachment.description
        binding.captionSheet.visible(showingDescription)

        startedTransition = false
-        loadImageFromNetwork(url, previewUrl, binding.photoView)
+        loadImageFromNetwork(attachment.url, attachment.previewUrl, binding.photoView)
    }

    override fun onCreateView(inflater: LayoutInflater, container: ViewGroup?, savedInstanceState: Bundle?): View {
@ -93,17 +89,6 @@ class ViewImageFragment : ViewMediaFragment() {
    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
        super.onViewCreated(view, savedInstanceState)

-        val arguments = this.requireArguments()
-        attachment = BundleCompat.getParcelable(arguments, ARG_ATTACHMENT, Attachment::class.java)
-        this.shouldStartTransition = arguments.getBoolean(ARG_START_POSTPONED_TRANSITION)
-
-        BundleCompat.getParcelable(arguments, ARG_ATTACHMENT, Attachment::class.java)?.let {
-            url = it.url
-            description = it.description
-        } ?: arguments.getString(ARG_SINGLE_IMAGE_URL)?.let {
-            url = it
-        } ?: throw IllegalArgumentException("attachment or image url has to be set")
-
        val singleTapDetector = GestureDetectorCompat(
            requireContext(),
            object : GestureDetector.SimpleOnGestureListener() {
@ -216,7 +201,7 @@ class ViewImageFragment : ViewMediaFragment() {

    override fun onResume() {
        super.onResume()
-        finalizeViewSetup(attachment?.previewUrl)
+        finalizeViewSetup()
    }

    override fun onToolbarVisibilityChange(visible: Boolean) {
--- a/app/src/main/java/app/pachli/fragment/ViewMediaFragment.kt
+++ b/app/src/main/java/app/pachli/fragment/ViewMediaFragment.kt
@ -17,6 +17,7 @@ package app.pachli.fragment

 import android.os.Bundle
 import android.text.TextUtils
+import android.view.View
 import androidx.annotation.OptIn
 import androidx.fragment.app.Fragment
 import androidx.media3.common.util.UnstableApi
@ -27,7 +28,6 @@ abstract class ViewMediaFragment : Fragment() {
    private var toolbarVisibilityDisposable: Function0<Boolean>? = null

    abstract fun setupMediaView(
-        previewUrl: String?,
        showingDescription: Boolean,
    )

@ -36,14 +36,20 @@ abstract class ViewMediaFragment : Fragment() {
    protected var showingDescription = false
    protected var isDescriptionVisible = false

-    /** URL of the media to show */
-    protected lateinit var url: String
+    /** The attachment to show. Set in [onViewCreated] */
+    protected lateinit var attachment: Attachment

-    /** The attachment to show. Null if [newSingleImageInstance] was used */
-    protected var attachment: Attachment? = null
+    protected var shouldStartTransition = false

-    /** Media description */
-    protected var description: String? = null
+    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
+        super.onViewCreated(view, savedInstanceState)
+
+        attachment = arguments?.getParcelable<Attachment>(ARG_ATTACHMENT)
+            ?: throw IllegalArgumentException("ARG_ATTACHMENT has to be set")
+
+        shouldStartTransition = arguments?.getBoolean(ARG_START_POSTPONED_TRANSITION)
+            ?: throw IllegalArgumentException("ARG_START_POSTPONED_TRANSITION has to be set")
+    }

    companion object {
        @JvmStatic
@ -52,9 +58,6 @@ abstract class ViewMediaFragment : Fragment() {
        @JvmStatic
        protected val ARG_ATTACHMENT = "attach"

-        @JvmStatic
-        protected val ARG_SINGLE_IMAGE_URL = "singleImageUrl"
-
        @JvmStatic
        @OptIn(UnstableApi::class)
        fun newInstance(attachment: Attachment, shouldStartPostponedTransition: Boolean): ViewMediaFragment {
@ -75,10 +78,21 @@ abstract class ViewMediaFragment : Fragment() {
        }

        @JvmStatic
-        fun newSingleImageInstance(imageUrl: String): ViewMediaFragment {
+        fun newInstance(imageUrl: String): ViewMediaFragment {
            val arguments = Bundle(2)
            val fragment = ViewImageFragment()
-            arguments.putString(ARG_SINGLE_IMAGE_URL, imageUrl)
+            arguments.putParcelable(
+                ARG_ATTACHMENT,
+                Attachment(
+                    id = "unused",
+                    url = imageUrl,
+                    type = Attachment.Type.IMAGE,
+                    previewUrl = null,
+                    meta = null,
+                    description = null,
+                    blurhash = null,
+                ),
+            )
            arguments.putBoolean(ARG_START_POSTPONED_TRANSITION, true)

            fragment.arguments = arguments
@ -88,12 +102,12 @@ abstract class ViewMediaFragment : Fragment() {

    abstract fun onTransitionEnd()

-    protected fun finalizeViewSetup(previewUrl: String?) {
+    protected fun finalizeViewSetup() {
        val mediaActivity = activity as ViewMediaActivity

-        showingDescription = !TextUtils.isEmpty(description)
+        showingDescription = !TextUtils.isEmpty(attachment.description)
        isDescriptionVisible = showingDescription
-        setupMediaView(previewUrl, showingDescription && mediaActivity.isToolbarVisible)
+        setupMediaView(showingDescription && mediaActivity.isToolbarVisible)

        toolbarVisibilityDisposable = (activity as ViewMediaActivity)
            .addToolbarVisibilityListener { isVisible ->
--- a/app/src/main/java/app/pachli/fragment/ViewVideoFragment.kt
+++ b/app/src/main/java/app/pachli/fragment/ViewVideoFragment.kt
@ -125,11 +125,6 @@ class ViewVideoFragment : ViewMediaFragment() {
    @SuppressLint("ClickableViewAccessibility")
    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
        super.onViewCreated(view, savedInstanceState)
-        val attachment = arguments?.getParcelable<Attachment>(ARG_ATTACHMENT)
-            ?: throw IllegalArgumentException("attachment has to be set")
-
-        url = attachment.url
-        description = attachment.description

        isAudio = attachment.type == Attachment.Type.AUDIO

@ -259,7 +254,7 @@ class ViewVideoFragment : ViewMediaFragment() {
    override fun onResume() {
        super.onResume()

-        finalizeViewSetup(attachment?.previewUrl)
+        finalizeViewSetup()

        if (Build.VERSION.SDK_INT <= 23 || player == null) {
            initializePlayer()
@ -351,17 +346,16 @@ class ViewVideoFragment : ViewMediaFragment() {

    @SuppressLint("ClickableViewAccessibility")
    override fun setupMediaView(
-        previewUrl: String?,
        showingDescription: Boolean,
    ) {
-        binding.mediaDescription.text = description
+        binding.mediaDescription.text = attachment.description
        binding.mediaDescription.visible(showingDescription)
        binding.mediaDescription.movementMethod = ScrollingMovementMethod()

        // Ensure the description is visible over the video
        binding.mediaDescription.elevation = binding.videoView.elevation + 1

-        binding.videoView.transitionName = url
+        binding.videoView.transitionName = attachment.url

        binding.videoView.requestFocus()

--- a/app/src/main/java/app/pachli/pager/SingleImagePagerAdapter.kt
+++ b/app/src/main/java/app/pachli/pager/SingleImagePagerAdapter.kt
@ -12,7 +12,7 @@ class SingleImagePagerAdapter(

    override fun createFragment(position: Int): Fragment {
        return if (position == 0) {
-            ViewMediaFragment.newSingleImageInstance(imageUrl)
+            ViewMediaFragment.newInstance(imageUrl)
        } else {
            throw IllegalStateException()
        }