-
Notifications
You must be signed in to change notification settings - Fork 44
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
KeyPublisher #81
KeyPublisher #81
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works great! I left several comments, mainly related to style.
We're trying to get better about documenting features as we add them. So how about adding an example config that uses this plugin to examples/config
, and documenting it in this tutorial? This config worked for me:
<?xml version="1.0"?>
<plugin filename="KeyPublisher">
</plugin>
<plugin filename="TopicEcho">
</plugin>
Then you open with ign gui -c path_to_config
, type the correct topic in the Topic Echo plugin and you see the key messages!
Signed-off-by: mohamedsayed18 <[email protected]> rebase
Signed-off-by: mohamedsayed18 <[email protected]>
Signed-off-by: mohamedsayed18 <[email protected]>
Signed-off-by: mohamedsayed18 <[email protected]>
Signed-off-by: mohamedsayed18 <[email protected]>
2edd280
to
17ad992
Compare
Signed-off-by: mohamedsayed18 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is working great, thanks for refactoring!
Just a couple more things left:
There are some codechecker errors left, you can check it locally too:
$ bash ./tools/code_check.sh
./src/plugins/KeyPublisher.cc:23: Found C system header after C++ system header. Should be: KeyPublisher.h, c system, c++ system, other. [build/include_order] [4]
./src/plugins/KeyPublisher.cc:37: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line] [4]
./src/plugins/KeyPublisher.cc:60: Lines should be <= 80 characters long [whitespace/line_length] [2]
./include/ignition/gui/plugins/KeyPublisher.hh:35: Should have a space between // and comment [whitespace/comments] [4]
./include/ignition/gui/plugins/KeyPublisher.hh:44: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line] [4]
./include/ignition/gui/plugins/KeyPublisher.hh:48: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line] [4]
Total errors found: 6
Also, mind adding an item to this tutorial for the user to run the example config, change the echo topic to /keyboard/keypress
and see the keys being published? Thanks!
Signed-off-by: mohamedsayed18 <[email protected]>
Signed-off-by: mohamedsayed18 <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works great! I pushed one last cppcheck fix and the tutorial note on 8df6f35
a plugin that publish the key strokes of the keyboard with type Int32