Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Renaming of display doesn't change panel name #482

Merged
merged 1 commit into from Apr 7, 2017
Merged

Renaming of display doesn't change panel name #482

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions ...planning_rviz_plugin/include/moveit/motion_planning_rviz_plugin/motion_planning_display.h
View file
Open in desktop
Expand Up @@ -93,6 +93,8 @@ class MotionPlanningDisplay : public PlanningSceneDisplay
virtual void update(float wall_dt, float ros_dt);
virtual void reset();

void setName(const QString& name);

robot_state::RobotStateConstPtr getQueryStartState() const
{
return query_start_state_->getState();
Expand Down
9 changes: 8 additions & 1 deletion moveit_ros/visualization/motion_planning_rviz_plugin/src/motion_planning_display.cpp
View file
Open in desktop
Expand Up @@ -228,7 +228,7 @@ void MotionPlanningDisplay::onInitialize()

if (window_context)
{
frame_dock_ = window_context->addPane("Motion Planning", frame_);
frame_dock_ = window_context->addPane(getName(), frame_);
connect(frame_dock_, SIGNAL(visibilityChanged(bool)), this, SLOT(motionPanelVisibilityChange(bool)));
frame_dock_->setIcon(getIcon());
}
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it necessary to call this static method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This static method changes the name of the Display.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems redundant given that you are already setting the window title and the underlying QTObject name in the lines that follow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, makes sense to me.
Could you cherry pick to jade and kinetic?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jrgnicho can I take that as your approval to merge+cherry-pick this as is?
Then I would directly do so.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@v4hn I'll merge and do the cherry pick into jade and kinetic then

frame_dock_->setWindowTitle(name);
frame_dock_->setObjectName(name);
}

void MotionPlanningDisplay::backgroundJobUpdate(moveit::tools::BackgroundProcessing::JobEvent, const std::string&)
{
addMainLoopJob(boost::bind(&MotionPlanningDisplay::updateBackgroundJobProgressBar, this));
Expand Down