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

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

Open
wants to merge 11 commits into
base: kinetic
from

Conversation

Projects
None yet
4 participants
@roncapat
Copy link

commented Mar 7, 2019

No description provided.

roncapat added some commits Feb 15, 2019

Wear OS 2 port
This patch allows ROS to operate on a WearOS 2 smartwatch.
Android Wear full support (#2)
* Porting to Wear OS 2.0

* Add MasterChooserWear

* Keep both RosActivity and RosWearActivity

@googlebot googlebot added the cla: yes label Mar 7, 2019

roncapat and others added some commits Mar 7, 2019

@googlebot

This comment has been minimized.

Copy link

commented Mar 7, 2019

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.

@googlebot googlebot added cla: no and removed cla: yes labels Mar 7, 2019

@jubeira
Copy link
Contributor

left a comment

Thanks for submitting this @roncapat @lucregrassi !

Before doing a review in depth I have some overall comments / questions (I'm not super familiar with Wear devices, sorry if the questions are a bit silly):

  • Is it necessary to bump the minSdkVersion to 23? I wouldn't mind bumping it to 19 nowadays, but even being somewhat old devices, 23 seems a bit high.
  • I think it's a good idea to apply a linter to the codebase, but if you could please apply it in a separate PR with just lint changes, that would help a lot to review the changes so as not to mix things.
  • I see that you added a Wear activity which is pretty much the RosActivity with some minor adaptations. I'd rather not have two activities that do pretty much the same with different implementations. Do you think it's possible to isolate the functionality for the RosActivity in an object, and then compose that object into both RosActivity and RosWearActivity? That way we would have only one piece of code handling the logic, which would be shared by both classes.

That said, supporting Wear devices sounds like a great contribution!

@roncapat

This comment has been minimized.

Copy link
Author

commented Mar 7, 2019

Hi @jubeira ,

  • no, probably we can still keep v19, we'll check and report;

  • yes, having duplicate code is not ideal, we will investigate in the best way to deduplicate that

  • we'll work to separate linting as a different PR

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

@roncapat

This comment has been minimized.

Copy link
Author

commented Mar 8, 2019

@lucregrassi please sign the CLA

@lucregrassi

This comment has been minimized.

Copy link

commented Mar 8, 2019

Done! @roncapat

@googlebot

This comment has been minimized.

Copy link

commented Mar 8, 2019

So there's good news and bad news.

👍 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.

😕 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

This comment has been minimized.

Copy link
Author

commented Mar 9, 2019

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

This comment has been minimized.

Copy link
Author

commented Mar 9, 2019

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 added some commits Mar 9, 2019

@roncapat roncapat force-pushed the EmaroLab:wear_os_2 branch from ddecad8 to 513a11c Mar 9, 2019

@roncapat roncapat force-pushed the EmaroLab:wear_os_2 branch from 513a11c to 2104e29 Mar 9, 2019

@roncapat

This comment has been minimized.

Copy link
Author

commented Mar 10, 2019

@jubeira in the last commits, you can see:

  • downgrade to minSdkVersion 19
  • rollback of the reformats/linting

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.

@jubeira jubeira added cla: yes and removed cla: no labels Mar 11, 2019

@googlebot

This comment has been minimized.

Copy link

commented Mar 11, 2019

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

@googlebot googlebot removed the cla: yes label Mar 11, 2019

@jubeira

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

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 🙂 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.