search

rosjava / android_core

Android libraries for rosjava
144 stars 166 forks source link

Add AppCompatRosActivity class #249

Closed PerrineAguiar closed 7 years ago

PerrineAguiar commented 7 years ago

This PR adds a AppCompatRosActivity class. This allows to use RosActivity with the android support library.

@adamantivm ptal.

adamantivm commented 7 years ago

@PerrineAguiar thanks so much for this contribution. One question: did you try building this project after this file is added? I imagine we would also require to list the additional support lib dependencies in build.gradle, no?

PerrineAguiar commented 7 years ago

@adamantivm I updated the gradle dependencies. Unfortunately I have troubles building the android_10 project in android studio, so I was not able to test if these changes are compiling. Do you think you can try to build it?

adamantivm commented 7 years ago

@PerrineAguiar thank you for the update. I see you changed the target SDK version from 10 to 22, but the projects in android_10 like the name suggest have been kept so far this way so as to make it backwards compatible all the way to SDK 10, so that change will break that assumption. Is 22 the lowest possible SDK version that is supported by the support library? If it could be lowered to say 15, then we could place this activity in android_15 instead. What do you think?

PerrineAguiar commented 7 years ago

@adamantivm Thanks for the explanation. According to the android doc: "Starting with Support Library version 24.2.0 (released in August 2016), the minimum supported API level has changed to Android 2.3 (API level 9) for all support library packages". So targeting SDK version 10 should be fine.

adamantivm commented 7 years ago

Ah, great! I'll try to have myself or @jubeira test if this builds fine and then it's good to go. @HelloEgg if you are interested in trying this out it would also be helpful.

adamantivm commented 7 years ago

So trying to build this as is yields exactly on the errors reported by @HelloEgg in #248 This is because it is necessary to use the same SDK to build than the version of support library used, in this case version 25. After updating compileSdkVersion to 25, the next error is that notification.setEventInfo has been deprecated, so it needs to be replaced by a builder as explained here

googlebot commented 7 years ago

So there's good news and bad news.

:thumbsup: The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

:confused: The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

adamantivm commented 7 years ago

I am the other committer and I am OK with contributing @googlebot

adamantivm commented 7 years ago

@PerrineAguiar so the only missing bit was updating compileSdkVersion to a version compatible to the support library version. I downgraded the support library used to version 22 because version 23 introduces an API change that would require changing some code. I think version 22 (Android 5.1.1) is sufficiently high for recent builds. We can think of updating beyond 22 at some point in the future.

@PerrineAguiar @jubeira could you give me your opinion on this change? If you both agree, we could merge the addition.

@HelloEgg FYI.

PerrineAguiar commented 7 years ago

@adamantivm Thanks for completing this work. I agree with using version 22 of the support library.

jubeira commented 7 years ago

@PerrineAguiar @adamantivm I had no problems when I built the latest update. I think that downgrading the Support Library's version is a better solution for now than keeping it at 23 or 24 with some API changes. As @adamantivm points out, than can be performed in another PR. MinSdkVersion remains the same, so that's OK too. :+1: on merging!