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

Base & mobile modules revision #76

Merged
merged 92 commits into from
Nov 24, 2016
Merged

Base & mobile modules revision #76

merged 92 commits into from
Nov 24, 2016

Conversation

vzamanillo
Copy link
Collaborator

@vzamanillo vzamanillo commented Nov 16, 2016

  • Butterknife update

  • Picasso improvements

  • New response treatment for providers API (with jackson, removed slow GSON)

  • Other Java language fixes.

Simplified logic.
Fixed dereference of 'channel' may produce 'java.lang.NullPointerException'.
Moved to a public static variable
@vzamanillo
Copy link
Collaborator Author

A question, Who is the API owner?

@ChrisAlderson
Copy link
Member

Guess I am, I developed it.

@vzamanillo
Copy link
Collaborator Author

vzamanillo commented Nov 16, 2016

The common per quality torrent includes seeds and peers but there are an inconsistency with movies, for movies are seedand peer thats we have to extend the torrent per quality object and override the getter for both properties.

If we are the API owners the reponse and entity treatmen for each provider could be simplified a lot, we should not need formatListForPopcornor formatDetailForPopcorn methods anymore, I think we could work with the same models, not for Android app only, for desktop too.

I can contribute with the API backend to do some improvements if you need some help, I am not a good frontend developer, but I am very experienced with backend development.

@ChrisAlderson
Copy link
Member

At the time of developing the API I didn't touch the android or desktop version, so in order to keep those working I ended up using the old value names. Now I'm fine with changing it for consistency reasons.

@vzamanillo
Copy link
Collaborator Author

Thank you, Chris, first step should be change the seed and peer json properties for movies to unify it with the other providers, we can think about the model refactorization process later.

@ChrisAlderson
Copy link
Member

ChrisAlderson commented Nov 16, 2016

Two files would need to be changed:

And the current field names in the MongoDB database would need to be updated.

@vzamanillo
Copy link
Collaborator Author

vzamanillo commented Nov 16, 2016

Great!! tell me when we are in production to modify both, desktop and Android applications.

API mod PR ready

@vzamanillo
Copy link
Collaborator Author

vzamanillo commented Nov 16, 2016

This PR fixes #65 too, and probably others...

@vzamanillo
Copy link
Collaborator Author

I announce that we have an Android mobile version almost usable 😊

LeakCanary.install(this);
Foreground.init(this);

//initialise logging
if (BuildConfig.DEBUG) {
if (debugable || BuildConfig.DEBUG) {
Copy link

Choose a reason for hiding this comment

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

not totally sure, but the debuggable flag manages BuildConfig.DEBUG so this would be double checking the same thing

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that BuildConfig.DEBUG has no effect at Android Studio debugging runtime.

Copy link

Choose a reason for hiding this comment

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

According to this BuildConfig.DEBUG should be enough

@MrRory
Copy link

MrRory commented Nov 22, 2016

I was halfway when I decided that I'm not even going to look at it all. If it works, then 👍 wow. Some refactoring was really needed

@MrRory
Copy link

MrRory commented Nov 22, 2016

btw, i saw that butter moved forward with using dagger. maybe there is some way to merge it somewhere? not really easy i guess

@vzamanillo
Copy link
Collaborator Author

vzamanillo commented Nov 22, 2016

This is impossible to merge, I am afraid of we have to do it manually, dagger and OkHttp 3.

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.

None yet

4 participants