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.

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

Renaming of display doesn't change panel name #482

Merged
merged 1 commit into from Apr 7, 2017

Conversation

Projects
None yet
3 participants
@Jntzko
Copy link

commented Apr 6, 2017

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.

Yannick Jonetzko
Renaming of display doesn't change panel name
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.
@jrgnicho

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2017

@Jntzko does this allow changing the Display name at runtime? I'm not sure about what this change fixes.

@Jntzko

This comment has been minimized.

Copy link
Author

commented Apr 6, 2017

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);

This comment has been minimized.

Copy link
@jrgnicho

jrgnicho Apr 6, 2017

Contributor

Why is it necessary to call this static method?

This comment has been minimized.

Copy link
@Jntzko

Jntzko Apr 6, 2017

Author

This static method changes the name of the Display.

This comment has been minimized.

Copy link
@jrgnicho

jrgnicho Apr 6, 2017

Contributor

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

This comment has been minimized.

Copy link
@Jntzko

Jntzko Apr 6, 2017

Author

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

This comment has been minimized.

Copy link
@jrgnicho

jrgnicho Apr 6, 2017

Contributor

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

This comment has been minimized.

Copy link
@Jntzko

Jntzko Apr 6, 2017

Author

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.

This comment has been minimized.

Copy link
@jrgnicho

jrgnicho Apr 6, 2017

Contributor

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

This comment has been minimized.

Copy link
@v4hn

v4hn Apr 7, 2017

Member

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

This comment has been minimized.

Copy link
@jrgnicho

jrgnicho Apr 7, 2017

Contributor

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

@jrgnicho jrgnicho merged commit 42dfcae into ros-planning:indigo-devel Apr 7, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

jrgnicho added a commit to jrgnicho/moveit that referenced this pull request Apr 7, 2017

Merge pull request ros-planning#482 from Jntzko/renameDisplay
Renaming of display doesn't change panel name

@jrgnicho jrgnicho referenced this pull request Apr 7, 2017

Merged

Cherry pick into kinetic from #482 renameDisplay #483

0 of 4 tasks complete

jrgnicho added a commit to jrgnicho/moveit that referenced this pull request Apr 7, 2017

Merge pull request ros-planning#482 from Jntzko/renameDisplay
Renaming of display doesn't change panel name

@jrgnicho jrgnicho referenced this pull request Apr 7, 2017

Merged

Cherry pick #482 from renameDisplay PR #484

0 of 4 tasks complete

jrgnicho added a commit that referenced this pull request Apr 9, 2017

Merge pull request #483 from jrgnicho/indigo-#482-cherry-pick-to-kinetic
Cherry pick into kinetic from #482 renameDisplay

jrgnicho added a commit that referenced this pull request Apr 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.