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

call propertyHiddenChanged synchronously #1795

Conversation

simonschmeisser
Copy link
Contributor

Property might no longer exist when event is triggered

fixes #1657

Property might no longer exist when event is triggered
@rhaschke
Copy link
Contributor

I'm not sure why a queued connection was used here. But instead of changing the connection type (which might have other side effects), what about changing the argument from Property* to QPointer<Property>?
This way, we would notice any invalidation of the held Property and you could easily check this in PropertyTreeWidget::propertyHiddenChanged.

@simonschmeisser
Copy link
Contributor Author

I'll test QPointer as well, didn't have it in my mind.

@simonschmeisser
Copy link
Contributor Author

simonschmeisser commented May 16, 2023

I didn't observe any difference between the QueuedConnection variant with QPointer and the synchronous version with the raw pointer.

Git blame does not give any reason as to why this is queued, it was added in the context of initially implementing point cloud displays.

@rhaschke
Copy link
Contributor

Thanks for implementing the QPointer variant too. Thanks, also, for checking git history. Indeed, the queue connection was introduced as part of 3bbf90e. Given that, the QPointer approach requires API /ABI changes, I validated that a direct connection doesn't harm showing/hiding properties of PCD. I think I will merge your initial solution then. I'm sorry for the extra work.

@rhaschke
Copy link
Contributor

@simonschmeisser, could you please revert the 2nd commit? Thanks.

@simonschmeisser simonschmeisser force-pushed the property-hidden-synchronous branch from 9b8e790 to f923c15 Compare May 17, 2023 22:44
@rhaschke rhaschke merged commit fb5348f into ros-visualization:noetic-devel May 18, 2023
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.

Crash when quickly toggling robot_description_property in RobotStateDisplay
2 participants