Tool: Propagate name changes to VisualizationFrame

[jcmonteiro]

Description

With this PR the tool parameters will be available when onInitialize is called. One can use this feature to solve #1138 by adding
an rviz::StringProperty, e.g. named Tool Name, and then use its value to rename the tool in onInitialize.

[rhaschke]

I'm not sure about the purpose of this PR yet. Isn't it common practice to first initialize an object (display, panel, tool, ...) and later load its parameters? I don't like the idea of coupling the loading to the creation of a tool. Regarding #1138: I guess you can always call setName() on your tool, can't you?

[jcmonteiro]

Isn't it common practice to first initialize an object (display, panel, tool, ...) and later load its parameters?

Hmm.. I'm not quite sure. After all, the initialization might depend on the parameters. What would be the downside of having the parameters available in onInitialize?

Regarding #1138: I guess you can always call setName() on your tool, can't you?

Calling setName has no effect after onInitialize. At least it has no effect when called in activate. Besides, activate takes place only when the tool is clicked, so even if it worked there the name would only change to the correct one once the tool had been clicked.

[rhaschke]

After all, the initialization might depend on the parameters.

It shouldn't. It's always possible to load a different config later. However, the initialization should always work.

Calling setName has no effect after onInitialize.

I don't see why this should be the case. Currently, setName() is called just shortly before initialize():

rviz/src/rviz/tool_manager.cpp

Lines 235 to 237 in 13d061d

	tool->setName(addSpaceToCamelCase(factory_->getClassName(class_id)));
	tool->setIcon(factory_->getIcon(class_id));
	tool->initialize(context_);

[jcmonteiro]

It shouldn't. It's always possible to load a different config later. However, the initialization should always work.

I see... I did some more digging in the code and the issue is the following with setName and setIcon: they have no effect on the menu entries because they are created only once per tool in

rviz/src/rviz/visualization_frame.cpp

Lines 1177 to 1188 in 2fe6d33

	void VisualizationFrame::addTool(Tool* tool)
	{
	QAction* action = new QAction(tool->getName(), toolbar_actions_);
	action->setIcon(tool->getIcon());
	action->setIconText(tool->getName());
	action->setCheckable(true);
	toolbar_->insertAction(add_tool_action_, action);
	action_to_tool_map_[action] = tool;
	tool_to_action_map_[tool] = action;
	remove_tool_menu_->addAction(tool->getName());
	}

From what you explained about the initialization, I believe that the proper solution to #1138 is to change the setName and setIcon methods so that they force a reload of the action and update the member variables action_to_tool_map_, tool_to_action_map_, toolbar_, and remove_tool_menu_ from VisualizationFrame. After doing so, calling setName or setIcon from an overloaded load method should do the trick. What do you think?

[rhaschke]

That sounds reasonable. Would it be sufficient to change the name? I think, having the same icons for the same tool, would facilitate its identification. I suggest introducing a signal nameChanged(), such that the VisFrame is informed about name changes.

[jcmonteiro]

Completely agree. I'll do it then.

[jcmonteiro]

I've force-pushed the changes. Calling setName now updates the toolbar and this PR solves #1138.

[rhaschke]

Thanks for this contribution. In general, this looks good. Some minor remarks though.

[jcmonteiro]

Great suggestions! I've force-pushed again to meet them.

[rhaschke]

Two more nitpicks. Otherwise looks good. Thanks.

[jcmonteiro]

Two more nitpicks. Otherwise looks good. Thanks.

That's completely understandable 👍. I've pushed the changes.

[rhaschke]

Thanks a lot.