rosjava / android_core

Android libraries for rosjava
145 stars 166 forks source link

Android Wear OS 2 support (works on Melodic) #294

Closed roncapat closed 2 years ago

googlebot commented 5 years ago

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

roncapat commented 5 years ago

Hi @jubeira ,

Thank you for the quick comment! We would be very happy to contribute mainline, so we'll work to satisfy your requests.

roncapat commented 5 years ago

@lucregrassi please sign the CLA

lucregrassi commented 5 years ago

Done! @roncapat

googlebot commented 5 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 or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok 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 all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

roncapat commented 5 years ago

Deduplicating RosWearActivity and RosActivity code is not straightforward IMHO. They inherit from two different classes, although related (WearableActivity is derived from Activity)... any idea? I pushed a commit where the only differences of the two files, side-by-side is the swap from Activity to WearableActivity and from MasterChooser to MasterChooserWear.

Notice that also AppCompatRosActivity is duplicated, as an equivalent AppCompatRosWearActivity has to extend RosWearActivity instead of RosActivity

roncapat commented 5 years ago

The easiest path would be a code-generation step for generating both versions, since Java Generics do not support something like "extends T" unfortunately.

roncapat commented 5 years ago

@jubeira in the last commits, you can see:

Then, commit 50bd377 is just a draft for deduplicating code used by both RosWearActivity and RosActivity. In particular, two nested classes have been defined outside, at package level, as generics that take a reference to either a RosActivity or RosWearActivity reference. An interface has been defined, in order to describe what both Activities are required to offer. Only ~50 lines of code were saved, but now the Activites only have very short proxy methods. As I said, it's just a draft, and I don't like the fact that some protected stuff now is public for example.

googlebot commented 5 years ago

:frowning_face: Sorry, but only Googlers may change the label cla: yes.

jubeira commented 5 years ago

Thanks @roncapat! I took another quick look and I don't have a strong opinion against this approach.

Only ~50 lines of code were saved, but now the Activites only have very short proxy methods.

Even though the amount looks small, I'd still prefer this than duplicating code.

As I said, it's just a draft, and I don't like the fact that some protected stuff now is public for example.

The access specifiers is a good point indeed. Another approach would be using composition: a third object (separate from RosActivity and RosWearActivity) would handle all the common functionality between them. Then, both RosActivity and RosWearActivity would hold a reference to this object, and forward requests to it as needed (perhaps using different parameters where necessary).

This way the methods could still be protected on both top level classes while sharing the code. You could even make this new object implement RosInterface to standardize future implementations if they ever come up (RosAutoActivity??).

Again, this is not a strong opinion but a slightly higher preference; I would be fine with reviewing the draft in depth as well. Please let me know what you think.

/cc @ernestmc (JIC you have comments to add :slightly_smiling_face: )

EliteMasterEric commented 4 years ago

@jubeira what happened to this pull request? What changes still need to be made for it to be merged?

roncapat commented 4 years ago

Another approach would be using composition: a third object (separate from RosActivity and RosWearActivity) would handle all the common functionality between them. Then, both RosActivity and RosWearActivity would hold a reference to this object, and forward requests to it as needed (perhaps using different parameters where necessary).

This way the methods could still be protected on both top level classes while sharing the code. You could even make this new object implement RosInterface to standardize future implementations if they ever come up (RosAutoActivity??).

Again, this is not a strong opinion but a slightly higher preference; I would be fine with reviewing the draft in depth as well. Please let me know what you think.

I believe he was waiting for a revision by me implementing his proposals here quoted, but unfortunately, I moved to other projects. The patch itself works as it is now, we use it in various projects in University of Genova, Italy, for different experiments in gesture based interfaces for robotic applications based on commercial smartwatches.

roncapat commented 2 years ago

Closing for obsolescence