allow tools to get access to any keyevent #838
Conversation
with this patch the shortkey handling of rviz is altered to operate as such: the Tool baseclass offers a keep_control_ flag that if set to true allows a Tool to get all key events even those that usually are used as shortkeys to activate tools in oder to do so the QACtion handling within the visualization_frame was altered to check if a tool keeps control and if so it generates a KeyEvent from the actions shortkey and passes it to the tool
| @@ -76,6 +76,8 @@ Q_OBJECT | |||
|
|
|||
| char getShortcutKey() { return shortcut_key_; } | |||
|
|
|||
| bool keepControl() { return keep_control_;} | |||
There was a problem hiding this comment.
Can you place a between the;and the}?
|
Thanks for taking the time to put together this pull request and thoughtful description. I think the technical approach for this first step is solid, and after doing a code review the changes look good too (other than a few stylistic details). One thought I had though was, should we update any of the existing tools to use this new keep control mechanism? For example you mentioned the selection tool which can use Pending the above questions and comments, I think I would like to merge this pull request soon so that we can get some soak time on it in the next release. As for the "future" work you mentioned in section Once this pull request is merged, it would be great if you could continue this conversation in a new issue or pull request and be the champion for this general topic in rviz. Thanks again for the contribution! |
|
hi wjwwood, Regarding the future work, I'm happy to prepare a proposal for an extended key handling. I'll try to allocate some time for this asap and create an PR once I'm done. |
the Tool parameter 'keep_control' and it's getter function 'keepControl()' is now called 'access_all_keys_' and 'accessAllKeys()', respectively, since this nameing better indicates what's happening the visualization_frame.cpp which works with this parameter was changed accordingly
|
I now fixed the style issues and renamed the parameters from keepControl to accessAllKeys, which seems to be more suitable name for me. But... I would recommend not to merge this PR just yet. I just started to work on the "future work" and would like to include one feature: If a tool is selected by clicking the icon in the toolbar, the tool should be changed no matter if the current tool accesses all keys. Currently one has to first deactivate the tool first, before clicking the toolbar allows switching. I was aware of this the other day, and though that's fine, but right now this seems to be a bug to me, considering that we would change commonly used tools like "Select" as well. I'll fix that today. |
removed the shortkeys from the QActions in toolbar. so key events are no caught by the visualization_frame, but passed to the toolmanager (via the visualization_manager), where the tool switching logic (introduced in 8277331) is applied. this allows to let tools access all keys but still switch tools by clicking the toolbar.
hdeeken
force-pushed
the
hdeeken-indigo-devel
branch
from
62e92ae to
b8582f8
Compare
|
237c489 fixes the bug i mentioned above. The shortkey logic is now moved entirely into the tool manager. Which now allows to switch tools via click on the tool bar, even if the current tool has access to all key events. I further made changed the selection to, it is the only standard rviz tool that currently uses shortkeys internally. |
|
Cool, those changes make total sense to me. I was going to play test it a bit before merging, I'll try to do that today. Otherwise, are you happy with the current state of the pull request, is it ok to merge from your perspective? |
|
I just tested it out and the tool hotkeys seem to work as expected (so there at least wasn't a regression) as far as I can tell. |
|
Yeah, I'm happy with the PR and would say let's merge it, to give it "soak time" and the like! Can you tell me, when the next release will be happening? |
|
P.S. |
Soon, I have several fixes that need to go in and I want to maximize the time between releasing the next version and a sync of the new packages to the public
Honestly I haven't had time to get a handle on the documentation for rviz. There is quite a lot of stuff on the wiki in varying states, but the user guide is probably to most applicable user facing section for this issue: http://wiki.ros.org/rviz/UserGuide#Tools The developer documentation is a little more sparse, but there are related tutorials for making new tools: http://wiki.ros.org/rviz/Tutorials/Plugins%3A%20New%20Tool%20Type Which actually just redirects to: http://docs.ros.org/indigo/api/rviz_plugin_tutorials/html/tool_plugin_tutorial.html I think this last link would be the right place to add documentation about the changes in this pull request. That documentation is generated from: Which is in the |
allow tools to get access to any keyevent
|
Perfect, the last link was exactly what I was looking for. I'll add some docs there. |
The added comment explains why we are doing this. Signed-off-by: Chris Lalancette <[email protected]>
The current shortkey handling for RViz Tools is quite rigid, since it prevents tools from internally using shortkeys that overlay those defined to switch between tools. An issue, I stumbled across various times now and always worked around by choosing 'free' keys, but since those are limited, I would like to a) start a general discussion on the handling of shortkeys and get an idea what the community/the maintainers think about this issues, b) propose a minimal invasive patch that extendes the shortkey handling and c) offer my help to work out a proper shortkey handling.
a) General Discussion
I checked the source code an realized that the shortkeys that call a Tool are placed within the QActions which represent the Tools in the Toolbar and that if a shortkey is pressed the Toolbar deactivates the current tool and activates the tool connected to the QAction/shortkey. This leads to two counter-intuitive behaviors.
At first, it leads to deactivating and directly activating a tool again, if one presses the respective shortkey twice. Suppose a tool gathers some sort of data internally, which is flushed on deactivate(). In the current use case, this data would be lost, while the tool is still active, seem annoying to me. I would rather expect that pressing the shortkey of an inactive tool would activate it, and pressing the shortkey of an already active tool deactivates it and leads back to the default tool. Data might still be lost, but at least the UI would be intuitively stress this, due to the change to default.
Secondly, and more importantly, shortkeys that are meant not to activate a tool, but to support a tools usage are always overriden by the toolbar shortkeys, if those happen to be alike. As an example, consider the 'Select' Tool which features pressing 'f' to focus the camera on the currently selected item. If now another tool registers 'f' as it's shortkey and is added to the toolbar, pressing 'f' while using 'Select' results in switching to the other tool, not in focusing the item. Which is even more annoying, since the internal shortkeys of an active tool should have priority over the switching to another tool.
What do you think? Do you agree?
b) Patch
I cooked up a small patch that would get rid of these problems in a minimal invasive manner. Here's what i did:
The Tool BaseClass was extended by a boolean called 'keep_control_', which indicates if tool want's to get access to all keypresses that occur, even those that would usually activate another tool. By default this boolean is set to false, and thus there is no need for all Tools out there to be changed in any way.
If a shortkey is caught by on of the QActions representing the Tools it is checked whether the current tool want's to keepControl() or not. If so, the shortkey of the QAction is turned into a KeyEvent and handed down to the current tool, so that it can use it for it's purposes. If not, the tool is switched. Or reset to default, if the shortkey happens to be the one of the current tool, which is in fact the method to switch from a "control keeping" tool to any other.
These are the changes that come with the PR. To test the new keep_control_ feature, one can checkout this repository on the "keepControl" branch, where I extended the tool to declare keep_control_. I would be glad if these changes would be pulled.
c) Designing a proper shortkey management
But I think we could even go farther and develop an even better shortkey handling for RViz Tools. My PR is quite rigid in itself again, since a tool that "keeps control" just get's all key events even if it does not use them at all. I think I would be great if we did the following:
Let the Tool base class declare fields for the keys they operate on, each with a name, description and getter and setter methods. There should be a "primary shortkey" for activation and a set of "secondary shortkeys" for internal usage. Then let's get rid of the shortkeys within the QActions and just use them for directly switching tools by clicking the toolbar and move all Tool related shortkey handling into the handleChar method of the ToolManager. Where should be checked if the pressed key is within the secondary shortkey collection of the current tool. If so it should be passed down to the tool. If not it should be checked if the key deactivates the current tool or activates another and act accordingly. If a key that is both secondary key of the current tool and primary of another this could be mentioned in the status bar, so users are aware of this.
Further the tool manager could keep track of all used keys and detect potential conflicts. With the name, description and getters and setters at hand overview and modification GUI could be implemented to resolve those conflicts or just support custom key layouts, which could be stored in the config files.
I already fooled around with some of these ideas and would offer to develop this functionality of you RViz using people out there think that this would be beneficial for all of us. If so, please feel free to provide any feedback or feature ideas that might be useful to consider.