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.

Sign up for GitHub

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

Jump to bottom

use qmldir to define QML module with IgnSpinBox #319

Merged
merged 5 commits into from
Dec 2, 2021
Merged

use qmldir to define QML module with IgnSpinBox #319

merged 5 commits into from
Dec 2, 2021

Conversation

zflat
Copy link
Contributor

@zflat zflat commented Nov 24, 2021 •
edited
Loading

🎉 New feature

Closes #310

Summary

This change adds a qmldir file that defines an importable QML module for common QML components implemented in include/ignition/gui/qml/.

With this change, it will be possible to import QML components using module import statements instead of file path.

Updating import statements will look like this:

-import "qrc:/qml"
+import ignition.gui 1.0

Test it

  • Build and run the application ign run
  • Add the "Teleop" plugin
  • Verify that the plugin loads and that the IgnSpinBox is part of the UI

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

@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Nov 24, 2021
chapulina
chapulina reviewed Nov 24, 2021
View reviewed changes
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.

Nice, thank you for cleaning it up!

The approach looks good to me. I also checked that import "qrc:/qml" still works, so this doesn't break any existing users.

Would you be able to retarget this PR to ign-gui3? Then we can merge it forward to all supported versions from there.

Thanks!

include/ignition/gui/Application.hh Outdated Show resolved Hide resolved
include/ignition/gui/resources.qrc Show resolved Hide resolved
William Wedler added 2 commits November 24, 2021 16:04
use qmldir to define QML module with IgnSpinBox
36c8a1b 
Signed-off-by: William Wedler <william.wedler@resquared.com>
exlain where to add more QML components to use in the qmldir file
2669835 
Signed-off-by: William Wedler <william.wedler@resquared.com>
@zflat zflat changed the base branch from ign-gui6 to ign-gui3 November 24, 2021 21:16
@zflat
Copy link
Contributor Author

zflat commented Nov 24, 2021

Nice, thank you for cleaning it up!

The approach looks good to me. I also checked that import "qrc:/qml" still works, so this doesn't break any existing users.

Would you be able to retarget this PR to ign-gui3? Then we can merge it forward to all supported versions from there.

Thanks!

I branched off of ign-gui3 and force-pushed then updated this PR to target ign-gui3.

@chapulina chapulina added 🏰 citadel Ignition Citadel and removed 🏯 fortress Ignition Fortress labels Nov 24, 2021
@zflat zflat force-pushed the qml_modules branch 3 times, most recently from 63f8ac0 to 908ae31 Compare November 29, 2021 14:00
change qmlQrcImportPath to a helper method
908ae31 
Signed-off-by: William Wedler <william.wedler@resquared.com>
@zflat
Copy link
Contributor Author

zflat commented Nov 29, 2021

@chapulina Are there any more QML components that should be added to the ignition.gui 1.0 QML module in this pull request? Anything that has an Ign prefix?

  • IgnCard
  • IgnCardSettings
  • ...
  • IgnSplit

Or should additional components be added in a separate pull request?

@chapulina
Copy link
Contributor

Are there any more QML components that should be added to the ignition.gui 1.0 QML module in this pull request?

I was thinking about that. The only one that we really use downstream is IgnSpinBox, so I think that's the only one we really need to add right now. If users need to use the others, I think we can add them later.

There's been conversations about adding generic widgets for pose, vector, etc, which we repeat a lot in ign-gui and ign-gazebo. Once those are added I'd expect to put them in this module too.

chapulina
chapulina approved these changes Nov 29, 2021
View reviewed changes
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.

LGTM, are you planning on making more changes?

include/ignition/gui/Helpers.hh Outdated Show resolved Hide resolved
fix documentation formatting
09428d0 
Signed-off-by: William Wedler <william.wedler@resquared.com>
@zflat zflat marked this pull request as ready for review November 29, 2021 19:33
@zflat zflat requested a review from jennuine as a code owner November 29, 2021 19:33
@chapulina chapulina enabled auto-merge (squash) December 2, 2021 06:49
@chapulina chapulina merged commit 1827899 into gazebosim:ign-gui3 Dec 2, 2021
@zflat zflat deleted the qml_modules branch December 3, 2021 21:30
@chapulina chapulina mentioned this pull request Dec 6, 2021
Merged
7 tasks
chapulina added a commit that referenced this pull request Dec 8, 2021
@chapulina @NickNair
@srmainwaring @zflat @ahcorde @jennuine
6 ➡️ 7 (main) (#333)
71e3f6c 
* Added log storing for ign-gui (#272)

Signed-off-by: Nikhil Nair <nikhilnicky972@gmail.com>
Signed-off-by: Louise Poubel <louise@openrobotics.org>

Co-authored-by: Louise Poubel <louise@openrobotics.org>

* Don't crash if a plugin has invalid QML (#315)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

* Set marker point size from message (#317)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

* Don't set visual scale for point markers (#321)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

* 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 <rhys.mainwaring@me.com>

* Use qmldir to define QML module with IgnSpinBox (#319)

Signed-off-by: William Wedler <william.wedler@resquared.com>
Co-authored-by: Louise Poubel <louise@openrobotics.org>

* Add PreRender event to MinimalScene (#325)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

* Offer a way to disable warnings on marker manager (#326)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

* Fix codecheck (#329)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

* Fix codecheck (#332)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

* Grid config: set values from startup and improve layout (#324)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

Co-authored-by: Nikhil Nair <43491351+NickNair@users.noreply.github.com>
Co-authored-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Co-authored-by: Will <1305536+zflat@users.noreply.github.com>
Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Co-authored-by: Jenn Nguyen <jenn@openrobotics.org>
@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-01-10/1228/1

@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-01-24-citadel-edifice-fortress/1241/1

@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-03-01-citadel-edifice-fortress/1313/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jennuine jennuine jennuine approved these changes

@chapulina chapulina chapulina approved these changes

Assignees
No one assigned
Labels
🏰 citadel Ignition Citadel
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Provide reusable QML components with common widgets
4 participants
@zflat @chapulina @osrf-triage @jennuine