abrenoch / hyperion-android-grabber

Screen grabber for hyperion
MIT License
201 stars 35 forks source link

Refactor/common library #32

Closed ninovanhooff closed 6 years ago

ninovanhooff commented 6 years ago

I deduplicated the tv and mobile code. The shared code is moved to the common module. I still need to test it fully, but I thought I'd already submit a request because if you want to accept this, you probably want to merge it before adding more commits of your own

ninovanhooff commented 6 years ago

Note that the functionality of the app does not change in any way, but it is now much easier to maintaiun the code

abrenoch commented 6 years ago

Hey this is really great, thanks! I had not quite got around to figuring out how to use a common directory for both mobile and tv yet, so this is really helpful!

It looks like you are more or less using some gradle settings to share the common directory to both versions, correct? Would you mind pointing out the gradle files that make that happen? I would just like to know how that works better!

One thing I did notice, and maybe it isn't even a problem, in common/src/main/AndroidManifest.xml I saw the package name is com.abrenoch.hyperiongrabber.common - that would not interfere with how the play store checks the apk when uploading would it? I really don't have much experience with that platform, but I know it is strict about package names.

Thanks for the work! Would love to get this merged ASAP, just curious about the play store thing!

ninovanhooff commented 6 years ago

Not problem! Gradle is smart enough to merge xml files with the same name. In Android studio, open tv/src/main/AndroidManifest.xml. Click the Merged manifest tab below the editor. This will show you how it merged the current Manifest with all of the dependencies. (the common library is a dependency). Note that the package name is com.abrenoch.hyperiongrabber.tv

image

It basically works like this: defined in common: available to common, tv, nmobile defined in tv: availble to tv only (common won't compile if it uses tv code) defined in common and tv: tv overrides common

This works for other xml files too (strings, styles, values)

So you can have a string called app_name in common with value "Hyperion Grabber" And the same in tv with value "TV Grabber"

When mobile does not define it at all, mobile will show "Hyperion Grabber" and tv will show "TV Grabber"

https://developer.android.com/studio/build/manifest-merge

ninovanhooff commented 6 years ago

Btw @abrenoch , I found this project because I was doing research for creating the same kind of thing, I was happy to see you found MediaProjection etc!

I am a professional Android dev and would like to stay involved for some spare hours every week for the next 2 months. If you would like that, we could start using some free collaboration tools like Slack and Trello to make sure we don't do double work.

ninovanhooff commented 6 years ago

Ready to merge. @abrenoch : if you like it, please accept soon, or we might run into merge conflicts with future PR's

abrenoch commented 6 years ago

@ninovanhooff Collaboration sounds awesome! I would love to have someone on board that is really familiar with android development, I honestly do not know very much about it and had to learn a lot to get this far (not to mention this is about the extent of my experience with java). I always figured if I did the work to get the ball rolling someone more knowledgeable would show up and help me out eventually 😆

I've used neither slack or trello, have a preference for one or the other?

Thanks again!

abrenoch commented 6 years ago

@ninovanhooff Just one question about the version codes in the manifest:

It looks like currently the version code is being stored in the common AndroidManifest file rather than within the mobile or tv directories - I may be wrong here but I'm pretty sure then when uploading multiple apks under one application (within the play console, in order to deploy to different devices), their version codes need to be different (per here).

Be that the case, would we then just simply add the version code in the tv/mobile AndroidManifest instead? Seems like that is probably the case bu wanted to check 😃

ninovanhooff commented 6 years ago

A package name is what defines "the same application". The way it is currently setup is that we will publish 2 separate apps.

With a device attached to your pc, you can list all installed packages with

adb shell "pm list packages -f"

My phone has com.google.android.youtube installed, while my shield tv has com.google.android.youtube.tv. So I thought we'd best follow that example. The result is that:

The last point is interesting: we'll have to configure the separate manifests so that each app only shows up in either the TV or phone store sections. That should be possible.

Version codes could be equal between TV and phone since they are separate apps, but I guess it would be best to define version codes in the separate manifests because we might want to update the app code multiple times for the same version of the common core

ninovanhooff commented 6 years ago

Trello is an online task board. You can organise items in todo, in progress and done for example

Slack is professional chat, including very convenient ways to share files and discuss features. We could use both these tools and invite people who have had 1 pull request accepted to join.

If you could create a free account for both, and share the project names here, that would be great

abrenoch commented 6 years ago

Here are some links!

https://hg-android.slack.com https://trello.com/b/hjG9KmPr/hyperion-android-grabber

Just let me know if I need to send an invite or anything like that!

ninovanhooff commented 6 years ago

Thanks, could you send Slack invite to ninovanhooff [at] gmail.com? Thanks As for Trello, I found out Github might support this functionality as well: https://help.github.com/articles/about-project-boards/#templates-for-project-boards

Maybe give that a try?

abrenoch commented 6 years ago

Hey just sent the slack invite, and we can try out the git project boards for now (already familiar with those from my day job). I'll add you as a collaborator!