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

Update new / removed entity GUI events #1213

Merged
merged 5 commits into from
Nov 17, 2021
Merged

Conversation

chapulina
Copy link
Contributor

🦟 Bug fix

Summary

Attempt of making the API a bit clearer after #1053 and #1138

  • Made the distinction between GUI and server creation / removal
  • PIMPlized events in case we want to extend them later
  • Made both events consistent, including both new and removed entities
  • Updated names from "added" to "new" to be consistent with other APIs
  • Added unit tests

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

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

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

@adlarkin adlarkin left a comment

Choose a reason for hiding this comment

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

Changes LGTM, thanks for cleaning this up a bit! I just left a few small comments about test coverage.

src/gui/GuiEvents_TEST.cc Show resolved Hide resolved
src/gui/GuiEvents_TEST.cc Show resolved Hide resolved
src/gui/GuiEvents_TEST.cc Show resolved Hide resolved
src/gui/GuiEvents_TEST.cc Show resolved Hide resolved
Signed-off-by: Louise Poubel <[email protected]>
@codecov
Copy link

codecov bot commented Nov 16, 2021

Codecov Report

Merging #1213 (d70982e) into ign-gazebo6 (97f2e9c) will increase coverage by 0.04%.
The diff coverage is 66.66%.

Impacted file tree graph

@@               Coverage Diff               @@
##           ign-gazebo6    #1213      +/-   ##
===============================================
+ Coverage        61.88%   61.92%   +0.04%     
===============================================
  Files              274      275       +1     
  Lines            22441    22457      +16     
===============================================
+ Hits             13887    13907      +20     
+ Misses            8554     8550       -4     
Impacted Files Coverage Δ
include/ignition/gazebo/gui/GuiEvents.hh 0.00% <ø> (ø)
src/gui/plugins/entity_tree/EntityTree.cc 8.06% <ø> (ø)
src/gui/plugins/scene_manager/GzSceneManager.cc 33.33% <0.00%> (-4.91%) ⬇️
src/gui/plugins/select_entities/SelectEntities.cc 10.09% <0.00%> (ø)
src/gui/GuiEvents.cc 100.00% <100.00%> (ø)

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 97f2e9c...d70982e. Read the comment docs.

Signed-off-by: Louise Poubel <[email protected]>
include/ignition/gazebo/gui/GuiEvents.hh Outdated Show resolved Hide resolved
include/ignition/gazebo/gui/GuiEvents.hh Outdated Show resolved Hide resolved
Signed-off-by: Louise Poubel <[email protected]>
@chapulina chapulina merged commit ee90892 into ign-gazebo6 Nov 17, 2021
@chapulina chapulina deleted the chapulina/gui_events branch November 17, 2021 03:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏯 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants