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

Offer a way to disable warnings on marker manager #326

Merged
merged 3 commits into from
Dec 6, 2021

Conversation

chapulina
Copy link
Contributor

🎉 New feature

Summary

Sometimes it's nice to treat the marker actions as idempotent, so performing the same action multiple times is not an error. But for other use cases, it may be expected that actions like deleting an nonexistent marker are mistakes. So I added a way of disabling the warnings while maintaining the default behaviour.

Test it

Try to delete markers in a nonexistent namespace. By default you'll see a warning, but if you add <warn_on_action_failure>false</warn_on_action_failure> to the plugin, then no warning is printed.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • 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

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

Signed-off-by: Louise Poubel <[email protected]>
@chapulina chapulina added the enhancement New feature or request label Dec 4, 2021
@chapulina chapulina requested a review from jennuine as a code owner December 4, 2021 04:53
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Dec 4, 2021
@codecov
Copy link

codecov bot commented Dec 4, 2021

Codecov Report

Merging #326 (93ca445) into ign-gui6 (679744a) will decrease coverage by 0.01%.
The diff coverage is 10.00%.

❗ Current head 93ca445 differs from pull request most recent head d248fd2. Consider uploading reports for the commit d248fd2 to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##           ign-gui6     #326      +/-   ##
============================================
- Coverage     65.76%   65.74%   -0.02%     
============================================
  Files            34       34              
  Lines          4761     4767       +6     
============================================
+ Hits           3131     3134       +3     
- Misses         1630     1633       +3     
Impacted Files Coverage Δ
src/plugins/marker_manager/MarkerManager.cc 60.00% <10.00%> (-0.87%) ⬇️
src/plugins/scene3d/Scene3D.cc 49.09% <0.00%> (+0.25%) ⬆️

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 679744a...d248fd2. Read the comment docs.

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.

Just a minor style issue

src/plugins/marker_manager/MarkerManager.cc Outdated Show resolved Hide resolved
src/plugins/marker_manager/MarkerManager.cc Outdated Show resolved Hide resolved
Signed-off-by: Louise Poubel <[email protected]>

Co-authored-by: Alejandro Hernández Cordero <[email protected]>
@chapulina chapulina enabled auto-merge (squash) December 6, 2021 17:47
@chapulina chapulina merged commit 7b0ae65 into ign-gui6 Dec 6, 2021
@chapulina chapulina deleted the chapulina/6/marker_warnings branch December 6, 2021 18:09
@chapulina chapulina mentioned this pull request Dec 7, 2021
7 tasks
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request 🏯 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants