Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

shared animation works and test, fixed warnings and test app #175

Merged
merged 2 commits into from
Oct 13, 2019

Conversation

KirkBushman
Copy link
Contributor

Hi, this is the work I did to make BigImageViewer better support shared transition animations,
this is all working and tested, but this is only a starting point.

The problem, as described in #98, is that while the animation is running between two activities, the View switches the thumbnailView and the SSIV, causing stutters, or black frames, or an animation that does not run at all.

My suggestion is to have an additional parameter to only show the thumbnail for the transition and delay the main image loading, so we can trigger it later using listeners.

I've made those modifications, and a test environment, using two activities and it does work, while not breaking the others activities in the sample app.

I've added another listener for testing purposes, tell me if you want to keep it or not.

While working with the sample app, I reduced the number of warnings, moving harcoded strings in resources and other stuff, hope you don't mind. Bumped okhttp to the next minor too.

What do you think? would you do it in a different way?

Good night for now.

@Piasy
Copy link
Owner

Piasy commented Oct 13, 2019

Great work! Thanks for your contribution!

I'll check it today.

@KirkBushman
Copy link
Contributor Author

I'm writing you here, probably should open an issue, would you consider moving this lib to java 8?
Now that desugaring is enabled by default, it makes it compatible to all previous bytecodes without using something like Retrolambda, so why miss all Java 8 features?

What do you think?

@Piasy
Copy link
Owner

Piasy commented Oct 13, 2019

I'm writing you here, probably should open an issue, would you consider moving this lib to java 8?
Now that desugaring is enabled by default, it makes it compatible to all previous bytecodes without using something like Retrolambda, so why miss all Java 8 features?

What do you think?

Do you mean we should change the library setting to compile it with Java 8? I don't understand what's the problem of compiling with Java 7?

@KirkBushman
Copy link
Contributor Author

There is no problem with compiling with Java 7, but Java 8 is free, there is no restriction, so why not?

@KirkBushman
Copy link
Contributor Author

Many libs are moving to Java 8 right now

@Piasy
Copy link
Owner

Piasy commented Oct 13, 2019

Okay I got it, but maybe some developers still stay at Java 7, if we bump to Java 8, they will be affected. Now that there is no problem with Java 7, I prefer stay at Java 7.

@KirkBushman
Copy link
Contributor Author

ok

@Piasy Piasy merged commit 5404ff9 into Piasy:master Oct 13, 2019
@Piasy
Copy link
Owner

Piasy commented Oct 13, 2019

Hi @KirkBushman , I did more test after merge this PR, I found some issues, and create a branch (shared_element_transition_bug) to reproduce them, could you please check out and fix them? Otherwise I can't publish this feature.

@KirkBushman
Copy link
Contributor Author

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.

Piasy pushed a commit that referenced this pull request Oct 16, 2019
…176)

* shared animation works and test, fixed warnings and test app (#175)

* shared animation works and test, fixed warnings and test app

* fixed naming schemes, passed view through ImageViewFactory

* added test environment for all combinations of shared element transition works.

* separated view retrival and animated content loading

* working for most, todo check fresco reenter transition

* fixed problem with Fresco reenter transition

* nullability checks, deleted comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants