483/Remove PlusClient dependencies #589
Conversation
# Conflicts: # app/src/main/java/org/gdg/frisbee/android/event/EventOverviewFragment.java
| import okhttp3.Request; | ||
| import okhttp3.Response; | ||
|
|
||
| public class PlusImageResizer implements Interceptor { |
There was a problem hiding this comment.
This is what is left from the PlusPersonDownloader
There was a problem hiding this comment.
This gives size to the api but the intention of this is not to resize the image. It gets the image from Google+ URL.
There was a problem hiding this comment.
You are right. What about PlusImageFetcher?
There was a problem hiding this comment.
Or PlusImageUrlConverter?
There was a problem hiding this comment.
Yeah actually it is not a downloader. Converter makes really sense.
|
Oh man, this is huge. I will review this later tonight. This is great! |
| import java.io.OutputStream; | ||
| import java.net.HttpURLConnection; | ||
|
|
||
| public class GapiOkHttpRequest extends LowLevelHttpRequest { |
|
I thought this would be much much more difficult. I thought we would have to refactor whole NewsFeed parsing etc. This is really awesome. I just have couple of comments and that's all. |
|
BTW, this PR also includes the changes in #588 |
| + "nextPageToken," | ||
| + "items(id,published,url,object/content,verb," | ||
| + "object/attachments,annotation,object(plusoners,replies,resharers))" | ||
| + "&key=" + BuildConfig.IP_SIMPLE_API_ACCESS_KEY) |
There was a problem hiding this comment.
Using key here (instead of in an interceptor) makes it more explicit where the authentication takes place. I prefer this solution of a
public class ApiKeyAdder implements Interceptor {
@Override
public Response intercept(Chain chain) throws IOException {
Request original = chain.request();
HttpUrl newUrl = original.url().newBuilder()
.addQueryParameter("key", BuildConfig.IP_SIMPLE_API_ACCESS_KEY).build();
Request.Builder requestBuilder = original.newBuilder().url(newUrl);
Request request = requestBuilder.build();
return chain.proceed(request);
}
}
There was a problem hiding this comment.
I really like this interceptor. API keys and Auth related things usually done in interceptor anyways. That's what Square does
| public static final String CACHE_KEY_PERSON = "person_"; | ||
| public static final String CACHE_KEY_NEWS = "news_"; | ||
| public static final String CACHE_KEY_PERSON = "person2_"; | ||
| public static final String CACHE_KEY_NEWS = "news2_"; |
There was a problem hiding this comment.
new model classes -> new cache keys
|
We are hitting now User Rate limits for the people api on g+ in the infofragment on chapters with more than 5 organizers. Organizers are shown as "Name not known" We should move to Plus.PeopleApi.load that allows to retrieve all organizers in one call. @tasomaniac Do you agree? Even though you don't like that api. Change is out of scope for this PR, though. |
| @@ -407,7 +407,7 @@ public void onNotFound(final String key) { | |||
| @Override | |||
| public void success(Person person) { | |||
| if (person != null) { | |||
| App.getInstance().getModelCache().put(key, person); | |||
There was a problem hiding this comment.
👍 We should look into the usages of put and make all of them async. The ModelCache causes a lot of issues in strict mode.
| @@ -124,28 +93,38 @@ public void onActivityCreated(Bundle savedInstanceState) { | |||
|
|
|||
| final String chapterPlusId = getArguments().getString(Const.EXTRA_PLUS_ID); | |||
| if (Utils.isOnline(getActivity())) { | |||
| new Builder<>(String.class, Person.class) | |||
| .setOnBackgroundExecuteListener(new CommonAsyncTask.OnBackgroundExecuteListener<String, Person>() { | |||
| App.getInstance().getModelCache().getAsync(Const.CACHE_KEY_PERSON + chapterPlusId, | |||
There was a problem hiding this comment.
This line now exists in the else statement as well here
I don't think this isOnline check is helpful. Because of that we have a hell of indentation here. I think shouldn't have getAsync from model 2 times.
What we can do is that, we can first try to get it online, if that is unsuccesful, we can try to get from the cache, if that fails, we can use setIsloading(false)
There was a problem hiding this comment.
Let's refactor this in a new PR.
|
LGTM |
483/Remove PlusClient dependencies
This PR removes the dependency to the Google Plus Client (json/java) and removes the dependencies.
Now the app is below 65k methods and is used without multidex.
This PR includes also the changes of #587