global-shortcuts: Signal arguments are outputs #917
Conversation
|
I don't think this is correct. Out arguments mean the application is handing out values to the D-Bus methods / signals, which is not the case here. |
Out means it's the service that provides and client that receives. See for example But I believe the "direction" part could be removed completely, and I think it'll default to |
That's correct. If you prefer I can remove it too. |
Personally I have no preference, and both ways exist already in the other XML files. |
|
Okay, then I suggest we merge this. FYI, did this when implementing mumble's support as a client implementation of the portal. |
Alright. |
There was a problem hiding this comment.
The D-Bus Specification says:
The direction element on may be omitted, in which case it defaults to "in" for method calls and "out" for signals. Signals only allow "out" so while direction may be specified, it's pointless.
which implies that changing them to be explicitly out is valid; but if you look at the introspect.dtd provided in the dbus source code, technically the default for validating XML parsers is in... so really it's anyone's guess.
The "direction" of a signal argument is meaningless anyway: signals are a single message from sender to recipient(s), so there is no need to partition arguments into "this goes in the method call" and "this goes in the method reply" like we do for method calls.
In practice I would expect all D-Bus implementations to be able to cope with a signal with no direction specified, and I would expect that explicitly specifying the direction is more likely to break someone.
One place where this might genuinely matter is for QtDBus' type annotations.
If I was designing D-Bus introspection today and I was forced to use XML, I'd probably use something like <method><in><arg/>...</in><out><arg/>...</out></method> vs. <signal><arg/>...</signal>; but we're about 20 years too late for that change, so no. Then again, if you were designing Qt's dialect of D-Bus introspection today, you'd probably put the Qt annotations inside the <arg> (which was a new spec feature in 2016), rather than putting them one level up from where they should be and using argument numbering...
| <arg type="s" name="shortcut_id" direction="out"/> | ||
| <arg type="t" name="timestamp" direction="out"/> | ||
| <arg type="a{sv}" name="options" direction="out"/> | ||
| <annotation name="org.qtproject.QtDBus.QtTypeName.In3" value="QVariantMap"/> |
There was a problem hiding this comment.
Is this right? You've given it 4 out arguments, and I infer from your other contributions of this annotation that they're using 0-based indexing, so I'd expect the annotation to be .Out3 now?
(If I'm reading correctly, this was also an attempt at a silent bug-fix for Qt consumers because the annotation used to apply to a nonexistent 5th argument (In4 in 0-based indexing), and was changed to point to the 4th argument?)
There was a problem hiding this comment.
@aleixpol, can you open a PR changing In3 to Out3?
There was a problem hiding this comment.
Good catch!
I was thinking it could make sense to run qtdbusxmltocpp in the CI to make sure it's all sound. Do you think it would be useful?
It would also need fixing some of the files that at the moment are not entirely working with it just yet.
I'm a bit surprised that the different tooling didn't complain about this, is this something that should be supported?