On Mon, 2007-12-17 at 11:37 +0100, Alexander Larsson wrote: > I commited all of these as a base to work from
Thanks. FYI I'll keep on committing fixes / feature to the hal backend in the mean time (one thing I want is to generate GVolume objects for gphoto2 cameras pointing to a camera:// URI; then do a gvfs backend for those URI's at some point). > It seems this uses dbus-glib-1, which is an unstable API/ABI. Is this > really a good idea? It means gvfs could break at any point. > > Also, it calls dbus_connection_setup_with_g_main() on the main shared > dbus connection. This means that if the application previously > initialized the mainloop with some other kind of mainloop integration > call (for instance the talked about lowlevel dbus-glib integration > library) then things will break in weird ways. For the gio integration > I've used a custom dbus connection OK, I've fixed this; dbus-glib-1 is no longer required for the hal volume monitor. Instead an extra roundtrip (a few actually) to the bus daemon will be made since I have to use dbus_bus_get_private(). > until we have a sane lowlevel > mainloop integration setup (as discussed on the dbus list). We just should get this done and in a dbus release we can depend on. It's really bad to use private D-Bus connections. > You have an empty g_daemon_mount_eject, it should be NULL so we get > proper G_IO_ERROR_NOT_SUPPORTED support from the GMount baseclass. OK, I see that you've fixed this already; (I just committed a small fix to set the can_eject vtable instead of the can_unmount twice.) > All the volume manager code just uses a single name for the GIcon. Why > not use a set of icons I actually thought the gtk+ code using this would use the newly introduced GTK_ICON_LOOKUP_GENERIC_FALLBACK that is available as of 2.12... > +get_mount_for_mount_path (const char *mount_path) > ... > + if (the_volume_monitor == NULL) > + { > + /* Dammit, no monitor is set up.. so we have to create one, find > + * what the user asks for and throw it away again. > + * > + * What a waste - especially considering that there's IO > + * involved in doing this: connect to the system message bus; > + * IPC to hald... > + */ > + volume_monitor = G_HAL_VOLUME_MONITOR (_g_hal_volume_monitor_new > ()); > + } > > get_mount_for_mount_path is used in the implementation of > g_file_find_enclosing_mount() for local files. This implementation stats > up the filesystem looking for the toplevel of the mountpoint. This is > then passed into get_mount_for_mount_path to get a GVolume object for > the volume that is mounted at that particular place. > > This call was added to implement things like displaying where a file is > stored (with volume icon + name) in the properties dialog, and to e.g. > allow unmounting the current filesystem when displaying the toplevel of > a volume. > > There is no requirement for the returned GMount object to be part of a > full volume monitor. Instead the idea is that we can just create one for > the particular mountpoint. This should be far more efficient than > creating a full volume monitor. That's my understanding when I read the code too (it would probably be good to add this to the docs too). However, if we want the information to be accurate (and I think we do) we do need to connect to hal. Since I just introduced a fast path for getting info from hal [1] it's not really more expensive, IO wise anyway, to construct a full volume monitor. In the majority of cases (nautilus, file chooser) the caller will already have a volume monitor so there's no penalty for them. I can't think of any apps without a volume monitor that would need to call it. [1] : gvfs uses libhal_get_all_devices_with_properties() if it's available (both build- and runtime checks are in place). This reduced the time to get info from hal from 54ms to 15ms by doing a single IPC call instead of N+1 (with N being the number of devices). (The hal bits will land in hal git later today - needs a little tweaking). > I see that you're building the hal stuff in a separate module. It might > be better to add it to the general gvfs module. This will save 4k of > writable memory per process that uses gio. Sure, I'll look into doing this when... > > One fix is to make it handle when the constructor for a > > GNativeVolumeMonitor fails - this is important to handle as the hal > > volume monitor will fail if hald isn't running. I'm not happy about > > how I've implemented it; suggestions welcome. > > The way this is handled by g_vfs_get_default() is that the instantiated > GVfs object is checked with g_vfs_is_active() to make sure that > construction was successful. This is the general pattern used for this > in GObject. As there is no exception handling in GObject it is not > generally allowed for construction to fail. > > I'll start working on these details in svn. (cont'd from above paragraph) ...this is done (sounds like you are doing it? otherwise I can look into it). > Also, is there a risk of the hal_ symbols in that module leaking out? We > load the file with G_MODULE_BIND_LOCAL which should mean this is not a > problem, but maybe there are system where this is not supported? Does > anyone know of something like that? Hmm.. I thought module_flags = -export_dynamic -avoid-version -module -no-undefined -export-symbols-regex '^g_io_module_(load|unload)' took care of that; I mean, we really only need to export the two functions g_io_module_(load|unload) right? My knowledge of symbol visibility is a bit rusty, sorry. > /* TODO: Since dynamic types need some fixing we want to make ourselves > resident; I couldn't > * figure out how to get a GModule from GIOModule so do this hack > until then > */ > g_type_module_use (G_TYPE_MODULE (module)); > > A GTypeModule (and thus a GIOModule which is a subclass of it) is not > actually a GModule. It is something that defines a set of GTypes that > are dynamically loaded (from e.g. a GModule or somewhere else). It knows > what types are contained in the module and only loads the actual code > when these types are in use (by calling GTypeModuleClass->load which in > GIOModule is implemented by loading the .so). > > Thus, you really should handle dynamic types correctly. Otherwise the > GModule won't be unloaded after the inital scan of the GIOModules even > if the application doesn't use the types in question. I saw you did this already. Thanks. David _______________________________________________ gtk-devel-list mailing list gtk-devel-list@gnome.org http://mail.gnome.org/mailman/listinfo/gtk-devel-list