search

ros-visualization / rviz

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

call propertyHiddenChanged synchronously #1795

Closed simonschmeisser closed 1 year ago

simonschmeisser commented 1 year ago

Property might no longer exist when event is triggered

fixes #1657

rhaschke commented 1 year ago

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 commented 1 year ago

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

simonschmeisser commented 1 year ago

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 commented 1 year ago

Thanks for implementing the QPointer variant too. Thanks, also, for checking git history. Indeed, the queue connection was introduced as part of 3bbf90e1079f1976280a4f4720b2b9b174ad5583. 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 commented 1 year ago

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