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 Qt annotations #1352

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

pterror
Copy link

@pterror pterror commented Apr 28, 2024

Tested both by adding all .xml interface files to a Qt project via qt_add_dbus_interface, and through meson test -C _build.

Two tests are failing, attached for posterity:
testlog.txt
They don't seem to be introduced by this PR though, as I can still reproduce them on the previous commit, even after a clean rebuild.

Note that Lockdown.xml still errors as its property names are kebab-cased instead of snake_cased, which Qt considers as invalid.

  • Add missing Qt type annotations for complex types
  • Reorder Qt type annotations to the line above their corresponding parameters for consistency
  • Fix incorrect argument names

Also does minor formatting changes for consistency:

  • Prefer double quotes over single quotes
  • Prefer type= before name=

@pterror
Copy link
Author

pterror commented Apr 28, 2024

Upon trying to actually compile the Qt project, it seems like the arguments named namespace in Settings.xml cause issues in the C++ that Qt generates.

Probably not relevant here though as it's probably something that should be fixed on Qt's side.

@davidedmundson
Copy link
Contributor

The annotations look correct.

@@ -74,6 +74,7 @@
<method name="OpenURI">
<arg type="s" name="parent_window" direction="in"/>
<arg type="s" name="uri" direction="in"/>
<annotation name="org.qtproject.QtDBus.QtTypeName.In2" value="QVariantMap"/>
<arg type="a{sv}" name="options" direction="in"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

a{sv} isn't needed to be special cased, which makes up a lot of these changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Needs Triage
Development

Successfully merging this pull request may close these issues.

None yet

2 participants