search

ros-visualization / rviz

ROS 3D Robot Visualizer
BSD 3-Clause "New" or "Revised" License
792 stars 459 forks source link

Don't disable display if associated widget tab changes #1739

Closed rhaschke closed 2 years ago

rhaschke commented 2 years ago

Fixes #1738

If an associated widget is tabbed and deselected, its visibility changes to false, which triggers disabling the display. However, in this case, the widget is still there and thus the display shouldn't be disabled.

rhaschke commented 2 years ago

This PR requires more work: For a camera display, it makes sense to disable the display if the widget becomes invisible, because its tab is deselected. Instead, to solve your issue #1738, @Levi-Armstrong, you could simply disconnect the visibilityChanged signal for your display as the closed signal will still disable the display if the panel is closed: https://github.com/ros-visualization/rviz/blob/1f622b8c95b8e188841b5505db2f97394d3e9c6c/src/rviz/display.cpp#L370-L372

However, by doing so, the widget doesn't become visible anymore if re-enabled.

Levi-Armstrong commented 2 years ago

Since it seems like there are two valid use cases; would it make sense to add a second parameter to the method for adding the associated widget to indicate which functionality is desired?

rhaschke commented 2 years ago

Would it make sense to add a second parameter to the method for adding the associated widget to indicate which functionality is desired?

I agree that an extra boolean flag would be most clear to the users. But I'm hesitating to change public API. Do you see a big issue in disconnecting the mentioned signal, i.e.: disconnect(panel, SIGNAL(visibilityChanged(bool)), display, SLOT(associatedPanelVisibilityChange(bool))); after calling setAssociatedWidget()?

rhaschke commented 2 years ago

@Levi-Armstrong, do you think this PR (and https://github.com/ros-visualization/rviz/pull/1739#issuecomment-1107884401) works for you?

Levi-Armstrong commented 2 years ago

I think so. I will pull this down and confirm.