-
Notifications
You must be signed in to change notification settings - Fork 5
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
Correctly map ImageManager list model index to topic vector #46
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Kettenhoax for finding the bug and proposing the fix! I have added a test that confirms the behaviour change. Please tell me if it looks good to you, and we can get it merged and backported!
@@ -154,7 +154,6 @@ std::optional<ImageTopic> ImageManager::getImageTopic(unsigned index) | |||
void ImageManager::addImageTopicExplicitly(ImageTopic imageTopic) | |||
{ | |||
beginResetModel(); | |||
imageTopics.clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has this change intentionally been added to this PR? It seems like it fixes a different issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, this was intentional. With the test I added, I couldn't call addImageTopicExplicitly
consecutively because the second call clears the topic added by the first call.
As I didn't see a point in having it there, (and so it actually "adds" the image topic, rather than clearing-then-adding).
Signed-off-by: Kenji Brameld <[email protected]>
Going to merge once CI passes. |
Goingto merge, as build failure seems related to ros-sports/ros_image_to_qimage#23 |
@Mergifyio backport galactic humble |
✅ Backports have been created
|
Correctly map ImageManager list model index to topic vector (backport #46)
Correctly map ImageManager list model index to topic vector (backport #46)
Most functions of ImageManager interpret index 0 as unselected and map indices > 0 to the actual topics.
This PR does the same in getImageTopic.
Fixes #45