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 GzSpinBox to use the SpinBox QML module from QtQuick.Controls2 #654

Merged
merged 4 commits into from
Feb 6, 2025

Conversation

azeey
Copy link
Contributor

@azeey azeey commented Feb 5, 2025

Toward #586

Summary

The SpinBox module from QtQuick.Controls1 has been deprecated in Qt5 and removed in Qt6. The new SpinBox has a different API and does not directly support floating point values. The workaround in this PR is to store the floating point numbers in a fixed-point representation.

The implementation was inspired by the example in https://doc.qt.io/qt-5/qml-qtquick-controls2-spinbox.html

Checklist

  • Signed all commits for DCO
  • Added tests
  • 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.

The SpinBox module from QtQuick.Controls1 has been deprecated in Qt5 and
removed in Qt6. The new SpinBox has a different API and does not
directly support floating point values. The workaround in this PR is to
store the floating point numbers in a fixed-point representation.

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey azeey requested a review from jennuine as a code owner February 5, 2025 01:09
@github-actions github-actions bot added the 🪵 jetty Gazebo Jetty label Feb 5, 2025
Copy link

codecov bot commented Feb 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.72%. Comparing base (8edf24d) to head (c0aa01e).
Report is 11 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #654      +/-   ##
==========================================
+ Coverage   68.49%   68.72%   +0.23%     
==========================================
  Files          38       38              
  Lines        5567     5544      -23     
==========================================
- Hits         3813     3810       -3     
+ Misses       1754     1734      -20     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@iche033
Copy link
Contributor

iche033 commented Feb 5, 2025

code changes look good to me.

I noticed an issue that when expanded the - button overlaps with the display text:

Screenshot 2025-02-05 at 1 49 39 PM

Also not sure if it's possible to make the - and + buttons a different background color from the display text field (something like the screenshots in https://doc.qt.io/qt-5/qml-qtquick-controls2-spinbox.html). At first glance, the - button makes it look like the spinbox contains a negative number.

azeey added 3 commits February 6, 2025 00:07
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
This leaves more real estate for the numbers and addresses a reviewer
feedback where the down button overlapped with the number.

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey
Copy link
Contributor Author

azeey commented Feb 6, 2025

code changes look good to me.

I noticed an issue that when expanded the - button overlaps with the display text:

Screenshot 2025-02-05 at 1 49 39 PM Also not sure if it's possible to make the `-` and `+` buttons a different background color from the display text field (something like the screenshots in https://doc.qt.io/qt-5/qml-qtquick-controls2-spinbox.html). At first glance, the `-` button makes it look like the spinbox contains a negative number.

I've addressed your feedback in 7330c06 and actually got it to look like the SpinBox from the previous version.

image

8eae434 cleans up some warnings related to bindings.

Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

nice, works well for me

@azeey azeey merged commit 7395bff into gazebosim:main Feb 6, 2025
10 checks passed
@azeey azeey deleted the spinbox_qtcontrol2 branch February 6, 2025 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🪵 jetty Gazebo Jetty
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants