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

Add common widget for pose #424

Merged
merged 11 commits into from
Jul 1, 2022
Merged

Conversation

AzulRadio
Copy link
Contributor

@AzulRadio AzulRadio commented Jun 21, 2022

New feature

Summary

Add the common widget for Pose

Test it

Can be tested with branch azulradio/common_widget_pose in repo gz-sim

demo_common_pose

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.

@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label Jun 21, 2022
@chapulina chapulina added the OOBE 📦✨ Out-of-box experience label Jun 21, 2022
@codecov
Copy link

codecov bot commented Jun 21, 2022

Codecov Report

Merging #424 (ee26c69) into ign-gui3 (27afc95) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           ign-gui3     #424   +/-   ##
=========================================
  Coverage     66.12%   66.12%           
=========================================
  Files            29       29           
  Lines          3218     3218           
=========================================
  Hits           2128     2128           
  Misses         1090     1090           

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 27afc95...ee26c69. Read the comment docs.

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.

Looks good, left some comments.

There's a couple of bugs I've encountered (not sure if they are correlated). When you first expand the pose submenu in the component inspector, you're able to update the pose fine and when the pose submenu stays open when you click on another object. If you keep selecting other objects, eventually the pose submenu closes and when you re-open it you're no longer able to update the pose. See gif below. Also, notice the sphere will eventually move when selected from the entity tree.

Ignore previous statement, it wasn't reproducible by others

include/ignition/gui/qml/GzPose.qml Outdated Show resolved Hide resolved
include/ignition/gui/qml/GzPose.qml Outdated Show resolved Hide resolved
include/ignition/gui/qml/GzPose.qml Outdated Show resolved Hide resolved
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.

Looking good! Just left some reminders regarding what we discussed in our video call

include/ignition/gui/qml/GzPose.qml Outdated Show resolved Hide resolved
include/ignition/gui/qml/GzPose.qml Outdated Show resolved Hide resolved
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.

Left some minor comments but looks good, thanks for iterating!

Comment on lines +90 to +93
// Dummy component to use its functions.
IgnHelpers {
id: gzHelper
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to set up IgnHelpers.qml in a way that we don't have to instantiate a dummy component and just be able to call the functions (e.g., IgnHelpers.getDecimals(...))?

This is out of scope for this PR but if it is possible, after we merge this, this could potentially be a next task for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well. This is doable but we need to make sure every qml using this common widget must also include IgnHelpers.qml as well. Do we want to do this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed that this would be a nice change. This PR is just following the weird pattern that I started with IgnHelpers a while back, so we don't need to block on it.

we need to make sure every qml using this common widget must also include IgnHelpers.qml as well.

I think there may be ways to work around this, either with a singleton or a javascript file.


/**
* Useful only when readOnly == false. User should read spinbox values from
* its parameters.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you specify "its parameters"? Do you mean, from the property var *Item variables?

When you say, "User should read spinbox...", is that when readOnly is true? If this is the case, it would be good to explicitly state that eg., "When true, users should read spinbox..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"its parameters" refers to the parameters of this signal.

I think you mean when readOnly is false. Here, I also want to stress that when readOnly is "true", this signal is not used (so should not be used by users, too). Would you like to check if my latest commit is clear on these?

include/ignition/gui/qml/GzPose.qml Outdated Show resolved Hide resolved
include/ignition/gui/qml/GzPose.qml Outdated Show resolved Hide resolved
@AzulRadio AzulRadio merged commit cc557e9 into ign-gui3 Jul 1, 2022
@AzulRadio AzulRadio deleted the azulradio/common_widget_pose branch July 1, 2022 23:02
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.

Oh I was just leaving a review and saw that the PR was merged. @AzulRadio , do you mind opening a new PR addressing the comments that you agree with? Thanks!

include/ignition/gui/qml/GzPose.qml Show resolved Hide resolved
include/ignition/gui/qml/GzPose.qml Show resolved Hide resolved
include/ignition/gui/qml/GzPose.qml Show resolved Hide resolved
include/ignition/gui/qml/GzPose.qml Show resolved Hide resolved
include/ignition/gui/qml/GzPose.qml Show resolved Hide resolved
include/ignition/gui/qml/GzPose.qml Show resolved Hide resolved
@AzulRadio AzulRadio mentioned this pull request Jul 1, 2022
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel OOBE 📦✨ Out-of-box experience
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants