Move to compile target Marshmallow (Android 6 - v23)

[AndyScherzinger]

This is a PR ready to test drive and review and so on, probably some further discussion...

Beware: This Branch is based on material_fab - works for gradle and for ant

What has been done so far:

• Bumped compile target to 23
• Bumped AppCompat to 23.1.1
• Bumped build tools to 23.0.2 - same for the travis build
• Fixed MediaService implementation
  • Moved on to NotificationCompat.Builder (as it is done in all aother placed within the app) since the Notification.setLatestEventInfo() has been removed with Android M, see http://developer.android.com/about/versions/marshmallow/android-6.0-changes.html#behavior-notifications
  • Also added the notification background coloring as done on master for other notifications and also added the material design "play button" icon for the notification/system bar
• Fixed Apache HTTP
  • With Android M, Apache HTTP has been removed from Android (!). I added the Android suggestion in case you "have" to stay with apache http, see http://developer.android.com/about/versions/marshmallow/android-6.0-changes.html#behavior-apache-http-client - This could be refactored in a future, separate PR
• Added AppCompat Design lib 23.1.1
• ONGOING: v23 permission API calls for write access to external storage (necessary since compikle target has been bumped to 23)

Pinging @davivel @masensio @rperezb for the Upgrade fact and the technical debt (which should be fixed in the future, see apache http)

Pinging @tobiasKaminsky @LukeOwncloud @stoyicker for community code review

Pinging @jancborchardt due to the fact that setting that background color (now also) for the Media player Notification is a UI change - a minor one, but a change nonetheless.

[masensio]

Hi @AndyScherzinger,

Thanks for working in this PR. It is very interesting for us.
We want to merge this change on master branch, so it would be better if this PR were independent of the FAB branch, because we prefer to prioritaze this PR before the FAB PR #1100.
So, could you make this PR independent of FAB PR?

Also this branch has conflicts with master, could you rebase with master?

[AndyScherzinger]

Hi @masensio sure thing, I can do that, but this PR is still under development since up to now I only added the changes necessary file access but not for contacts even though I don't know where the contacts would be accessed, but I can add that code change, probably over the weekend.

I'll try to rebase but maybe I need to do the changes again, because whenever I tried to rebase something goes wrong (I still don't fully know how to rebase 😞)

Please be aware that this branch "only" has a initial rudimentary implementation checing for the permissions when the app starts. I does not (yet) check for the permission in all the places where they are needed which we should do at some point since with Marshmallow I can revoke permission at any time even while the app is running. But that is something which I would consider mid-priority not high. :)

This PR would also fix #1401 😁

[masensio]

Hi @AndyScherzinger,

I know when you rebase you have to fix the conflicts step by step, but the repository history is cleaner at the end. I suppose you know the steps to start a rebase, and your problems with that process are the conflicts founded in the middle of the process. You need to fix these conflicts and continue with the rebase. If I can help you with thisin any way please let me know.

On the other hand, I know this PR is not ready to review, I've seen the Developing label, but I've seen it is not master branch dependent and I prefer tell you that we need this PR independent of FAB branch before the end of development.

[AndyScherzinger]

@masensio Yes please. I have never successfully done a rebase (resolving conflicts isn't the issue) but I simple don't know how to do this (with tortoiseGit) So if you have any hints for me. I googled but I just don't get it 😢

[AndyScherzinger]

I can even name all the commits that need to be kept if that is of any help...

It is basically all commits starting @ #1220 (commits)

[davivel]

Hi, @AndyScherzinger . Joining here to bear a hand. Let me have a look.

[davivel]

I only added the changes necessary file access but not for contacts even though I don't know where the contacts would be accessed,

I don't understand, why do you want to add permission to access contacts? The app doesn't need nor use it.

[AndyScherzinger]

@davivel Sweet! :) - Still up for discussion is if we need to redo the changes I mentioned since @masensio wants the PR to be based on master not on #1100
On the other hand @MTRichards wrote that #1100 is in the merge pipeline but open issue imho is the lib discussion. If the lib is fine for now than #1100 can be merged, and than this one is on par again with master.

For now this PR only implemented the file system permission check, open issue here is the contacts permission check.

[AndyScherzinger]

@davivel I don't want to add the permission! But if you open up the app permission in the system settings you will see that for some reason the app defines permissions for file access and contacts. That hasn't been added by me. I just moved to the new permission implementation, so the branch gives you the implementation to ask for the file access permission.

[davivel]

@masensio wants the PR to be based on master not on #1100
On the other hand @MTRichards wrote that #1100 is in the merge pipeline but open issue imho is the lib discussion.

Yes, the point is that we need the lib update before than the FAB. The target is fixing #1401, that you researched about. We'd prefer to invert the order because testing on FAB will be more extensive, while the upgrade to v23 is mainly technical and will not require QA time.

[davivel]

OK, I understand. That's weird... will check it.

[davivel]

In any case, is it strictly needed to add the dynamic permission handling to get the compilation target in v23 working?

[AndyScherzinger]

Sure thing, sounds fine to me, while I have to say in order to do it right (permission implementation) it would need quite the QA since on Android M I can revoke any granted right at any point in time, so permission checks would need to be implemented at all places where it is needed.

Thanks for the collaboration on this issue! 👍

Regarding you question: Yes dynamic permission handling is the way of the 23 API. For a quick win scanario I think we can start with simply having the start check I already implemented and not having check in all the other places, so whenever a user revokes permissions things won't work again until he restarts the app and get's asked again for the permission (this is all already implemented on this branch).

[davivel]

Sounds fine. In any case, we'll have to push up the rest of permission handling in priority. D**n dependencies...

cc @rperezb

[davivel]

OK, found the reason. GET_ACCOUNTS permission is in the CONTACTS group, and the interaction of the user is to approve or deny permission groups, that's why it's accessible in settings.

[davivel]

Reference: http://developer.android.com/intl/es/guide/topics/security/permissions.html#normal-dangerous

EDIT: real reference: http://developer.android.com/intl/es/guide/topics/security/permissions.html#perm-groups

[davivel]

I transplanted target_marshmallow from material_fab to master. Pushed it in a different branch, new PR is  #1473, still in progress.

Let's follow there :)

[davivel]

Keeping branch alive some time just in case I screwed something in the rebase process.