added test environment for all combinations of shared element trans.

[KirkBushman]

here is a test environment, for all cases of shared element transition,
we can base our decision on this.

[Piasy]

Below is my test result on Pixel2:

[KirkBushman]

As I wrote on the other thread:

Ok, I've did some testing and there are no problems with the standars ImageView or with using Glide or Fresco, I see only issues when using GlideImageViewFactory and FrescoImageViewFactory.
And that is caused by the fact that you load the images additionally inside the overridden ImageViewFactory.

We should think about moving that call out of there, since we want to have control on the whole process, also it does not make sense to me, that loading the image should be responsability of a View Factory, that should only provide the Views.

I've made a nice test environment for all this, I'm sending you a PR.

[KirkBushman]

Do you agree on modifying ImageViewFactory? what would you do?

[Piasy]

Actually the ImageLoader and ImageViewFactory should be the same flavor, e.g. GlideImageLoader + GlideImageViewFactory, or FrescoImageLoader + FrescoImageViewFactory.

In the screen recording above, I'm using GlideImageLoader + GlideImageViewFactory, it has bug.

The ImageViewFactory is designed to handle animated image and thumbnail view earlier, if it cause problem here for shared element transition, I'm fine to move thumbnail handling logic out of it.

But here is my question again: can we use BIV in the FirstAnimActivity? what if the image is a gif?

[KirkBushman]

We are using the same flavor:

[KirkBushman]

    if (useGlide) {
        BigImageViewer.initialize(GlideImageLoader.with(applicationContext))
    }

    if (useFresco) {
        BigImageViewer.initialize(FrescoImageLoader.with(applicationContext))
    }

    if (useGlide && useViewFactory) {
        biv.setImageViewFactory(GlideImageViewFactory())
    }

    if (useFresco && useViewFactory) {
        biv.setImageViewFactory(FrescoImageViewFactory())
    }

[KirkBushman]

I think we could use a BIV for the first activity but I don't think it would make sense, usually you would have a MainActivity showing only thumbnails, maybe in a recyclerview, and once you click the image you go into a DetailActivity, the shows you the full resolution image.

[KirkBushman]

Where would you move the logic? into GlideImageLoader?

[Piasy]

Oh sorry I misunderstand your first conclusion, you said "I see only issues when using GlideImageViewFactory and FrescoImageViewFactory", I thought you have issue with GlideImageLoader + FrescoImageViewFactory.

But actually I also have issue when using FrescoImageLoader only, the enter transition works fine, but when exit the SecondAnimActivity, the FirstAnimActivity become white screen, see screen recording below.

As per moving the thumbnail logic out of ImageViewFactory, I'm open to any proposal.

[KirkBushman]

Hey, I've separated the tasks on ImageViewFactory, and now we are better managing what we are doing in the overloaded classes, from the sample I was able to solve most cases of transition cut (on my Pixel 3, at least).

The one that I cannot seem to solve, is the reenter animation when using fresco, do you have an idea of why it is doing it? I've always used Glide, so I don't have experience with fresco.

It's not something that is inside of BigImageView, or the transition in general, it happens on FirstAnimActivity, on setting the regular image.
I've seen online a variety of problems with this implementation with Fresco, I don't know if has problems with scale, with setting the image twice, or it's just a hacky pile of crap.

I'm going to do more test later on.
Tell me what you think of these changes, and what's on your mind in general.

Thanks in advance.

[Piasy]

Hi @KirkBushman , sorry for the late response, these changes look good to me in general.

Looking forward for the fix of fresco issue :)

[KirkBushman]

Ok @Piasy, got it working it was a problem with crappy Fresco library: facebook/fresco#1445

I've added a couple of callbacks and now everything is working to a fast check,
if you could push a new version I can set the code on my app.

[Piasy]

Nice work @KirkBushman , thanks!

[KirkBushman]

done!

[Piasy]

Hi @KirkBushman , I just released v1.6.0, but it has below issues on my Pixel Android 7.1:

• The shared image may flicker during enter transition, or become white after return transition,
  when using Fresco, see Fresco issue #1445;
• The shared image may flicker after return transition, especially after you zoomed SSIV;

[KirkBushman]

Emmm... I just opened the new version from upstream, why are there two activities for shared animation? Last time checked all was working fine, now it cuts the animation mid-way.

The only problem I see logically is that the animation can just modify the bounds, position of the view, it doesn't touch the zoom. We should trigger an animated de-zoom before the reenter animation.
That can be done to make the experience better for the final user...

But first, what are you trying to do with the sample app? :)

[Piasy]

I thought it may be caused by mixing Glide and Fresco together before, but it isn't.

But developer won't use Glide and Fresco at the same time to load image, so I just separate two cases into two different first animation activities.

[KirkBushman]

I doubt it is because of the two mixing.

Just to be sure I guess, I can separate the two, and try to add the de-zoom feature, I'll make a PR as soon as I go back home.

[KirkBushman]

Hey @Piasy, I looked through the code and I can say the reason the transition cuts it's because the starting image and the thumbnail don't have the same parameters (scaletype, adjustviewbounds) .

In my commits I harcoded the scale type in mThumbnailView, and forgot to remove that, you changed correctly to ImageView.createThumbnailView() passing scaletype and all...

But if you go in debugging, even if you set the parameter they don't get set!
I need more time to look at this, I'll send a PR when I'm done. unfortunately I'm very busy these days...

[Piasy]

Okay, no hurry.

[KirkBushman]

moved this discussion to #179