-
-
Notifications
You must be signed in to change notification settings - Fork 194
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
base: main
Are you sure you want to change the base?
Add Qt annotations #1352
Conversation
Upon trying to actually compile the Qt project, it seems like the arguments named Probably not relevant here though as it's probably something that should be fixed on Qt's side. |
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"/> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From matrix, the QVariantMap
annotations seem unnecessary, so I'd rather like to see a commit which removes them everywhere and then a commit adding the not-QVariantMap
annotations.
@@ -43,7 +43,7 @@ | |||
|
|||
This document describes version 2 of the permission store interface. | |||
--> | |||
<interface name='org.freedesktop.impl.portal.PermissionStore'> |
There was a problem hiding this comment.
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?
fc4dfa7
to
d95eda7
Compare
d95eda7
to
ad1edc8
Compare
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
|
We upgraded the CI. Try rebasing to get it green. |
Tested both by adding all
.xml
interface files to a Qt project viaqt_add_dbus_interface
, and throughmeson 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 arekebab-cased
instead ofsnake_cased
, which Qt considers as invalid.Also does minor formatting changes for consistency:
type=
beforename=
beforedirection=
direction="in"
explicit?