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

Move each plugin to its own directory #64

Merged
merged 2 commits into from
May 19, 2020
Merged

Conversation

chapulina
Copy link
Contributor

Builds on top of #63


We were not installing plugin headers, so there's no need for them to be in the include folder.

This pull request also moves each plugin into its own directory so the code is more organized and we can more cleanly add plugins with multiple files.

The new ign_gui_add_plugin function helps reduce the boilerplate for each plugin.

Other changes:

  • Removed QWT code that was commented out - we won't need that with QtQuick.
  • Removed the Requester and Responder cc files since those plugins were not being compiled.

CC @claireyywang and @AmrElsersy

Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina chapulina added enhancement New feature or request 🔮 dome Ignition Dome labels May 19, 2020
@chapulina chapulina self-assigned this May 19, 2020
@codecov
Copy link

codecov bot commented May 19, 2020

Codecov Report

Merging #64 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #64   +/-   ##
=======================================
  Coverage   14.56%   14.56%           
=======================================
  Files          13       13           
  Lines        1332     1332           
=======================================
  Hits          194      194           
  Misses       1138     1138           
Impacted Files Coverage Δ
src/plugins/world_control/WorldControl.cc 1.16% <ø> (ø)
src/plugins/world_stats/WorldStats.cc 1.12% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5995cfc...41f90ca. Read the comment docs.

claireyywang
claireyywang previously approved these changes May 19, 2020
Copy link
Contributor

@claireyywang claireyywang left a comment

Choose a reason for hiding this comment

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

Wow this is so neat! much easier to understand the structure now 🤩 I still need to try it locally, but seems like the CIs are failing. Missing dependency in CMakeLists.txt?

1: [GUI] [Err] [Application.cc:369] Failed to load plugin [TestBadInheritancePlugin] : couldn't get [ignition::gui::Plugin] interface.
1: [GUI] [Err] [Application.cc:333] Failed to load plugin [TestNotRegisteredPlugin] : couldn't load library on path [/var/lib/jenkins/workspace/ignition_gui-ci-pr_any-ubuntu_auto-amd64/build/lib/libTestNotRegisteredPlugin.so].

@claireyywang claireyywang dismissed their stale review May 19, 2020 18:23

clicked the wrong button

Base automatically changed from 3_to_4 to master May 19, 2020 20:44
Copy link
Contributor

@claireyywang claireyywang left a comment

Choose a reason for hiding this comment

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

LGTM! thanks for refactoring!

@chapulina
Copy link
Contributor Author

but seems like the CIs are failing

Those error messages are expected, because those tests are making sure that we properly handle invalid plugins and don't crash. I see some builds list these lines as "problems" though, so I ticketed an issue for that: gazebo-tooling/release-tools#205

@claireyywang
Copy link
Contributor

claireyywang commented May 19, 2020

but seems like the CIs are failing

Those error messages are expected, because those tests are making sure that we properly handle invalid plugins and don't crash. I see some builds list these lines as "problems" though, so I ticketed an issue for that: ignition-tooling/release-tools#205

I see. I was able to build locally with no error -> approval

@chapulina chapulina merged commit a60cb7d into master May 19, 2020
@chapulina chapulina deleted the reorganize_plugins branch May 19, 2020 22:28
@chapulina chapulina mentioned this pull request Jun 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔮 dome Ignition Dome enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants