Re: GIO API review

[Martyn Russell <martyn imendio com>]

Alexander Larsson wrote:
> On Tue, 2007-12-11 at 17:48 +0100, Michael Natterer wrote: 
>>
>> GAsnc*        -> GIOAsync*
>> G*Stream      -> GIOStream*
>> GIcon         -> GIOIcon
>> G*Icon        -> GIOIcon*
>> GCancellable  -> GIOCancellable
>> ...
> 
> I strongly oppose this. 

I personally prefer the naming Mitch suggests. I know from the name
which sub-library the API belongs to in GLib.

> So, I don't think the problem with this is that bad. I mean, gobject
> doesn't call its symbols GObjectClosure, g_object_signal, GObjectValue,
> etc, and this has not been a problem.

There are of course inconsistencies through out the toolkit already.

> The negative aspect of such a change is that every symbol and name gets
> longer, which is harder to read and harder to write. Also when you work
> on a higher level you're forced to know which sub library of glib each
> class is from (which you don't really care about, especially if these
> types are exposed in higher level APIs). 

That's true.

> It also makes the symbol names weird. I mean GIOIcon? Is that in icon
> somehow related to an io operation (no), or is it an generic icon type
> (yes, that we hopefully will be using in a lot more APIs later)?

Well, that depends how you look at it. Some people might think that it
is just an icon object which is part of the GIO library, others might
think it is an icon object with IO functions.

I suppose you could argue that if it doesn't have any IO functionality,
could it be added to the glib core library?

Before you mentioned that it could be considered to have IO
functionality, I hadn't really thought of it like that, I just assumed
it was an Icon object in the GIO library :) But I see your point.

>> 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).
> 
> This was brought up on this list before during the initial gio reviews.
> The original version in fact had things appended (at least at places, it
> wasn't totally consistent). But it was all cleaned up to be consistent,
> and after that discussion I picked the current model.
> 
> And I really think the current model is better. Its flows more natural
> in human language and its what other programming languages do, Its also
> easy to understand what name a class should have, while the postfix
> model does not always make this obvious, especially with multiple levels
> of inheritance:

I tend to find the current naming less clear regarding what exactly it
inherits from and takes longer to adopt to when learning the new API. I
agree though, it doesn't sound too nice when saying it from a human
language point of view.

Just don't look at the Gossip source code ;)

>> 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.
> 
> While it certainly won't hurt to add it there I don't really see it as
> much of an advantage either. You really want the cancel button to
> immediately cancel the operation, not wait until the next progress
> callback. Furthermore, many ops don't have a progress callback so you
> need a cancellable object stored anyway. So, the natural thing is to
> store the cancellable in the cancel dialog and then pass it into the
> operation and use this for all cancellation. Then, when updating the
> progress bar in the dialog cancellation is not involved at all.

That is assuming you never want to cancel the operation from some
calculation inside the callback. I personally would prefer the
cancellation in the callback.

>> 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
> 
> standard::type is 14 bytes, std:type is 8, given 10 std:* attributes per
> file and 1000 files in a directory, this means transfering 60000 more
> bytes over dbus when reading this directory using gvfs. I'm not sure
> this is a horrible number, as there are bound to be other slow parts
> when involving gvfs (such as network traffic), but it might matter.
> 
> I don't mind changing this though. What do other people change?

I think "standard" and "filesystem" are better. They save questions and
people looking up the acronyms in the documentation too.

>> 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.
> 
> GFileAttributeValue was initially an implementation detail of GFileInfo,
> but I made it publically availible so that i could implement
> g_file_set_attribute() as a single vtable function instead of having one
> for each type. Maybe this was the wrong move though, as its kinda ugly
> and not used anywhere else. I could have set_attribute take a type + a
> gpointer.
> 
>> 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,
>>                                 ...)

+1.

>> 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
> 
> Yeah, this seems like unnecessary pollution. I'll fix.
> 
>> 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 ;-)
> 
> I think it might be bad naming from my side. g_schedule_io_job()
> schedules the run of a function in the threadpool.
> g_io_job_send_to_mainloop() is used by the job to send something from
> the job thread to the mainloop (oneway or blocking until its finished
> running).

It's not a great suggestion, but maybe:

  g_io_scheduler_send_to_threadpool
  g_io_scheduler_send_to_mainloop
  g_io_scheduler_cancel_all

OR

  g_io_scheduler_job_add
  g_io_scheduler_job_use_mainloop
  g_io_scheduler_job_cancel_all

-- 
Regards,
Martyn