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

Migrate measure tool #264

Merged
merged 15 commits into from
May 23, 2018
Merged

Conversation

anhosi
Copy link
Contributor

@anhosi anhosi commented May 16, 2018

Closes #212.

It also introduces a new property for the measure tool that allows to configure the color of the measurement line as the old default was barely visible in front of the default background. The default color was changed to a dark yellow. This property is visible once #251 is merged.

@anhosi
Copy link
Contributor Author

anhosi commented May 16, 2018

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Edit: Restarting ci jobs that failed due to ssh_agend disconnect.

@Martin-Idel-SI
Copy link
Contributor

Martin-Idel-SI commented May 17, 2018

Next try (after adding gmock to package xml):

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@wjwwood wjwwood added the in review Waiting for review (Kanban column) label May 23, 2018
@wjwwood wjwwood added this to the bouncy milestone May 23, 2018
@wjwwood wjwwood added the 🔥 label May 23, 2018
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

lgtm, I had some small comments, but they can be addressed separately or not at all.

*/

#include <gtest/gtest.h>
#include <gmock/gmock.h>
Copy link
Member

Choose a reason for hiding this comment

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

I think you should only include one or the other here. I don't think it will cause a problem, but I do think it's unnecessary as I think gmock include gtest for you. At some point in the past it could cause problems to include both, but I think that's been resolved at this point. Either way, consider not doing this in the future or a small follow up pr to change this instance.

Copy link
Contributor

Choose a reason for hiding this comment

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

We'll make a small followup PR, because there are also some other small things I'd like to polish (e.g. the select tool test isn't included in the CMakeLists, etc.). Thanks for pointing this out to us!

@@ -28,6 +28,7 @@
*/

#include <gtest/gtest.h>
#include <gmock/gmock.h>
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

Copy link
Member

Choose a reason for hiding this comment

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

And several places more in this pr, but I won't flag all of them.

@wjwwood wjwwood merged commit 36780ec into ros2:ros2 May 23, 2018
@Martin-Idel-SI Martin-Idel-SI deleted the feature/migrate_measure_tool branch May 23, 2018 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔥 in review Waiting for review (Kanban column)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants