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

Add GlideModule for lazily initialization. #311

Merged
merged 2 commits into from
Jan 22, 2015
Merged

Conversation

sjudd
Copy link
Collaborator

@sjudd sjudd commented Jan 21, 2015

Fixes #299.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 86.28% when pulling 1e5ebf1 on sjudd:master into e099ef6 on bumptech:master.

@TWiStErRob
Copy link
Collaborator

Concern: metadata without a value looks strange.
Wouldn't it be better if you did name="com.bumptech.glide.module.flickr" value="full.class.name", less parsing (no need for substring, but you can keep the startswith check) and better readability, compliance.
The doc for metadata.name says to prefix your name with package: http://developer.android.com/guide/topics/manifest/meta-data-element.html

@sjudd
Copy link
Collaborator Author

sjudd commented Jan 21, 2015

@TWiStErRob The reason why the fully qualified class name has to be in the key is that there can be only one metadata tag for a given key. If we use anything other than the fully qualified class name, it's possible a user would accidentally try to add two different implementations with the same key (two different classes in the same package for example). If they did so, we'd have no way to detect the error because the manifest merger/parser would silently merge the two.

I agree the syntax is funky, but unless there's some way around the fully qualified class name issue, I'm not sure we have anything useful to put in the value.

@TWiStErRob
Copy link
Collaborator

Note that I still left flicker in the name, which is just a random name. That name after that is not used anywhere just helps readability, users can organize modules nicer. E.g. for okhttp it would be different, the point is to start it with ...glidemodule..
I read the design doc lately and as I remember the merger will conflict if two modules have the same name after glidemodule, but the values are different.
The best would be a multivalued metadata which is not supported sadly.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 86.28% when pulling d6eb14d on sjudd:master into e099ef6 on bumptech:master.

@TWiStErRob
Copy link
Collaborator

Also in case of placeholders (e.g. I hate duplicating the package name so I have a placeholder for it like ${packageName}.subpackage.Class) it's cleaner to start with that instead of having it in the middle. Placeholders are required because the .sub.Class format support is limited to a few elements.

@sjudd
Copy link
Collaborator Author

sjudd commented Jan 21, 2015

We've actually tried something like this with user specifiable names, and found that people would still occasionally choose the same name. Using the class name just completely eliminates the possibility, unless they add the same class twice, in which case ignoring the second copy should be fine.

We could drop glidemodule: altogether, but then we wouldn't be able to assert as much (malformed class name and failing to implement GlideModule would have to be ok).

We could append :glidemodule instead if you think that's cleaner?

@sjudd
Copy link
Collaborator Author

sjudd commented Jan 21, 2015

Another option would be to leave the fully qualified class name as the key, but set glidemodule as the value. Maybe that's cleaner? It would at least avoid the string parsing.

@TWiStErRob
Copy link
Collaborator

Hmm that's interesting... just realized: all of these are hacks, so I guess the last one is the best of them you're taking onto consideration.

Rant follows :) Your response to people occasionally choosing the same names is the approach I saw at my university in the classes following my year, a lot of students were failing the most basic programming tests so they made them easier every semester. My view/approach is always: use stuff as designed, so keys as keys and value as values. End of rant.

@sjudd
Copy link
Collaborator Author

sjudd commented Jan 21, 2015

I hear you. It's worth keeping in mind though that it's not even necessarily possible for people to do the right thing. If you have a number of independent library projects, each owned by separate teams, and multiple of those teams define their own modules, we don't want to force people to go inspect all of the other team's modules before they come up with a module name (or keep a list of all used names somewhere...).

I agree it's the best of bad options, I'll move module into the value and remove the prefix.

@sjudd
Copy link
Collaborator Author

sjudd commented Jan 21, 2015

Made the change, PTAL, thanks for the input!

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 86.28% when pulling d83859f on sjudd:master into e099ef6 on bumptech:master.

@sjudd
Copy link
Collaborator Author

sjudd commented Jan 21, 2015

I added documentation about manifest merging, hopefully I got it right (having not done this in gradle before...)

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 86.28% when pulling b986f0a on sjudd:master into e099ef6 on bumptech:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 86.28% when pulling c38e4c3 on sjudd:master into e099ef6 on bumptech:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 86.26% when pulling 7fb3601 on sjudd:master into e099ef6 on bumptech:master.

@sjudd sjudd merged commit 7fb3601 into bumptech:master Jan 22, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reduce Glide setup time
3 participants