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 crash if a plugin has invalid QML #315

Merged
merged 1 commit into from
Nov 19, 2021
Merged

Conversation

chapulina
Copy link
Contributor

🦟 Bug fix

Summary

I introduced a regression on #249, so plugins with invalid QML now crash instead of failing gracefully. This PR fixes that. Also added a test so we don't break it again in the future.

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

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

@chapulina chapulina added the bug Something isn't working label Nov 18, 2021
@chapulina chapulina requested a review from jennuine as a code owner November 18, 2021 22:16
@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label Nov 18, 2021
@codecov
Copy link

codecov bot commented Nov 18, 2021

Codecov Report

Merging #315 (c39abee) into ign-gui3 (5a02f30) will increase coverage by 7.23%.
The diff coverage is 58.04%.

Impacted file tree graph

@@             Coverage Diff              @@
##           ign-gui3     #315      +/-   ##
============================================
+ Coverage     59.57%   66.80%   +7.23%     
============================================
  Files            20       25       +5     
  Lines          2471     2961     +490     
============================================
+ Hits           1472     1978     +506     
+ Misses          999      983      -16     
Impacted Files Coverage Δ
src/plugins/grid_3d/Grid3D.cc 28.57% <0.00%> (ø)
src/plugins/image_display/ImageDisplay.cc 23.27% <ø> (ø)
src/plugins/key_publisher/KeyPublisher.cc 56.52% <ø> (ø)
src/plugins/topic_viewer/TopicViewer.cc 82.63% <ø> (ø)
src/plugins/screenshot/Screenshot.cc 33.33% <33.33%> (ø)
src/plugins/scene3d/Scene3D.cc 42.38% <40.67%> (+29.60%) ⬆️
src/Plugin.cc 61.44% <63.15%> (+1.16%) ⬆️
src/ign.cc 44.68% <70.00%> (+6.84%) ⬆️
src/plugins/teleop/Teleop.cc 84.95% <84.95%> (ø)
src/Application.cc 84.69% <90.00%> (-0.01%) ⬇️
... and 11 more

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 970914a...c39abee. Read the comment docs.

@chapulina chapulina merged commit 8b3f2b0 into ign-gui3 Nov 19, 2021
@chapulina chapulina deleted the chapulina/3/bad_qml branch November 19, 2021 06:24
@chapulina chapulina mentioned this pull request Dec 8, 2021
chapulina added a commit that referenced this pull request Dec 8, 2021
* Added log storing for ign-gui (#272)

Signed-off-by: Nikhil Nair <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>

Co-authored-by: Louise Poubel <[email protected]>

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

Signed-off-by: Louise Poubel <[email protected]>

* Set marker point size from message (#317)

Signed-off-by: Louise Poubel <[email protected]>

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

Signed-off-by: Louise Poubel <[email protected]>

* 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 <[email protected]>

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

Signed-off-by: William Wedler <[email protected]>
Co-authored-by: Louise Poubel <[email protected]>

* Add PreRender event to MinimalScene (#325)

Signed-off-by: Louise Poubel <[email protected]>

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

Signed-off-by: Louise Poubel <[email protected]>

Co-authored-by: Alejandro Hernández Cordero <[email protected]>

* Fix codecheck (#329)

Signed-off-by: Louise Poubel <[email protected]>

* Fix codecheck (#332)

Signed-off-by: Louise Poubel <[email protected]>

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

Signed-off-by: Louise Poubel <[email protected]>

Co-authored-by: Nikhil Nair <[email protected]>
Co-authored-by: Rhys Mainwaring <[email protected]>
Co-authored-by: Will <[email protected]>
Co-authored-by: Alejandro Hernández Cordero <[email protected]>
Co-authored-by: Jenn Nguyen <[email protected]>
@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 🏰 citadel Ignition Citadel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants