Skip to content

Support for Glide 4.0 #802

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

Merged
merged 9 commits into from
Sep 20, 2017
Merged

Support for Glide 4.0 #802

merged 9 commits into from
Sep 20, 2017

Conversation

samtstern
Copy link
Contributor

See #731

Change-Id: I5126936ae60f1e3251b24211403f35a32faced08
@samtstern samtstern mentioned this pull request Jul 14, 2017
Change-Id: Ibc59436db533fd71055c5eb65ca74ef4bf1e6117
return true;
}

private class FirebaseStorageKey implements Key {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be static?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call.

Change-Id: I50a3fbde9127d435c38d5f3de9608d428b82d08d
@samtstern samtstern added this to the 3.0 milestone Jul 19, 2017
@akaSourav
Copy link

akaSourav commented Sep 17, 2017

Change the Override method in GlideModule from

@Override
public void registerComponents(Context context, Registry registry) {
        registry.append(StorageReference.class, InputStream.class, new FirebaseImageLoader.Factory());
    }

to

@Override
public void registerComponents(Context context, Glide glide, Registry registry) {
        registry.append(StorageReference.class, InputStream.class, new FirebaseImageLoader.Factory());
    }

@samtstern
Copy link
Contributor Author

@SUPERCILEX @TheCraftKid any objections to pushing this through for 3.0?

@samtstern
Copy link
Contributor Author

samtstern commented Sep 19, 2017

@Sourav-21 can you explain your comment? That does not seem to be a valid override on AppGlideModule.

Edit: nevermind, worked out when I went to 4.1.1

Change-Id: I60bbd392b21be9adf4bf31790d071151b5397330
@samtstern samtstern changed the base branch from version-2.1.0-dev to version-3.0.0-dev September 19, 2017 23:53
@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

Change-Id: I97a2011c49eb8b1dc45beb8cbd563fe43a6be6b3
@googlebot
Copy link

CLAs look good, thanks!

Change-Id: I2d190c5ced6cb06ea9b9b93ecc5e0886855efecc
@WillieCubed
Copy link
Contributor

LGTM!

@akaSourav
Copy link

Using GlideApp instead of Glide will be better.

@WillieCubed
Copy link
Contributor

@Sourav-21 Is there a specific reason? The samples don't make that distinction.

@akaSourav
Copy link

With the generated API, the RequestOptions calls can be inlined:

GlideApp.with(fragment)
    .load(url)
    .centerCrop()
    .placeholder(R.drawable.placeholder)
    .error(R.drawable.error)
    .priority(Priority.HIGH)
    .into(imageView);

@SUPERCILEX
Copy link
Collaborator

@Sourav-21 I don't think you can add new load() methods with the generated API. In addition, you'll still be using it on your side—we just provide the loading mechanism.

@samtstern I'm not super familiar with providing loaders for Glide, but at a glance this LGTM. 😄

@akaSourav
Copy link

@SUPERCILEX I am just giving an example. load(storageRef) method with the generated API working fine.

@akaSourav
Copy link

akaSourav commented Sep 20, 2017

With the release of Glide 4.1.1. I suggest the following changes
update Glide dependencies to 4.1.1 is
provided 'com.github.bumptech.glide:glide:4.1.1'

Change the GlideModule

@GlideModule
public class MyGlideModule extends AppGlideModule {
    @Override
    public void registerComponents(Context context, Glide glide, Registry registry) {
        registry.append(StorageReference.class, InputStream.class, new FirebaseImageLoader.Factory());
    }
}

And use GlideApp instead of Glide for simplicity.

GlideApp.with(fragment)
    .load(storageRef)
    .centerCrop()
    .into(imageView);

@SUPERCILEX
Copy link
Collaborator

Ohhhhh, I get it. You're suggesting simplifying the sample. Yeah @samtstern that's probably a good idea.

@akaSourav
Copy link

akaSourav commented Sep 20, 2017

Yeah, I am suggesting you modify the demo. The changes @TheCraftKid has done with FirebaseImageLoader.java is great.

Change-Id: I00d8193756ee71b0ef1d51da9ecd766196a4c5a1
@samtstern
Copy link
Contributor Author

@Sourav-21 thanks for the pointer about GlideApp.

I think this is ready to go now.

Change-Id: If9eb0e450c434b8516b9a59fa61b556652bfb695
@samtstern samtstern merged commit 34adac2 into version-3.0.0-dev Sep 20, 2017
@samtstern samtstern deleted the glide-4.0 branch September 29, 2017 00:00
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.

5 participants