Skip to content
This repository was archived by the owner on Aug 11, 2023. It is now read-only.

This closes rosjava/rosjava_core#233. Adds fix for shutting down Defa…#234

Merged
adamantivm merged 1 commit intorosjava:indigofrom
stratomda:indigo
Mar 6, 2017
Merged

This closes rosjava/rosjava_core#233. Adds fix for shutting down Defa…#234
adamantivm merged 1 commit intorosjava:indigofrom
stratomda:indigo

Conversation

@stratomda
Copy link
Copy Markdown
Contributor

…ultNodeMainExecutor ListenerGroup to prevent leak in android when activities are destroyed. Added ability to remove listener from ListenerGroup to fix android_core issue #254.

…ecutor ListenerGroup to prevent leak in android when activities are destroyed. Added ability to remove listener from ListenerGroup to fix android_core issue rosjava#254.
@googlebot
Copy link
Copy Markdown

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@adamantivm
Copy link
Copy Markdown
Contributor

Awesome contribution. Thank you very much @stratomda. This is ready to merge as soon as the CLA bot is happy.

@adamantivm adamantivm self-requested a review March 3, 2017 20:09
@stratomda
Copy link
Copy Markdown
Contributor Author

stratomda commented Mar 3, 2017

I signed it!

Copy link
Copy Markdown
Contributor

@adamantivm adamantivm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

making sure we don't accidentally introduce unintended changes.

build.gradle Outdated

buildscript {
apply from: "https://github.com/rosjava/rosjava_bootstrap/raw/indigo/buildscript.gradle"
apply from: "https://github.com/stratomda/rosjava_bootstrap/raw/indigo/buildscript.gradle"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason for this change? This should probably be reverted before merging, right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this was a left over. Sorry bout that. Feel free to revert.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't commit the package bump in the package.xml file because I don't know how the package version numbering works. Let me know when you update the package number so i can include that reference in my android_core bug (rosjava/android_core#254). Thanks. I appreciate it!

@stratomda
Copy link
Copy Markdown
Contributor Author

Reverted. And also bumped package number. Sorry for the confusion. Reject if package number doesn't follow conventions.

@adamantivm
Copy link
Copy Markdown
Contributor

@stratomda for some reason the @googlebot still thinks there's something wrong with the CLA. Did you sign an individual CLA or a Company CLA? Could you try to verify if anything may not have gone right with that? otherwise I can try to follow-up with Google to see what's wrong.

About versioning, I prefer to bump the package version as part of the release process using standard ROS release tools. Also, I may wait for a couple more fixes or additions before pushing out a new version - unless you have a pressing need to have this one released to maven sooner than that. Bottom line, if you could revert the package version change I'd appreciate it.

Again, thank you very much for your contribution here, it's very valuable. We're almost there.

@adamantivm
Copy link
Copy Markdown
Contributor

@stratomda besides the CLA, it was just pointed to me that the PR is going to indigo. Could you target it to kinetic instead? Or do you need to do this in indigo for any particular purpose? I would have a strong preference to focus all our efforts on kinetic instead of indigo at this point.

@adamantivm adamantivm removed the cla: no label Mar 6, 2017
@adamantivm
Copy link
Copy Markdown
Contributor

@googlebot the CLA is signed, what's up?

@adamantivm
Copy link
Copy Markdown
Contributor

I had the CLA checked and verified in the Google system and it is OK to accept.

@adamantivm adamantivm merged commit 5ff54ed into rosjava:indigo Mar 6, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants