Renaming of display doesn't change panel name #482
Conversation
Renaming the Motion Planning Display doesn't change the name of the Motion Planning Panel right now. These changes overrides the function setName of the display class, since we don't have a associated widget the setName function from display doesn't work.
|
@Jntzko does this allow changing the Display name at runtime? I'm not sure about what this change fixes. |
|
You can change the Display name at runtime with the rename button, this works already. What is missing is the renaming of the Panel. The Panel should get the same name as the Display, for example like the behavior of the Image Display and Panel. |
| @@ -305,6 +305,13 @@ void MotionPlanningDisplay::reset() | |||
| query_robot_goal_->setVisible(query_goal_state_property_->getBool()); | |||
| } | |||
|
|
|||
| void MotionPlanningDisplay::setName(const QString& name) | |||
| { | |||
| BoolProperty::setName(name); | |||
There was a problem hiding this comment.
Why is it necessary to call this static method?
There was a problem hiding this comment.
This static method changes the name of the Display.
There was a problem hiding this comment.
This seems redundant given that you are already setting the window title and the underlying QTObject name in the lines that follow.
There was a problem hiding this comment.
BoolProperty::setName(name) sets the Display name.
setWindowTitle(name) sets the Panel name or title.
I'm not sure what setObjectName(name) does, but the original code from rviz::display says in comments:
"QMainWindow::saveState() needs objectName to be set."
There was a problem hiding this comment.
@Jntzko it looks like the reason why BoolProperty::setName(name) works is because the MotionPlanningPanel inherits from rviz::Display which itself inherits from BoolProperty so this is not a static method but a super class call. The inherited setName() method is already implemented by the rviz::Display class and it already does call the same functions that you have added. Since the MotionPlanningPanel already has inherits this method from rviz::Display then there should be no need to override it.
There was a problem hiding this comment.
The problem is, that the MotionPlanning Panel is not a "associated widget", so the functions in the rviz::Display class will not change the name of the Panel but only the name of the Display.
There was an issue why the MotionPlanning Panel can't be a "associated widget". See here #442 and here #452.
There was a problem hiding this comment.
I see, makes sense to me.
Could you cherry pick to jade and kinetic?
There was a problem hiding this comment.
@jrgnicho can I take that as your approval to merge+cherry-pick this as is?
Then I would directly do so.
There was a problem hiding this comment.
@v4hn I'll merge and do the cherry pick into jade and kinetic then
Renaming of display doesn't change panel name
Renaming of display doesn't change panel name
Cherry pick into kinetic from #482 renameDisplay
Cherry pick #482 from renameDisplay PR
Co-authored-by: Henning Kayser <henningkayser@picknik.ai>
Renaming the Motion Planning Display doesn't change the name of the Motion Planning Panel
right now.
These changes overrides the function setName of the display class, since we don't have a associated widget
the setName function from display doesn't work.