-
Notifications
You must be signed in to change notification settings - Fork 205
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
Add hotkeys (also to help) #312
Conversation
92f64cb
to
9e1feb4
Compare
9e1feb4
to
0d274d3
Compare
5997b63
to
5872a16
Compare
0d274d3
to
5c89d87
Compare
@@ -73,6 +73,8 @@ FocusTool::~FocusTool() = default; | |||
|
|||
void FocusTool::onInitialize() | |||
{ | |||
shortcut_key_ = 'c'; |
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.
Should this be in the constructor like the other two?
@@ -58,6 +58,8 @@ namespace tools | |||
MeasureTool::MeasureTool() | |||
: is_line_started_(false), length_(-1) | |||
{ | |||
shortcut_key_ = 'n'; |
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.
Should all of these be part of the member initialization section? : is_line_..., shurtcut_key_('n')
?
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.
The problem is that they are protected members of the rviz_common::Tool
class, so I can't do that - I would have to provide another non-default constructor for Tool.
There is a comment for the Tool constructor saying that the shortcut_key_
member should be initialized in the constructor.
5c89d87
to
329947a
Compare
Closes #304
Also added hotkeys to help description to make discovery easy.
Note: This PR should probably only be merged when a corresponding PR is merged with ros-visualization/rviz