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 2 commits into
base: main
Choose a base branch
from
Open

Add Qt annotations #1352

wants to merge 2 commits into from

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= before direction=
  • Make implicit direction="in" explicit?

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

We already have a whole bunch of those annotations where a{sv} is annotated with a QVariantMap. Could we remove all of them?

Copy link
Contributor

Choose a reason for hiding this comment

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

@pterror do you know why the annotations on a{sv} exist and why @davidedmundson claims they are unnecessary?

@@ -43,7 +43,7 @@

This document describes version 2 of the permission store interface.
-->
<interface name='org.freedesktop.impl.portal.PermissionStore'>
Copy link
Contributor

Choose a reason for hiding this comment

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

The changes from ' to " for consistency are a great cleanup. Can you split them into their own commit?

@pterror
Copy link
Author

pterror commented Oct 5, 2024

test results:

Summary of Failures:

 2/30 xdg-desktop-portal / test-doc-portal                    ERROR            0.21s   killed by signal 6 SIGABRT
 9/30 xdg-desktop-portal:portals / test-portals-location      ERROR            0.27s   killed by signal 6 SIGABRT
12/30 xdg-desktop-portal:portals / test-portals-openuri       ERROR            0.26s   killed by signal 6 SIGABRT
10/30 xdg-desktop-portal:portals / test-portals-notification  ERROR            0.67s   killed by signal 6 SIGABRT

Ok:                 26  
Expected Fail:      0   
Fail:               4   
Unexpected Pass:    0   
Skipped:            0   
Timeout:            0  

test-doc-portal and test-portals-location were already failing since last time.
i can confirm the two new failures are still present in 840fd1d0e14b5d2e7292bdfc1cbc94e8e0a8b0ef so it should not be a regression introduced by this PR

@swick
Copy link
Contributor

swick commented Oct 9, 2024

We upgraded the CI. Try rebasing to get it green.

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.

3 participants