Hey everybody, We've been doing a GIO API review in the last couple of days and here is the list of comments and issues we've come up with:
General: ======== It seems GIO allows individual files to be included, this should be avoided like gobject does it: #if !defined (__GLIB_GIO_H_INSIDE__) && !defined (GIO_COMPILATION) #error "Only <gio.h> can be included directly." #endif A big issue is that GIO wastes namespaces massively: g_app, GAsync, g_buffered, g_cancellable, g_content, g_data, g_desktop, g_directory, g_drive, g_file, g_filename, g_filter, g_icon, g_input, g_io, g_schedule, g_cancel, g_loadable, g_local, g_memory, g_mount, g_native, g_seekable, g_simple, g_themed, g_unix, g_vfs, g_volume, g_win32, g_push, g_pop That's clearly too much and can be reduced. While these are not strictly "namespaces" (the namespace is g_), they partly clash with g_foos and g_bars we already have, or might need in the future. We stronlgy suggest to use a common g_io/GIO prefix for all functions/types in GIO. GAsnc* -> GIOAsync* G*Stream -> GIOStream* GIcon -> GIOIcon G*Icon -> GIOIcon* GCancellable -> GIOCancellable ... Also, subclasses should probably append their name, not prepend it: GFilterOutputStream -> GIOOutputStreamFilter GUnixOutputStream -> GIOOutputStreamUnix ... This makes the file and inheritence structure much clearer and would be consistent with stuff we already have (e.g. GtkTreeModelFilter, GtkTreeModelSort). All progress callbacks should have a cancellable argument, otherwise you always need to pass the cancellable as user_data if your progress dialog has a cancel button. All parent instance members should be called "parent_instance" for consistency, "parent" is a far too common member name to be used like that. Nitpick: The number of padding elements in class structures varies, why? GFile: ====== g_mount_for_location should probably be g_file_mount_for_location Some async operations are not implemented: _query_settable_attributes_async _query_writable_namespaces_async _delete_file_async _trash_async _make_directory_async _make_symbolic_link_async _copy_async _move_async Is there an ETA for these? Especially copy_async() and move_async() seem very important missing pieces. A General issue with GFile is that the name suggests that you are holding some kind of file handle, where you actually have nothing but a filename (or uri). This could be problematic since the API does not make clear which functions simply work on the filename representation in memory and which do actual I/O (read: which function is cheap and which is expensive). E.g. g_file_contains_file gives no hint about whether it's just doing string operations on the filename, or actual I/O. Since GFile is merely an interface, it can probably not be stated generally which of the functions does I/O and which doesn't. It might however make sense to mention that in the docs for the built-in default implementation of local files. GFileEnumerator: ================ GFileEnumerator -> GIOFileEnumerator GMountOperation: ================ GMountOperation -> GIOMountOperation G_PASSWORD_FLAGS_ANON_SUPPORTED -> G_IO_PASSWORD_FLAGS_ANONYMOUS_SUPPORTED Most of the API seems to deal with setting user/pass/choice in response to signals. Shouldn't there be one function to pass the entire set of response fields related to one signal instead of a combo of setters plus reply() ? G*Monitor: ========== GFileMonitor -> GIOMonitorFile GDirectoryMonitor -> GIOMonitorDirectory Wouldn't it make sense to have a common (perhaps abstract) base class here, or derive one from the other? The APIs are very similar at least. GFileAttribute: =============== GFileAttribute* -> GIOFileAttribute* GFileAttributeValue: why is the union not a GValue? Our biggest issue is probably that the attributes are semantically very similar to object properties and should therefore follow the conventions introduced there: Namespace should be separated from attribute name by '::', not ':', gobject does this for both properties and detailled signals, so GIO should name file attributes consistently. The use of '_' should be avoided, please use '-' instead. We put a great deal of effort into canonicalizing identifiers elsewhere (object property names and signal names), so GIO should follow that habit here for the sake of consistency. Also, there is no good reason to use abbreviations like "std" and "fs", we are not in the 60ies any more ;) std -> standard fs -> filesystem std:type -> standard::type std:is_hidden -> standard::is-hidden There also seems to be quite some duplication between GFileInfo and GFileAttributeValue for no good reason, such as: g_file_info_get_attribute_string vs. g_file_attribute_value_get_string g_file_info_set_attribute_int32 vs. g_file_attribute_value_set_int32 ... Where these functions in GFileInfo just seem to create/get a GFileAttributeValue and call the corresponding API, in fact these seem to be the only calls to g_file_attribute_value_* inside GIO. It also seems like a good idea to provide a more generic API similar to g_object_get in order to get multiple values at the same time: g_io_file_info_get_attributes (GIOFileInfo *info, const gchar *first_attribute_name, ...) G*Stream: ========= GInputStream -> GIOInputStream (or even GIOStreamInput) GFileInputStream -> GIOInputStreamFile (or even GIOStreamInputFile) ... GCancellable: ============= GCancellable -> GIOCancellable There is no need to break the common prefix for global things: g_push_current_cancellable -> g_cancellable_push_current g_pop_current_cancellable -> g_cancellable_pop_current G*AsyncResult: ============= GAsyncResult -> GIOAsyncResult GSimpleAsyncResult -> GIOAsyncResultSimple GIOScheduler: ============= Each function has a different namespace here, I suggest: GIOSchedulerJobFunc GIOSchedulerDataFund g_io_scheduler_schedule_job g_io_scheduler_cancel_all_jobs g_io_scheduler_send_to_mainloop Especially the "schedule" and "send_to_mainloop" functions are quite similar. Doesn't the first require a running main loop too? Either it's mis-naming or lack of documentation (or understanding on my side ;-) G*Icon: ======= Same as above: GIcon -> GIOIcon GFileIcon -> GIOIconFile GLoadableIcon -> GIOIconLoadable GThemedIcon -> GIOIconThemed GContentType: ============= Same namespace issues: GContent* -> GIOContent* g_content_type_guess: problematic since not all contents can be guessed from file headers (which is what this function probably does). Perhaps rather pass a GFile or a GInputStream so the file's end is accessible too? GAppInfo: ========= GAppInfo -> GIOAppInfo _______________________________________________ gtk-devel-list mailing list gtk-devel-list@gnome.org http://mail.gnome.org/mailman/listinfo/gtk-devel-list