Hi, Finally, maybe it's useful to go more concrete looking at eggdbus and mentioning some of the specific stuff in there. I know it's a lot of comments but I'm just giving my opinion, take it for whatever it's worth.
General stuff === * Maybe this should have some use of gobject-introspection. In particular I think it might be nice if I have any introspectable object, to be able to just stick it on the bus, and have incoming calls mapped to it. Then I can just write a normal gobject, stick it on the bus, and have it work, much of the time. In fact for *implementing* objects I'm not sure there's much point in generating anything statically beyond the g-i typelib ... a couple gobject-introspection annotations could specify that some char* are really object paths, or whatever. * the code generator can probably generate smaller code; it's worth factoring out pretty much everything except what actually varies per-method into a generic function, to get the size on this generated stuff down * the library is missing what I called pieces 4 and 5 for non-C bindings, because it tangles that functionality into object proxies and thus the particular GType object mapping. * of the files here, only parts of eggdbusconnection and eggdbusbusnametracker are useful for other bindings. *everything* else is the C object mapping, or pretty close to everything else. Of course, bindings also need the dbus-glib main loop glue which is being used here. * despite all the comments the object mapping looks nice and that's most of the code. Most of my comments are about the "generic" plumbing parts. eggdbusconnection === Like: * wrapping DBusConnection in GObject makes memory management less annoying for bindings, more gobject-introspection friendly * exports unique name and bus type properties * maps pending call to GAsyncResult Suggestions: * object proxy and interface registration is part of object mapping; should really be split out * egg_dbus_connection_get_bus() is blocking rather than async; imo apps should never get the bus anyway, they should always instead either ask to acquire a name or ask to watch a name, and the bus connection is dealt with behind scenes * because apps should not touch connection much if ever, that's all the more reason to make it an API for use by bindings, and thus not have object-mapping-specific stuff in it and not bother to try to hide libdbus * egg_dbus_connection_send_message_with_reply_sync(): imo it's worth strongly considering flat-out disallowing synchronous calls. Yes async is painful in C. No you should not block in a GUI app or a daemon, which means almost all apps. Remove the temptation! * EGG_DBUS_CALL_FLAGS_BLOCK_IN_MAINLOOP nooooooooooooooooooo! Seriously, you have no idea how much time was wasted chasing heisenbugs from people doing this with bonobo. You simply cannot write a correct program of any size that uses this. The warning in the docs is "be careful", but the problem is, it's almost impossible to be careful enough to actually handle this in any real app. It should say "this will crash your app." If people are going to be lazy and block, they need to block, not use some mainloop hack that kinda-blocks. * egg_dbus_connection_register_interface() seems to register an object with interfaces, not an interface, seems like it should be called register_object eggdbusnametracker === Like: * essential functionality, "piece 3" * clean gobject interface beats hippo-dbus-helper type of hackiness Suggestions: * egg_dbus_bus_name_tracker_get_owner_for_bus_name blocks in recursive main loop; nooooooooooooooo again. * aside from re-entrancy hell, any app using this egg_dbus_bus_name_tracker_get_owner_for_bus_name will be broken because it probably won't handle the name reappearing when the other side restarts. For a _correct_ app this API is not convenient. It's only convenient if your app is buggy and gets wedged when the other end restarts. * API should be async by default; consider C convenience version where you hide the tracker object, and developer can provide two callbacks statically instead of having to alloc/get object, get current state, connect to two signals, worry about freeing object. * egg_dbus_bus_name_tracker_get_known_well_known_bus_names_for_unique_bus_name (longest function name ever?) this should be an internal API probably, if it exists, but I don't think it should. I would recode the use of this in eggdbusconnection such that at any given time a proxy is bound only to a unique bus name. Proxies could all watch the well-known name they are bound to, and just update the unique bus name they use when it changes. Then just route signals to proxies currently bound to a given unique bus name. This will be more efficient and less work. * because it's 1 global tracker instead of a separate tracker per watch of a name, to watch a name someone has to first get the current state, then set up the two signal connections. It's more convenient if you can just say "I care about this name; here are my two callbacks. Guarantee you will call 1 of them next time I hit main loop." If the name already has a known owner, then just queue an idle and invoke the "name gained owner" callback in there. * might work out better to make this entire "track all names" object private, and make the public API be lightweight objects allocated each time someone wants to watch a specific name * an advantage of that approach is that signal handlers don't need a strcmp(bus_name_owner, "NameICareAbout") in them, much more efficient if name watchers only get a callback for the name they want to watch eggdbuserror, eggdbushashmap, eggdbusmessage, eggdbusstructure, eggdbusvariant === This is the type-mapping stuff that's only useful for the particular C object mapping. This is one of the big areas I think is pretty much bloat for other language bindings. Aside from just amount of code, eggdbusmessage imposes an extra copy on all data; JavaScript can't use a g_malloc'd string or a GHashTable or anything, so they are created and live for a millisecond then nuked, if you were to bind JavaScript or Python or something to this. It's more appropriate IMO to go straight from the DBusMessage into the native language type. eggdbusobject/ifaceproxy === Like: * maps props to GObject properties * keeps proxy objects separate from implementation objects Suggestions: * would love a thing like dbus_g_proxy_call that lets you skip the introspection and static binding stuff * add a got-properties signal perhaps so it's at least more possible to deal with properties asynchronously; consider an API that actively encourages it, e.g. an async 'get proxy' call that returns the proxy with properties loaded * I don't think proxies bound to a well-known name are very good. They assume all methods are completely independent and remote objects are stateless, or else they have races. I don't think many methods and objects are independent and stateless. Pretty much *nothing* in the NetworkManager dbus API is, to use an example I was just working with. To use NetworkManager API correctly, you would need to regenerate all proxies whenever the NM daemon reappeared on the bus. IMO if an app is written nicely with good dbus binding support, this is just about as easy as doing things wrong; you just have to make it so that in the well-known-name-appeared callback you create your proxies, and in the well-known-name-vanished callback you destroy them. Not really harder than creating the proxies in a one-time init function. Yes apps can connect to notify::name-owner, which is convenient for one singleton root object, but I think kind of fails for NetworkManager where you have a root object containing devices containing access points ... and the same object path across restarts might not even be the same logical object. So the well-known-name proxy here makes sense only for the root singleton object, and all other proxies should be created with unique name inside the notify::name-owner on the root singleton, or something. Anyway, bottom line, to me a separate watch_name operation plus unique-name proxies makes everything a lot more clear and easier to keep correct. * I know sync method calls are convenient, but they are just 99% of the time wrong.... not convinced it's doing anyone any good to let them write code they are going to have to rewrite. Just saying. * I'm not sure it's strictly needed to allow object proxies to represent multiple interfaces - for me, just having two proxies, 1 per interface, would generally be fine. Or just have a proxy with no interface in it and provide the interface at method call time. Harmless but not sure it's useful / worth the effort. * comment says "TODO: Right now we block in the mainloop. This is *probably* wrong." - yes it is wrong! Well there is my couple hundred pennies. Havoc _______________________________________________ gtk-devel-list mailing list gtk-devel-list@gnome.org http://mail.gnome.org/mailman/listinfo/gtk-devel-list
