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

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

Merged

Conversation

rhaschke
Copy link
Contributor

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 rhaschke force-pushed the fix-enabling-displays branch from 7d628ff to 4804a5e Compare April 21, 2022 20:26
@rhaschke
Copy link
Contributor Author

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:

rviz/src/rviz/display.cpp

Lines 370 to 372 in 1f622b8

connect(associated_widget_panel_, SIGNAL(visibilityChanged(bool)), this,
SLOT(associatedPanelVisibilityChange(bool)));
connect(associated_widget_panel_, SIGNAL(closed()), this, SLOT(disable()));

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

@Levi-Armstrong
Copy link

Levi-Armstrong commented Apr 22, 2022

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?

…get_panel_

We need the flag just to suppress hiding of the associated widget if
it became an unselected tab
@rhaschke rhaschke force-pushed the fix-enabling-displays branch from fa180af to 4455043 Compare April 24, 2022 17:21
@rhaschke
Copy link
Contributor Author

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
Copy link
Contributor Author

@Levi-Armstrong, do you think this PR (and #1739 (comment)) works for you?

@Levi-Armstrong
Copy link

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

@rhaschke rhaschke merged commit 0c0d903 into ros-visualization:noetic-devel May 13, 2022
@rhaschke rhaschke deleted the fix-enabling-displays branch May 13, 2022 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Associated widget which is docked creating a tabbed panels gets disabled if tab is not visible
2 participants