From a3fa0647b60ce80efe6b2f50a0e834478d86dde1 Mon Sep 17 00:00:00 2001 From: Ivan Kupalov Date: Sat, 17 Aug 2019 20:05:24 +0200 Subject: [PATCH] Fix image viewer & transitions (#1448) --- .../tusky/fragment/ViewImageFragment.kt | 77 ++++++++++++++----- .../tusky/pager/ImagePagerAdapter.kt | 11 ++- 2 files changed, 69 insertions(+), 19 deletions(-) diff --git a/app/src/main/java/com/keylesspalace/tusky/fragment/ViewImageFragment.kt b/app/src/main/java/com/keylesspalace/tusky/fragment/ViewImageFragment.kt index 1f6596c31..1c3672519 100644 --- a/app/src/main/java/com/keylesspalace/tusky/fragment/ViewImageFragment.kt +++ b/app/src/main/java/com/keylesspalace/tusky/fragment/ViewImageFragment.kt @@ -17,6 +17,7 @@ package com.keylesspalace.tusky.fragment import android.animation.Animator import android.animation.AnimatorListenerAdapter +import android.annotation.SuppressLint import android.content.Context import android.graphics.drawable.Drawable import android.os.Bundle @@ -51,6 +52,11 @@ class ViewImageFragment : ViewMediaFragment() { private lateinit var photoActionsListener: PhotoActionsListener private lateinit var toolbar: View private var transition = BehaviorSubject.create() + 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. + @Volatile + private var startedTransition = false override lateinit var descriptionView: TextView override fun onAttach(context: Context) { @@ -78,11 +84,13 @@ class ViewImageFragment : ViewMediaFragment() { } } + startedTransition = false loadImageFromNetwork(url, previewUrl, photoView) } override fun onCreateView(inflater: LayoutInflater, container: ViewGroup?, savedInstanceState: Bundle?): View { toolbar = activity!!.toolbar + this.transition = BehaviorSubject.create() return inflater.inflate(R.layout.fragment_view_image, container, false) } @@ -91,6 +99,7 @@ class ViewImageFragment : ViewMediaFragment() { val arguments = this.arguments!! val attachment = arguments.getParcelable(ARG_ATTACHMENT) + this.shouldStartTransition = arguments.getBoolean(ARG_START_POSTPONED_TRANSITION) val url: String? var description: String? = null @@ -129,6 +138,7 @@ class ViewImageFragment : ViewMediaFragment() { override fun onDestroyView() { Glide.with(this).clear(photoView) + transition.onComplete() super.onDestroyView() } @@ -160,6 +170,23 @@ class ViewImageFragment : ViewMediaFragment() { } /** + * We start transition as soon as we think reasonable but we must take care about couple of + * things> + * - Do not change image in the middle of transition. It messes up the view. + * - Do not transition for the views which don't require it. Starting transition from + * multiple fragments does weird things + * - Do not wait to transition until the image loads from network + * + * Preview, cached image, network image, x - failed, o - succeeded + * P C N - start transition after... + * x x x - the cache fails + * x x o - the cache fails + * x o o - the cache succeeds + * o x o - the preview succeeds. Do not start on cache. + * o o o - the preview succeeds. Do not start on cache. + * + * So start transition after the first success or after anything with the cache + * * @param isCacheRequest - is this listener for request image from cache or from the network */ private inner class ImageRequestListener( @@ -168,32 +195,46 @@ class ViewImageFragment : ViewMediaFragment() { override fun onLoadFailed(e: GlideException?, model: Any, target: Target, isFirstResource: Boolean): Boolean { - // If cache for full image failed, complete transition - if (isCacheRequest && !isThumnailRequest) photoActionsListener.onBringUp() + // If cache for full image failed complete transition + if (isCacheRequest && !isThumnailRequest && shouldStartTransition + && !startedTransition) { + photoActionsListener.onBringUp() + } // Hide progress bar only on fail request from internet if (!isCacheRequest) progressBar?.hide() - return false + // We don't want to overwrite preview with null when main image fails to load + return !isCacheRequest } + @SuppressLint("CheckResult") override fun onResourceReady(resource: Drawable, model: Any, target: Target, dataSource: DataSource, isFirstResource: Boolean): Boolean { progressBar?.hide() // Always hide the progress bar on success - if (isThumnailRequest) { - photoView.post { - target.onResourceReady(resource, null) - photoActionsListener.onBringUp() - } - } else { - transition - .take(1) - .subscribe { - target.onResourceReady(resource, null) - photoActionsListener.onBringUp() - } - } - return true - } + if (!startedTransition || !shouldStartTransition) { + // 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. + photoView.post { + target.onResourceReady(resource, null) + if (shouldStartTransition) photoActionsListener.onBringUp() + } + } 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 menmory + transition + .take(1) + .subscribe { + target.onResourceReady(resource, null) + // It's needed. Don't ask why, I don't know, setImageDrawable() should + // do it by itself but somehow it doesn't work automatically. + // Just do it. If you don't, image will jump around when touched. + attacher.update() + } + } + return true + } } override fun onTransitionEnd() { diff --git a/app/src/main/java/com/keylesspalace/tusky/pager/ImagePagerAdapter.kt b/app/src/main/java/com/keylesspalace/tusky/pager/ImagePagerAdapter.kt index 55e5b25c7..983770d2a 100644 --- a/app/src/main/java/com/keylesspalace/tusky/pager/ImagePagerAdapter.kt +++ b/app/src/main/java/com/keylesspalace/tusky/pager/ImagePagerAdapter.kt @@ -16,6 +16,7 @@ class ImagePagerAdapter( ) : FragmentStatePagerAdapter(fragmentManager), SharedElementTransitionListener { private var primaryItem: ViewMediaFragment? = null + private var didTransition = false override fun setPrimaryItem(container: ViewGroup, position: Int, item: Any) { super.setPrimaryItem(container, position, item) @@ -24,7 +25,14 @@ class ImagePagerAdapter( override fun getItem(position: Int): Fragment { return if (position >= 0 && position < attachments.size) { - ViewMediaFragment.newInstance(attachments[position], position == initialPosition) + // Fragment should not wait for or start transition if it already happened but we + // instantiate the same fragment again, e.g. open the first photo, scroll to the + // forth photo and then back to the first. The first fragment will trz to start the + // transition and wait until it's over and it will never take place. + ViewMediaFragment.newInstance( + attachment = attachments[position], + shouldStartPostponedTransition = !didTransition && position == initialPosition + ) } else { throw IllegalStateException() } @@ -39,6 +47,7 @@ class ImagePagerAdapter( } override fun onTransitionEnd() { + this.didTransition = true primaryItem?.onTransitionEnd() } }