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

Added Reset button to world_control #476

Merged
merged 8 commits into from
Nov 15, 2022
Merged

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Aug 24, 2022

Signed-off-by: ahcorde ahcorde@gmail.com

🎉 New feature

Summary

Added a reset button to the world Control plugin
reset_button

Test

gz sim rolling_shapes.sdf

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • 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 and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Signed-off-by: ahcorde <ahcorde@gmail.com>
@github-actions github-actions bot added the 🌱 garden Ignition Garden label Aug 24, 2022
@mjcarroll
Copy link
Contributor

A super pedantic nit: Can we get a reset icon? :)

Copy link
Contributor

@jennuine jennuine left a comment

Choose a reason for hiding this comment

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

Can we get a reset icon?

+1, the square is misleading. At a glace, it looks like a stop button.

Also, since we are feature frozen, this will need to come in a minor release

src/plugins/world_control/WorldControl.cc Outdated Show resolved Hide resolved
RoundButton {
id: stopButton
objectName: "stopButton"
visible: showReset
Copy link
Contributor

Choose a reason for hiding this comment

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

Should reset only be visible if the sim is paused?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you might want to reset when the simulation is running

/**
* Reset
*/
RoundButton {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a hovered tooltip that states what reset does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ahcorde
Copy link
Contributor Author

ahcorde commented Aug 25, 2022

A super pedantic nit: Can we get a reset icon? :)

@mjcarroll I was thinking about adding an icon but I was trying to follow the same style which is an unicode geometric shape https://en.wikipedia.org/wiki/List_of_Unicode_characters. I'm open to suggestion.

Anyhow the way that I implemented the reset is more like a stop (reset + pause)

@jennuine
Copy link
Contributor

How about using the same icon as refresh in ImageDisplay?

@ahcorde
Copy link
Contributor Author

ahcorde commented Aug 25, 2022

How about using the same icon as refresh in ImageDisplay?

nice! thank you @jennuine

@jennuine
Copy link
Contributor

Users may accidentally click this icon so another nice feature would be to have a confirmation dialog before resetting similar to #225 (with the ability to disable the dialog through the config)

@chapulina chapulina added the enhancement New feature or request label Aug 26, 2022
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.

It would be nice to also add the Ctrl+R shortcut, and then we can close

@scpeters
Copy link
Member

@osrf-jenkins run tests please

Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Copy link
Contributor Author

ahcorde commented Aug 30, 2022

It would be nice to also add the Ctrl+R shortcut, and then we can close

dbd8c74

Signed-off-by: ahcorde <ahcorde@gmail.com>
@codecov
Copy link

codecov bot commented Aug 30, 2022

Codecov Report

Merging #476 (ec25c37) into gz-gui7 (17bd485) will decrease coverage by 0.10%.
The diff coverage is 20.00%.

❗ Current head ec25c37 differs from pull request most recent head c204bbf. Consider uploading reports for the commit c204bbf to get more accurate results

@@             Coverage Diff             @@
##           gz-gui7     #476      +/-   ##
===========================================
- Coverage    69.13%   69.03%   -0.11%     
===========================================
  Files           44       44              
  Lines         4821     4831      +10     
===========================================
+ Hits          3333     3335       +2     
- Misses        1488     1496       +8     
Impacted Files Coverage Δ
src/plugins/world_control/WorldControl.cc 72.60% <0.00%> (-4.21%) ⬇️
...plugins/world_control/WorldControlEventListener.hh 100.00% <ø> (ø)
...plugins/world_control/WorldControlEventListener.cc 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ahcorde ahcorde requested a review from jennuine October 10, 2022 11:27
@ahcorde ahcorde self-assigned this Oct 10, 2022
@ahcorde
Copy link
Contributor Author

ahcorde commented Nov 8, 2022

friendly ping @jennuine @mjcarroll

Copy link
Contributor

@jennuine jennuine left a comment

Choose a reason for hiding this comment

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

nit: can we leave the play/step buttons how they were and add the reset button to the right of the step button? And have the reset button be the same size as step?

@ahcorde
Copy link
Contributor Author

ahcorde commented Nov 11, 2022

@jennuine

what do you think about this ?

Selection_094

Signed-off-by: ahcorde <ahcorde@gmail.com>
@jennuine
Copy link
Contributor

jennuine commented Nov 11, 2022

@jennuine

what do you think about this ?

Selection_094

Looks great, thanks! How do you and @mjcarroll feel about the reset button being to the right of the step button? Reasons being:

  1. Users are less likely to accidentally hit reset when attempting to pause (I know this is less likely but I can still see it happening)
  2. It keeps the old/previous layout the same while adding the new feature

@ahcorde
Copy link
Contributor Author

ahcorde commented Nov 11, 2022

@jennuine
what do you think about this ?
Selection_094

Looks great, thanks! How do you and @mjcarroll feel about the reset button being to the right of the step button? Reasons being:

  1. Users are less likely to accidentally hit reset when attempting to pause (I know this is less likely but I can still see it happening)
  2. It keeps the old/previous layout the same while adding the new feature

Anyhow when you click in the reset button a Dialog will appers to ensure that you want to reset the simulation. Not dangerous at all.

@ahcorde
Copy link
Contributor Author

ahcorde commented Nov 14, 2022

@jennuine
what do you think about this ?
Selection_094

Looks great, thanks! How do you and @mjcarroll feel about the reset button being to the right of the step button? Reasons being:

  1. Users are less likely to accidentally hit reset when attempting to pause (I know this is less likely but I can still see it happening)
  2. It keeps the old/previous layout the same while adding the new feature

Anyhow when you click in the reset button a Dialog will appers to ensure that you want to reset the simulation. Not dangerous at all.

changing layout doesn't seem to be working, the step botton uses a mouseclick area, it's tricky to change it

@mjcarroll
Copy link
Contributor

the step botton uses a mouseclick area, it's tricky to change it

If it is too difficult to fix, I think that the dialog confirmation is sufficiently safe.

@ahcorde
Copy link
Contributor Author

ahcorde commented Nov 14, 2022

@osrf-jenkins run tests please!

@ahcorde ahcorde enabled auto-merge (squash) November 14, 2022 18:30
@ahcorde
Copy link
Contributor Author

ahcorde commented Nov 14, 2022

@osrf-jenkins run tests please!

1 similar comment
@ahcorde
Copy link
Contributor Author

ahcorde commented Nov 15, 2022

@osrf-jenkins run tests please!

@mjcarroll mjcarroll merged commit 780cb23 into gz-gui7 Nov 15, 2022
@mjcarroll mjcarroll deleted the ahcorde/world_control/reset branch November 15, 2022 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request 🌱 garden Ignition Garden
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants