ToolManager: shortkey_to_tool_map_ not updated when removing tools

[fbacim]

I'm part of a DRC team, and we have our own rviz fork. Since we have our own tools that use librviz, we don't use the tools that are initially inserted when you create a new visualization manager. So, we do

manager_ = new rviz::VisualizationManager( render_panel_ );
manager_->getToolManager()->removeAll();

and then we add our own tools. I have just rebased our fork, and found that the following line caused issues:

https://github.com/ros-visualization/rviz/blame/indigo-devel/src/rviz/tool_manager.cpp#L140

It will insert a new uninitialized Tool* into shortkey_to_tool_map_ if it doesn't have that key in the map already, and causes the application to segfault a few lines after that when trying to set it as the current tool.

I'd suggest changing that line to the following:

Tool* tool = NULL;
  // need to check if it exists in the map, otherwise it will create an uninitialized Tool* with the event->key() inside shortkey_to_tool_map_ and it will segfault later on when trying to access it
  if( shortkey_to_tool_map_.find(event->key()) != shortkey_to_tool_map_.end() )
    tool = shortkey_to_tool_map_[ event->key() ];

Now, that by itself doesn't solve the issue that I'm having: shortkey_to_tool_map_ is never updated outside AddTool. After I remove all tools, shortkey_to_tool_map_ will retain all keys and invalid Tool*, causing it to crash.

[wjwwood]

@hdeeken do you have time to take a look at this issue?

@fbacim I'll have to dig into this to understand what needs to be changed beyond your suggestion, but I can't say right now when that will be. I'll try to work it in opportunistically.

[hdeeken]

@wjwwood @fbacim RemoveTool obviously needs to alter the map as well. My bad! I'll get to that asap, hopefully within the weekend, otherwise on monday morning. ;)

[wjwwood]

@hdeeken thanks, sorry I missed that in the review on the first go around.

[wjwwood]

Bump, does anyone have time to look into this?

[hdeeken]

@wjwwood the above patch should correctly manage the shortkey map now. i'm sorry for fixing this so late, i completely lost track of that! :(

[wjwwood]

@hdeeken that's ok, @fbacim do you have any time to try his patch and see if it addresses the issue for you?