Re: GIO API review

[Alexander Larsson <alexl redhat com>]

On Wed, 2007-12-12 at 15:13 +0000, Martyn Russell wrote:
> 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.

I don't think this is something that is all that important when you
actually use the API. It certainly isn't so important that its worth
(methaphorically) punching you in the face every time you read or write
the symbol.

> > 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.

I wouldn't call it inconsistent. I think it is a correct design decision
that it is called g_signal_stop_emission, and not
g_object_signal_stop_emission.

> > 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?

No it can't, because it uses GObject. This goes back to the previous
discussion about what library to use when adding other gobject based
APIs like DConf to glib. At the moment I think the idea is to put those
in libgio, but have separate headers and a separate .pc files. This
makes the g_io_ prefix even weirder.


> > 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.

I think this misses the point. I don't see the primary thing for a class
name is to describe what class it inherits from, it is to describe what
the class *does*. In some cases this is a completely different name from
the base class, as in GtkButton vs GtkWidget (not GtkButtonWidget), but
sometimes a good way to describe the class is to prepend something to
it. Take for instance GDataInputStream, where "Data" was prepended,
resulting in a new name that describes what it does (data input). 

Now, there is sort of a third case which is where we have a
implementation of an abstract class/interface for a specific backend,
such as GVolume/GUnixVolume or GFile/GLocalFile. I think its really this
case that people think is better to use postfixes for. But I don't think
its better even there.

> > 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.

Its certainly possible to cancel from the callback. All you have to do
is store the cancallable in the dialog and pass the dialog pointer as
user data. 

Now, do we want to make it *easier* to cancel from the progress
callback. I don't think doing so is a good design as it will cause
cancellation to not be as responsive as it should be. So, passing the
cancellable in the progress callback would encourage problematic
designs.

> >> 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

It doesn't actually always run in a thread. If GThread isn't initialized
its run in an idle, so _to_threadpool seems wrong.

> OR
> 
>   g_io_scheduler_job_add
>   g_io_scheduler_job_use_mainloop
>   g_io_scheduler_job_cancel_all

Hmmm... use_mainloop seems weird. call_in_mainloop maybe?