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

Fix message display in TopicEcho #322

Merged
merged 1 commit into from
Dec 1, 2021

Conversation

srmainwaring
Copy link
Contributor

@srmainwaring srmainwaring commented Nov 25, 2021

🦟 Bug fix

Summary

This PR fixes a problem with the TopicEcho plugin where the message is not displayed correctly.

This may be apparent on macOS because of the Qt version installed using homebrew (details below).

System

OS: macOS Big Sur 11.6.4

Qt version: 5.15.2

$ brew info qt@5
qt@5: stable 5.15.2 (bottled) [keg-only]
Cross-platform application and UI framework
https://www.qt.io/
/usr/local/Cellar/qt@5/5.15.2_1 (10,688 files, 365.9MB)
  Poured from bottle on 2021-10-22 at 15:10:42
From: https://github.com/Homebrew/homebrew-core/blob/HEAD/Formula/qt@5.rb
License: GFDL-1.3-only and GPL-2.0-only and GPL-3.0-only and LGPL-2.1-only and LGPL-3.0-only
==> Dependencies
Build: pkg-config ✔
==> Requirements
Build: Xcode ✔
Required: macOS >= 10.12 ✔

Before

The messages are not displayed correctly and a warning is printed to the terminal.

$ ign gui -c examples/config/pubsub.config
...
[GUI] [Wrn] [Application.cc:698] [QT] file::/TopicEcho/TopicEcho.qml:105: TypeError: Cannot read property 'width' of null

topicecho_before

After

The messages display as expected and there is no warning.

topicecho_after

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Nov 25, 2021
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should target ign-gui3, @chapulina ?

@chapulina chapulina added the bug Something isn't working label Nov 29, 2021
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix, works for me. Would you be able to retarget the PR to ign-gui3 (Citadel), and then we'll merge forward? Thanks!

@srmainwaring srmainwaring deleted the fix/ign-gui6-topicecho branch November 30, 2021 21:34
@srmainwaring srmainwaring restored the fix/ign-gui6-topicecho branch November 30, 2021 21:36
@srmainwaring srmainwaring reopened this Nov 30, 2021
@srmainwaring srmainwaring changed the base branch from ign-gui6 to ign-gui3 November 30, 2021 21:39
- Change binding of width property in delegate (see: https://stackoverflow.com/questions/63767669/parent-is-null-in-listview-delegate-after-upgrade-to-qt-5-15)
- Use scoped reference to model.display (see: https://forum.qt.io/topic/92085/using-qstringlistmodel-as-model-in-listview)

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
@srmainwaring
Copy link
Contributor Author

Thanks for the fix, works for me. Would you be able to retarget the PR to ign-gui3 (Citadel)

I've changed the base branch to ign-gui3 and rebased the topic branch. Unfortunately it seems you can't change the topic branch in a PR so I'm stuck with the name.

@codecov
Copy link

codecov bot commented Dec 1, 2021

Codecov Report

Merging #322 (734b856) into ign-gui3 (8b3f2b0) will decrease coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           ign-gui3     #322      +/-   ##
============================================
- Coverage     66.80%   66.73%   -0.07%     
============================================
  Files            25       25              
  Lines          2961     2961              
============================================
- Hits           1978     1976       -2     
- Misses          983      985       +2     
Impacted Files Coverage Δ
src/plugins/scene3d/Scene3D.cc 42.11% <0.00%> (-0.28%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b3f2b0...734b856. Read the comment docs.

@chapulina chapulina merged commit aa8632d into gazebosim:ign-gui3 Dec 1, 2021
@srmainwaring srmainwaring deleted the fix/ign-gui6-topicecho branch December 1, 2021 09:14
chapulina added a commit that referenced this pull request Dec 8, 2021
* Added log storing for ign-gui (#272)

Signed-off-by: Nikhil Nair <nikhilnicky972@gmail.com>
Signed-off-by: Louise Poubel <louise@openrobotics.org>

Co-authored-by: Louise Poubel <louise@openrobotics.org>

* Don't crash if a plugin has invalid QML (#315)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

* Set marker point size from message (#317)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

* Don't set visual scale for point markers (#321)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

* Fix TopicEcho plugin message display (#322)

- Change binding of width property in delegate (see: https://stackoverflow.com/questions/63767669/parent-is-null-in-listview-delegate-after-upgrade-to-qt-5-15)
- Use scoped reference to model.display (see: https://forum.qt.io/topic/92085/using-qstringlistmodel-as-model-in-listview)

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* Use qmldir to define QML module with IgnSpinBox (#319)

Signed-off-by: William Wedler <william.wedler@resquared.com>
Co-authored-by: Louise Poubel <louise@openrobotics.org>

* Add PreRender event to MinimalScene (#325)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

* Offer a way to disable warnings on marker manager (#326)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

* Fix codecheck (#329)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

* Fix codecheck (#332)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

* Grid config: set values from startup and improve layout (#324)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

Co-authored-by: Nikhil Nair <43491351+NickNair@users.noreply.github.com>
Co-authored-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Co-authored-by: Will <1305536+zflat@users.noreply.github.com>
Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Co-authored-by: Jenn Nguyen <jenn@openrobotics.org>
@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-01-10/1228/1

@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-01-24-citadel-edifice-fortress/1241/1

@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-03-01-citadel-edifice-fortress/1313/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 🏯 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants