refactor: Restructure ViewMediaActivity and related fragments (#489)

Highlights:

- Implement fragment transitions for video to improve the UX, video
won't start playing until the transition completes
- Remove rxJava
- Move duplicate code in to base classes

Details:

`MediaActionsListener`:

- Move to `ViewMediaFragment` as it's used by both subclasses
- Remove need for separate `VideoActionsListener`
- Rename methods to better reflect their purpose and improve readability

`ViewMediaFragment`:

- Move duplicated code from `ViewImageFragment` and `ViewVideoFragment`
- Rewrite code that handles fragment transitions to use a
`CompleteableDeferred` instead of `BehaviorSubject` (removes rxJava).
- Rename methods and properties to better reflect their purpose and
improve readability
- Add extra comments

`ViewImageFragment`:
- Rewrite code that handles fragment transitions to use a
`CompleteableDeferred` instead of `BehaviorSubject` (removes rxJava).

`ViewVideoFragment`:

- Implement fragment transitions for video to improve the UX, video
won't start playing until the transition completes
- Manage toolbar visibility with a coroutine instead of a handler
- Add extra comments

`ViewMediaActivity`:
- Rename properties to better reflect their purpose and improve
readability
- Add extra comments

`ImagePagerAdapter`:
- Rename properties to better reflect their purpose and improve
readability
- Add extra comments

app/src/main/java/app/pachli/ViewMediaActivity.kt

@@ -51,8 +51,7 @@ import app.pachli.core.navigation.AttachmentViewData
 import app.pachli.core.navigation.ViewMediaActivityIntent
 import app.pachli.core.navigation.ViewThreadActivityIntent
 import app.pachli.databinding.ActivityViewMediaBinding
-import app.pachli.fragment.ViewImageFragment
-import app.pachli.fragment.ViewVideoFragment
+import app.pachli.fragment.MediaActionsListener
 import app.pachli.pager.ImagePagerAdapter
 import app.pachli.pager.SingleImagePagerAdapter
 import app.pachli.util.getTemporaryMediaFilename
@@ -76,7 +75,7 @@ typealias ToolbarVisibilityListener = (isVisible: Boolean) -> Unit
  * Show one or more media items (pictures, video, audio, etc).
  */
 @AndroidEntryPoint
-class ViewMediaActivity : BaseActivity(), ViewImageFragment.PhotoActionsListener, ViewVideoFragment.VideoActionsListener {
+class ViewMediaActivity : BaseActivity(), MediaActionsListener {
     @Inject
     lateinit var okHttpClient: OkHttpClient
 
@@ -88,15 +87,21 @@ class ViewMediaActivity : BaseActivity(), ViewImageFragment.PhotoActionsListener
     var isToolbarVisible = true
         private set
 
-    private var attachments: List<AttachmentViewData>? = null
+    private var attachmentViewData: List<AttachmentViewData>? = null
     private val toolbarVisibilityListeners = mutableListOf<ToolbarVisibilityListener>()
     private var imageUrl: String? = null
 
     /** True if a download to share media is in progress */
     private var isDownloading: Boolean = false
 
+    /**
+     * Adds [listener] to the list of toolbar listeners and immediately calls
+     * it with the current toolbar visibility.
+     *
+     * @return A function that must be called to remove the listener.
+     */
     fun addToolbarVisibilityListener(listener: ToolbarVisibilityListener): Function0<Boolean> {
-        this.toolbarVisibilityListeners.add(listener)
+        toolbarVisibilityListeners.add(listener)
         listener(isToolbarVisible)
         return { toolbarVisibilityListeners.remove(listener) }
     }
@@ -108,16 +113,16 @@ class ViewMediaActivity : BaseActivity(), ViewImageFragment.PhotoActionsListener
         supportPostponeEnterTransition()
 
         // Gather the parameters.
-        attachments = ViewMediaActivityIntent.getAttachments(intent)
+        attachmentViewData = ViewMediaActivityIntent.getAttachments(intent)
         val initialPosition = ViewMediaActivityIntent.getAttachmentIndex(intent)
 
         // Adapter is actually of existential type PageAdapter & SharedElementsTransitionListener
         // but it cannot be expressed and if I don't specify type explicitly compilation fails
         // (probably a bug in compiler)
-        val adapter: ViewMediaAdapter = if (attachments != null) {
-            val realAttachs = attachments!!.map(AttachmentViewData::attachment)
+        val adapter: ViewMediaAdapter = if (attachmentViewData != null) {
+            val attachments = attachmentViewData!!.map(AttachmentViewData::attachment)
             // Setup the view pager.
-            ImagePagerAdapter(this, realAttachs, initialPosition)
+            ImagePagerAdapter(this, attachments, initialPosition)
         } else {
             imageUrl = ViewMediaActivityIntent.getImageUrl(intent)
                 ?: throw IllegalArgumentException("attachment list or image url has to be set")
@@ -137,12 +142,12 @@ class ViewMediaActivity : BaseActivity(), ViewImageFragment.PhotoActionsListener
 
         // Setup the toolbar.
         setSupportActionBar(binding.toolbar)
-        val actionBar = supportActionBar
-        if (actionBar != null) {
-            actionBar.setDisplayHomeAsUpEnabled(true)
-            actionBar.setDisplayShowHomeEnabled(true)
-            actionBar.title = getPageTitle(initialPosition)
+        supportActionBar?.apply {
+            setDisplayHomeAsUpEnabled(true)
+            setDisplayShowHomeEnabled(true)
+            title = getPageTitle(initialPosition)
         }
+
         binding.toolbar.setNavigationOnClickListener { supportFinishAfterTransition() }
         binding.toolbar.setOnMenuItemClickListener { item: MenuItem ->
             when (item.itemId) {
@@ -170,7 +175,7 @@ class ViewMediaActivity : BaseActivity(), ViewImageFragment.PhotoActionsListener
     override fun onCreateOptionsMenu(menu: Menu): Boolean {
         menuInflater.inflate(R.menu.view_media_toolbar, menu)
         // We don't support 'open status' from single image views
-        menu.findItem(R.id.action_open_status)?.isVisible = (attachments != null)
+        menu.findItem(R.id.action_open_status)?.isVisible = (attachmentViewData != null)
         return true
     }
 
@@ -179,15 +184,15 @@ class ViewMediaActivity : BaseActivity(), ViewImageFragment.PhotoActionsListener
         return true
     }
 
-    override fun onBringUp() {
+    override fun onMediaReady() {
         supportStartPostponedEnterTransition()
     }
 
-    override fun onDismiss() {
+    override fun onMediaDismiss() {
         supportFinishAfterTransition()
     }
 
-    override fun onPhotoTap() {
+    override fun onMediaTap() {
         isToolbarVisible = !isToolbarVisible
         for (listener in toolbarVisibilityListeners) {
             listener(isToolbarVisible)
@@ -214,14 +219,12 @@ class ViewMediaActivity : BaseActivity(), ViewImageFragment.PhotoActionsListener
     }
 
     private fun getPageTitle(position: Int): CharSequence {
-        if (attachments == null) {
-            return ""
-        }
-        return String.format(Locale.getDefault(), "%d/%d", position + 1, attachments?.size)
+        attachmentViewData ?: return ""
+        return String.format(Locale.getDefault(), "%d/%d", position + 1, attachmentViewData?.size)
     }
 
     private fun downloadMedia() {
-        val url = imageUrl ?: attachments!![binding.viewPager.currentItem].attachment.url
+        val url = imageUrl ?: attachmentViewData!![binding.viewPager.currentItem].attachment.url
         val filename = Uri.parse(url).lastPathSegment
         Toast.makeText(applicationContext, resources.getString(R.string.download_image, filename), Toast.LENGTH_SHORT).show()
 
@@ -248,12 +251,12 @@ class ViewMediaActivity : BaseActivity(), ViewImageFragment.PhotoActionsListener
     }
 
     private fun onOpenStatus() {
-        val attach = attachments!![binding.viewPager.currentItem]
+        val attach = attachmentViewData!![binding.viewPager.currentItem]
         startActivityWithSlideInAnimation(ViewThreadActivityIntent(this, attach.statusId, attach.statusUrl))
     }
 
     private fun copyLink() {
-        val url = imageUrl ?: attachments!![binding.viewPager.currentItem].attachment.url
+        val url = imageUrl ?: attachmentViewData!![binding.viewPager.currentItem].attachment.url
         val clipboard = getSystemService(Context.CLIPBOARD_SERVICE) as ClipboardManager
         clipboard.setPrimaryClip(ClipData.newPlainText(null, url))
     }
@@ -268,7 +271,7 @@ class ViewMediaActivity : BaseActivity(), ViewImageFragment.PhotoActionsListener
         if (imageUrl != null) {
             shareMediaFile(directory, imageUrl!!)
         } else {
-            val attachment = attachments!![binding.viewPager.currentItem].attachment
+            val attachment = attachmentViewData!![binding.viewPager.currentItem].attachment
             shareMediaFile(directory, attachment.url)
         }
     }

app/src/main/java/app/pachli/fragment/ViewImageFragment.kt

@@ -19,7 +19,6 @@ package app.pachli.fragment
 import android.animation.Animator
 import android.animation.AnimatorListenerAdapter
 import android.annotation.SuppressLint
-import android.content.Context
 import android.graphics.PointF
 import android.graphics.drawable.Drawable
 import android.os.Bundle
@@ -30,6 +29,7 @@ import android.view.View
 import android.view.ViewGroup
 import android.widget.ImageView
 import androidx.core.view.GestureDetectorCompat
+import androidx.lifecycle.lifecycleScope
 import app.pachli.R
 import app.pachli.ViewMediaActivity
 import app.pachli.core.common.extensions.hide
@@ -43,35 +43,21 @@ import com.bumptech.glide.request.RequestListener
 import com.bumptech.glide.request.target.Target
 import com.ortiz.touchview.OnTouchCoordinatesListener
 import com.ortiz.touchview.TouchImageView
-import io.reactivex.rxjava3.subjects.BehaviorSubject
 import kotlin.math.abs
+import kotlinx.coroutines.launch
 
 class ViewImageFragment : ViewMediaFragment() {
-    interface PhotoActionsListener {
-        fun onBringUp()
-        fun onDismiss()
-        fun onPhotoTap()
-    }
 
     private val binding by viewBinding(FragmentViewImageBinding::bind)
 
-    private lateinit var photoActionsListener: PhotoActionsListener
     private lateinit var toolbar: View
-    private var transition = BehaviorSubject.create<Unit>()
 
     // 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.
     @Volatile
     private var startedTransition = false
 
-    override fun onAttach(context: Context) {
-        super.onAttach(context)
-        photoActionsListener = context as PhotoActionsListener
-    }
-
-    override fun setupMediaView(
-        showingDescription: Boolean,
-    ) {
+    override fun setupMediaView(showingDescription: Boolean) {
         binding.photoView.transitionName = attachment.url
         binding.mediaDescription.text = attachment.description
         binding.captionSheet.visible(showingDescription)
@@ -82,7 +68,6 @@ class ViewImageFragment : ViewMediaFragment() {
 
     override fun onCreateView(inflater: LayoutInflater, container: ViewGroup?, savedInstanceState: Bundle?): View {
         toolbar = (requireActivity() as ViewMediaActivity).toolbar
-        this.transition = BehaviorSubject.create()
         return inflater.inflate(R.layout.fragment_view_image, container, false)
     }
 
@@ -95,7 +80,7 @@ class ViewImageFragment : ViewMediaFragment() {
             object : GestureDetector.SimpleOnGestureListener() {
                 override fun onDown(e: MotionEvent) = true
                 override fun onSingleTapConfirmed(e: MotionEvent): Boolean {
-                    photoActionsListener.onPhotoTap()
+                    mediaActionsListener.onMediaTap()
                     return false
                 }
             },
@@ -191,7 +176,7 @@ class ViewImageFragment : ViewMediaFragment() {
                  */
                 private fun onGestureEnd(view: View) {
                     if (abs(view.translationY) > 180) {
-                        photoActionsListener.onDismiss()
+                        mediaActionsListener.onMediaDismiss()
                     } else {
                         view.animate().translationY(0f).scaleX(1f).scaleY(1f).start()
                     }
@@ -200,11 +185,6 @@ class ViewImageFragment : ViewMediaFragment() {
         )
     }
 
-    override fun onResume() {
-        super.onResume()
-        finalizeViewSetup()
-    }
-
     override fun onToolbarVisibilityChange(visible: Boolean) {
         if (!userVisibleHint) return
 
@@ -228,30 +208,24 @@ class ViewImageFragment : ViewMediaFragment() {
         Glide.with(this).clear(binding.photoView)
     }
 
-    override fun onDestroyView() {
-        transition.onComplete()
-        super.onDestroyView()
-    }
-
+    @SuppressLint("CheckResult")
     private fun loadImageFromNetwork(url: String, previewUrl: String?, photoView: ImageView) {
         val glide = Glide.with(this)
         // Request image from the any cache
         glide
             .load(url)
             .dontAnimate()
             .onlyRetrieveFromCache(true)
-            .let {
-                if (previewUrl != null) {
-                    it.thumbnail(
+            .apply {
+                previewUrl?.let {
+                    thumbnail(
                         glide
-                            .load(previewUrl)
+                            .load(it)
                             .dontAnimate()
                             .onlyRetrieveFromCache(true)
                             .centerInside()
                             .addListener(ImageRequestListener(true, isThumbnailRequest = true)),
                     )
-                } else {
-                    it
                 }
             }
             // Request image from the network on fail load image from cache
@@ -297,10 +271,10 @@ class ViewImageFragment : ViewMediaFragment() {
             isFirstResource: Boolean,
         ): Boolean {
             // If cache for full image failed complete transition
-            if (isCacheRequest && !isThumbnailRequest && shouldStartTransition &&
+            if (isCacheRequest && !isThumbnailRequest && shouldCallMediaReady &&
                 !startedTransition
             ) {
-                photoActionsListener.onBringUp()
+                mediaActionsListener.onMediaReady()
             }
             // Hide progress bar only on fail request from internet
             if (!isCacheRequest) binding.progressBar.hide()
@@ -318,29 +292,26 @@ class ViewImageFragment : ViewMediaFragment() {
         ): Boolean {
             binding.progressBar.hide() // Always hide the progress bar on success
 
-            if (!startedTransition || !shouldStartTransition) {
+            if (!startedTransition || !shouldCallMediaReady) {
                 // Set this right away so that we don't have to concurrent post() requests
                 startedTransition = true
                 // post() because load() replaces image with null. Sometimes after we set
                 // the thumbnail.
                 binding.photoView.post {
                     target.onResourceReady(resource, null)
-                    if (shouldStartTransition) photoActionsListener.onBringUp()
+                    if (shouldCallMediaReady) mediaActionsListener.onMediaReady()
                 }
             } else {
-                // This wait for transition. If there's no transition then we should hit
-                // another branch. take() will unsubscribe after we have it to not leak memory
-                transition
-                    .take(1)
-                    .subscribe {
+                // Wait for the fragment transition to complete before signalling to
+                // Glide that the resource is ready.
+                transitionComplete?.let {
+                    viewLifecycleOwner.lifecycleScope.launch {
+                        it.await()
                         target.onResourceReady(resource, null)
                     }
+                }
             }
             return true
         }
     }
-
-    override fun onTransitionEnd() {
-        this.transition.onNext(Unit)
-    }
 }