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

KeyPublisher #81

Merged
merged 9 commits into from
Jun 23, 2020
Merged

KeyPublisher #81

merged 9 commits into from
Jun 23, 2020

Conversation

mohamedsayed18
Copy link
Contributor

a plugin that publish the key strokes of the keyboard with type Int32

@chapulina chapulina added enhancement New feature or request 🔮 dome Ignition Dome labels Jun 15, 2020
@mohamedsayed18 mohamedsayed18 changed the base branch from master to ign-gui3 June 15, 2020 18:35
@chapulina chapulina added 🏰 citadel Ignition Citadel and removed 🔮 dome Ignition Dome labels Jun 15, 2020
Copy link
Contributor

@chapulina chapulina left a 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!

include/ignition/gui/plugins/KeyPublisher.hh Outdated Show resolved Hide resolved
include/ignition/gui/plugins/KeyPublisher.hh Outdated Show resolved Hide resolved
include/ignition/gui/plugins/KeyPublisher.hh Outdated Show resolved Hide resolved
include/ignition/gui/plugins/KeyPublisher.hh Outdated Show resolved Hide resolved
include/ignition/gui/plugins/KeyPublisher.qml Outdated Show resolved Hide resolved
src/plugins/KeyPublisher.cc Outdated Show resolved Hide resolved
src/plugins/KeyPublisher.cc Outdated Show resolved Hide resolved
include/ignition/gui/plugins/KeyPublisher.hh Outdated Show resolved Hide resolved
include/ignition/gui/plugins/KeyPublisher.hh Outdated Show resolved Hide resolved
src/plugins/KeyPublisher.cc Outdated Show resolved Hide resolved
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]>
Signed-off-by: mohamedsayed18 <[email protected]>
Copy link
Contributor

@chapulina chapulina left a 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!

src/plugins/key_publisher/KeyPublisher.cc Outdated Show resolved Hide resolved
mohamedsayed18 and others added 3 commits June 22, 2020 20:06
Signed-off-by: mohamedsayed18 <[email protected]>
Signed-off-by: mohamedsayed18 <[email protected]>
Copy link
Contributor

@chapulina chapulina left a 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

keypress

@chapulina chapulina merged commit 9ec2140 into gazebosim:ign-gui3 Jun 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants