-
-
Notifications
You must be signed in to change notification settings - Fork 31
Conversation
That's a great start! I think a lot of small things will need to be updated but at least it has been started, thank you for that! |
|
||
glib_wrapper! { | ||
#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] | ||
pub struct DBusArgInfo(Shared<gio_sys::GDBusArgInfo>); |
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.
This is not useful as it is. Needs some accessors for the struct fields manually implemented, same for DbusPropertyInfo
and DbusMethodInfo
and all those other Info
structs
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 main use of Info
structs is to do gio::DBusNodeInfo::new_for_xml
, .lookup_interface
and pass the DBusInterfaceInfo
to register_object_with_closures
:)
Actual inspection of info would be nice I guess, but not a priority for me.
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.
Doesn't look like much is missing for that though, just the field accessors or not?
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.
(should create an issue about that so it can be added later and is not forgotten if you don't want to add it)
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.
Nothing is missing, you can see I'm using it in the example in the first post here.
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.
No I mean inspection is missing, and the struct accessors for these Info
structs seem to be the only thing missing for that :)
You're right that it can actually also be used otherwise like you did via the XML, I hadn't thought of that.
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.
I'll add these once this one is merged here
Overall looks quite nice, thanks :) It's all rather low-level (like the C API is) but could be wrapped in a nicer higher-level API on top of this for something more convenient. |
src/auto/dbus_interface.rs
Outdated
} | ||
|
||
impl<O: IsA<DBusInterface>> DBusInterfaceExt for O { | ||
fn dup_object(&self) -> Option<DBusObject> { |
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.
dup_object()
should be renamed to get_object()
, and get_object()
be hidden. get_object()
is not safe, see the docs, and otherwise equivalent.
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.
tbh I'm not sure about the "rename" part. Don't like it when bindings are that unfaithful to the original library :D
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 a bindings point of view both functions work exactly the same, and both are "dup"licating (i.e. g_object_ref()
) the return value. Just that the get()
variant is internally unsafe while the dup()
one is not (the get()
one was a API design bug from what I understand).
So we definitely don't want to expose the get()
variant, and as dup()
makes semantically no sense for the bindings renaming it to get()
would seem better and less confusing for users :)
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.
Went through everything in detail now. Lots of little things but generally looks good :)
Proper use of this API will also need gtk-rs/glib#113 to be solved so more than trivial types can be passed via method calls, etc. |
Added newtypes for everything in connection. Absolutely hated this process, editing generated code is evil >_< but whatever, not my project haha. Also the closures are typed now. |
&mut error, | ||
); | ||
if error.is_null() { | ||
Ok(RegistrationId(NonZeroU32::new_unchecked(id))) |
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.
Maybe safer to use `new(id).expect' ?
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.
This is what is used in glib
e.g. for SignalHandlerId
I don't think we really need to test gio's function contracts on every call :)
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.
glib
checked it before use new_unchecked
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.
Yeah either add an assertion before and new_unchecked()
, or new()
and expect()
. glib didn't do the latter because in older Rust you don't get too useful source file / line number with expect()
.
And closures themselves are not optional, because you'd have to provide a type argument anyway.. |
Yeah it sucks but I'm not sure what the alternative would be other than keeping the API suboptimal. The goal is not to keep the amount of work we have to do low :) There are unfortunately various patterns that we can't detect automatically. I'll go through the whole code again later today but I guess this is basically read now :) |
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.
Apart from that, good to be merged from my side.
Done.
Adding more and more features to |
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.
Thanks, looks good to me :) @GuillaumeGomez @EPashkin any more comments from you?
IMHO it looks good, Thanks, @myfreeweb |
@myfreeweb Do you want to provide your example from the screenshot or something similar (like using the notification dbus interface) as an example to https:/gtk-rs/examples/ ? |
Yes, sure. (How are versions handled? I see the examples repo doesn't use git versions of crates..) |
Look at the pending branch. ;) |
That was easy enough, even extracting the method invocation object in the
Closure
just worked :)the "unused import:
glib::translate
" is annoying thoughExample code