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

Fixes warnings about onFoo connections in Qt QML #493

Closed
wants to merge 1 commit into from

Conversation

mjcarroll
Copy link
Contributor

@mjcarroll mjcarroll commented Sep 28, 2022

🦟 Bug fix

Partially fixes #481
Split from #404

Summary

Cleans up warnings from Qt in the style of:

QML Connections: Implicitly defined onFoo properties in Connections are deprecated. Use this syntax instead: function onFoo(<arguments>) { ... }

Checklist

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: Michael Carroll <michael@openrobotics.org>
@mjcarroll mjcarroll self-assigned this Sep 28, 2022
@github-actions github-actions bot added the 🌱 garden Ignition Garden label Sep 28, 2022
@mjcarroll
Copy link
Contributor Author

I believe I introduced one of these test failures, do not merge.

@@ -101,11 +101,11 @@ ApplicationWindow
// C++ signals to QML slots
Connections {
target: MainWindow
onNotify: {
function onNotify(_message) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, according to this commit message, this syntax is not available for Qt 5.12 which is the version for focal. A suggestion to look into is disable the warning or figure out how to call these functions from C++ code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we aren't planning on keeping focal support for harmonic, I can retarget to main, at least.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think there have been discussions or decisions on this yet

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

From the commit message @jennuine linked, perhaps we can disable the warning using QLoggingCategory::setFilterRules("qt.qml.connections=false");?

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternative PR #534

Copy link
Contributor

Choose a reason for hiding this comment

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

From the commit message @jennuine linked, perhaps we can disable the warning using QLoggingCategory::setFilterRules("qt.qml.connections=false");?

Apologies, I missed this message but thanks for taking care of it 🙇‍♀️

@azeey
Copy link
Contributor

azeey commented Apr 12, 2023

Should we close this for now and reopen when we're on a version of gz-gui that doesn't support Focal?

@mjcarroll
Copy link
Contributor Author

Sounds good to me.

@jennuine
Copy link
Contributor

Fixed in #534

@jennuine jennuine closed this Apr 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Implicitly defined onFoo properties in Connections are deprecated.
4 participants