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.

Sign up for GitHub

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

Jump to bottom

Add vector drawable support via appcompat #1946

Merged
merged 1 commit into from
May 24, 2017
Merged

Add vector drawable support via appcompat #1946

merged 1 commit into from
May 24, 2017

Conversation

FrancoisBlavoet
Copy link
Contributor

Description

use the support lib in order to inflate resources.
This allows you to use the ids of VectorDrawables for placeholder, error or fallback calls.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

@FrancoisBlavoet FrancoisBlavoet changed the title Svg Add support lib SVG support May 19, 2017
@googlebot
Copy link

CLAs look good, thanks!

@TWiStErRob TWiStErRob self-requested a review May 21, 2017 12:05
TWiStErRob
TWiStErRob requested changes May 21, 2017
View reviewed changes
Copy link
Collaborator

@TWiStErRob TWiStErRob left a comment

Choose a reason for hiding this comment

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

I don't think that's SVG, there's no SVG in Android as far as I know, only vector drawables:
https://developer.android.com/reference/android/graphics/drawable/VectorDrawable.html

The path descriptor mini-language is the only thing that's borrowed from SVG.

Please change the commit message to not mislead people.

library/build.gradle Outdated
@@ -20,7 +20,7 @@ dependencies {
compile project(':third_party:disklrucache')
compile project(':annotation')
compile "com.android.support:support-v4:${SUPPORT_V4_VERSION}"

compile "com.android.support:appcompat-v7:${SUPPORT_V7_VERSION}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be possible to make this optional? Without adding a hard dependency on v7?

Also, @sjudd, we should probably revise the v4 dep and use the more fine-grained exploded libraries (core, fragment, etc).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I'm not 100% sure it makes sense to add a hard dependency on v7 either. Maybe this should be an integration library? You can probably override the setPlaceholder methods with a GlideExtension, though it's a bit weird.

As an alternative Apps can also pass in Drawables directly, so those that are using v7 can call this method themselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would prefer to be able to pass an drawable id.

  • Terser calls (no need to initialise the drawable yourself, just pass the id)
  • No risk of a crash because you passed the id of a VD to glide (right now it does crash)
  • If you refactor a raster to VD, no need to check all the glide calls

Alternatively,

I have not toyed too much with reflection in this kind of use case but I think it might be possible to check for the presence of the class & method from the support lib and use them if they are here.
If it is not there, fallback to the previous method.

I would also need to add a proguard rule for this class in the library's rules. Although I don't know yet whether I can add a rule for a class that might not be present in the classpath.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would that be an acceptable implementation ?

Copy link
Collaborator

@TWiStErRob TWiStErRob May 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I think a better way would be to use a provided scope and try { loadVectorWithV7 } catch(ClassNotFoundException ex) { loadVectorWithV4 } (could cache failure so the weight of the exception only happens once). This way there's no need for proguard.

Copy link
Collaborator

@TWiStErRob TWiStErRob May 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

(I support the PR: the more formats we support out of the box, the better)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just read about provided dependencies
Indeed it looks like a good way to do this.

samples/svg/src/main/res/drawable/image_loading.xml Outdated
<item>
<bitmap
android:src="@android:drawable/sym_def_app_icon"
android:gravity="center" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

According to the official docs this resource is available in O. It would be a breaking change for them to remove it without deprecation first. Maybe the preview is missing it (in which case report an AOSP bug), I would wait until a final release before taking action.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, removed

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm confused, it looks like you squashed it into the commit, not reverted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my bad, pushed too soon, fixed.

@TWiStErRob TWiStErRob changed the title Add support lib SVG support Add vector drawable support via appcompat May 21, 2017
@FrancoisBlavoet FrancoisBlavoet force-pushed the svg branch 2 times, most recently from 77592b5 to 4e30ffc Compare May 21, 2017 13:50
@FrancoisBlavoet
Copy link
Contributor Author

and yes, svg is an abuse of language.
Vector drawable is more like a subset of the svg standard mixed in with android constructs

@TWiStErRob TWiStErRob requested a review from sjudd May 21, 2017 18:26
@FrancoisBlavoet FrancoisBlavoet force-pushed the svg branch 5 times, most recently from 7b37a7e to 39fdfc9 Compare May 24, 2017 12:00
@FrancoisBlavoet
Copy link
Contributor Author

It should be ok now :-)
I have tested this commit on both pre and post L devices.

@sjudd
Copy link
Collaborator

sjudd commented May 24, 2017

From the failing travis build:
/home/travis/build/bumptech/glide/library/src/main/java/com/bumptech/glide/request/SingleRequest.java:359: error: Line is longer than 100 characters (found 101).

Otherwise looks good to me. Worth noting for the peanut gallery that this still won't let you call load() with a resource id that's a vector drawable. It will let you use vector drawables for placeholder, error, and fallback.

@FrancoisBlavoet
Add VectorDrawable support for placeholder
80f52c1 
This allows you to use VectorDrawable with the support lib
( vectorDrawables.useSupportLibrary = true)
for placeholder, error and fallback drawables.

Of course, if the id points to a regular (not vector drawable) resource, the
resource still inflate as usual.
@FrancoisBlavoet
Copy link
Contributor Author

My bad, I should not have added a javadoc at the last minute without running check afterwards.
Should be ok now

@sjudd sjudd merged commit bd9e265 into bumptech:master May 24, 2017
@sjudd
Copy link
Collaborator

sjudd commented May 24, 2017

Thanks!

@sjudd sjudd mentioned this pull request Oct 7, 2017
Closed
@TWiStErRob TWiStErRob added this to the 4.2 milestone Oct 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TWiStErRob TWiStErRob TWiStErRob approved these changes

@sjudd sjudd Awaiting requested review from sjudd

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.
4 participants
@FrancoisBlavoet @googlebot @sjudd @TWiStErRob