Re: Comments on GTK+ patches from Atk tarball

[Bill Haneman <bill haneman ireland sun com>]

Owen Taylor wrote:
> 
> Hi Bill,
> 
> I don't have a lot of time right now respond to your mail
> in detail; a lot of other stuff to do right now to get
> done before GUADEC. So, this is just basically brief notes.
> 
> I think it might be best if we (with Tim, Havoc, etc.) say
> down for a an hour or two at GUADEC to get get the details
> nailed down.

I'd love to do that, I think it would be very helpful.  Thanks for
making the time for this...

> 
>  * Although GtkAccessibleFactory is not the way we typically
>    do things in GTK+, its a reasonable clean way of doing
>    things, and if you (as the primary consumer) don't find
>    it painful, we can add it.

Thanks... this was the best model we could find for externalizing the
implementation efficiently and flexibly.
 
>  * I don't think your gtk_widget_class_set_accessible_factory
>    even works the way you think it does. (Look at what happens
>    when a child class is initialized before
>    gtk_widget_class_set_accessible_factory() is called.)

I think your point is that the factory can't be inherited "after the
fact".  So far we've only loaded our accessibility support via
GTK_MODULES preload, so this hasn't bitten us.  If we migrate to an
on-demand gmodule, which we might need to do (as discussed in a
previous email) then you are right, we will need a way of propagating
the factory through the inheritance tree.

>    Let me propose a compromise solution:
> 
>     void gtk_widget_register_accessible_factory (GType                 base_type,
>                                                  GtkAccessibleFactory *factory);
>    This can be implemented in several ways, as simply a hash table
>    from GType to GtkAccessibleFactory, or with g_type_set_qdata().
>    While this still causes all the types to be registered, type
>    registration is much less expensive than full class initialization.
> 
>    The overhead lookup here via type-ids when first accessing the
>    accessiblefor a widget should not be prohibitive - we do
>    similar lookups for many other opeations.

OK, that seems reasonable as a one-time expense (thereafter the
get_accessible method will return a cached AtkObject).  Perhaps you
can help quickly map out an implementation when we sit down at GUADEC.

> 
>  * Your (*get_accessible) virtual function should most likely
>    be (*create_accessible) since it seems to have different
>    semantics than the public get_accesslble() member functions.

I don't see this, it only does a "create" if there is no cached
value.  The signatures of gtk_widget_get_accessible, (static)
gtk_widget_real_get_accessible, and GtkWidget->get_accessible are
identical, and so are the semantics (except for details of conditions
leading to a NULL return value).

>  * I'm still getting the feeling that there is some confusion
>    about the way that casting/wrapper functions/etc. work.

You'll have to be specific, our usage pattern looks just like that of
glib/GTK+ to us ;-)

>    [
>      Quick sidenote, AtkObjectIface, AtkIfaceImplementor and
>      atk_object_ref_accessible are completely misnamed, to
>      the point I can't even talk about them properly, since
>      they have nothing to do with AtkObject. 

Well, not quite, the interface is all about *accessing*
AtkObjects...but I take your point.  We can use AtkImplementor and
AtkImplementorIface instead (your suggestion for "AtkImplementorClass"
seems to break with the Gtype interface naming model a little as
well).

I have edited your suggestion below, does this look OK ? :

      typedef struct _AtkImplementor AtkImplementor;  /* dummy typedef
*/
      typedef struct _AtkImplementorIface AtkImplementorIface;
 
      struct _AtkImplementorIface
      {
        GTypeInterface parent;
 
        AtkObject*   (*ref_accessible) (AtkImplementor *accessible);
      };
 
      AtkObject *atk_implementor_ref_accessible (AtkImplementor
*implementor);
 

>    You seem to think that there is something highly painful about
>    using AtkImplementor methods on a GtkWidget. Maybe you were
>    referring to the memory management. If not, it's just:

Yep, mostly the memory management is a hassle, the method usage below
is ok.  

>     object = atk_implementor_ref_accessible (ATK_IMPLEMENTOR (widget));

But most of the time within GTK+ and GTK+ apps one will just be using
the set methods, you don't want the memory hassle.  I suppose that we
could make set convenience methods or macros that did this:

   get a reference to the AtkObject
   set a property on the AtkObject or a subinterface
   decrement the refcount

But this seems so unnecessary when 

   a) we have a persistent object version used inside GTK+, and
   b) doing a "set" on a flyweight (as above) is pointless since it
will not persist.

>    Similarly, your condensed pseudo-code
> 
>    ====
>   /* set the GValue */
>   AtkValue accessible_value;
>   AtkObject accessible = gtk_widget_get_accessible (widget);
>   accessible_value = atk_object_get_value (accessible);
>   val_was_set = atk_value_set_current(accessible_value, value);
>    ===
> 
>    is, including all casts, simply:
> 
>    atk_value_set_current (ATK_VALUE (gtk_widget_get_accessible (widget)),
>                           value);
> 
>    Look at how GtkEditable works. From C, the lookup of the interface
>    is completely hidden inside the wrapper function
>    atk_value_set_current().

Right, this is what we are striving for.  Not sure why I forgot the
ATK_VALUE macro.


>  * Similarly, gtk_widget_get_accessible() can return a AtkObject without
>    causing much pain if we later introduce a "GtkAccessible"
>    subclass and people want to be able to call methods on that.
> 
>    Note that gtk_XXX_new() returns a GtkWidget * for all basically
>    all XXX.
> 
>  * We generally look dimly on making "protected" member variables part
>    of public interfaces. For one thing, they don't map into language
>    bindings at all well. Most GtkWidgets are close to completely
>    private in their member variables. Where not, the discrepancy
>    is typically historic.

Well, historic or not it is there, that's why we got the idea that
direct reference to members is common in GTK+ ;-)

If we had a "friend" concept in C that's what we would use for
gtk_accessible_get/set_widget(...).  The only place where the widget
member should be used is in the accessibility implementations (that
is, subclasses of GtkAccessible).  Mostly this will happen in the
external library, but the idea was that if a custom widget maintainer
wanted to install a factory or implement an AtkObject accessor for the
widget, the GtkAccessible class should be subclassed.  We see
GtkAccessible as the thin "glue" between GtkWidget and the Atk
accessibility interface definitions.  But of course we can hide
GtkAccessible, it will however make it a little less clean for some
custom widgets so there may still be an argument for keeping it
exposed.

On the other hand, if as we discussed we need to provide some property
cacheing for accessible properties in the absence of the external
library, then we will need to turn GtkAccessible into something more
"real", a trivial implementation with a property cache which lives in
libatk and not the external library.  

>    So, GtkAccessible, which has one member variable with no
>    accessors doesn't seem right to me.
> 
>    If getting the backpointer is something that a public user of
>    GtkAccessible needs to do, I'd consider having a ->get_widget()
>    virtual function with a wrapper member function to be the
>    right method.
> 
>    But in general, I don't really see having a public GtkAccessible
>    in GTK+ as something we need currently.
> 
>  * If we want to go with the model where we have one accessible
>    uniquely and statically associated with each GtkWidget, well,
>    then your patch needs to not leak that accessible ;-)

Right, Padraig fixed that this week :-)  Actually the accessible
object's association with a GtkWidget is not always entirely static,
but it usually will be (unless an adaptive technology client of ATK
wants to replace the AtkObject implementation with its own, as
mentioned in other email).

>  * GtkTreeView is the list/tree widget for GTK+-2.0. GtkList, GtkCTree,
>    GtkTree, GtkList are all deprecated, though I expect CList to still
>    be pretty commonly used in a lot of ported programs for a while.
> 
>    For a list, you simply add a flat model (GtkListStore usually)
>    to your GtkTreeView.
> 
>    In GtkTreeView, there is no widget per row. You can keep references
>    to rows around persistantly via something called GtkTreeRowReference,
>    but those are quite expensive, and I don't think it make sense
>    to try and keep per-row persistant data around for GtkTreeView.
> 
>    So, if you have a AtkObject for navigating around within
>    a GtkTreeView, it definitely should be a flyweight.

That was our conclusion also... does this mean that GtkListItem will
also go away?

One thing that seems to be a little weak in GTK+ is support for
spreadsheet-like tables, if there were a stock table widget it would
make it less likely that application-specific code will break our
accessibility support.  For instance if you can't select rows with the
widget, and have to put that logic in app-specific code, the app will
also have to add explicit accessibility support code in order to
support AtkTable, ugh.

>  * Putting wrappers around Atk to make it "pleasant to use with GTK+"
>    generally strikes me as something we should not have to do.
> 
>    Since you are writing it using GObject, you are using something
>    very close to GTK+'s native idiom, and a translation layer between
>    that and the real GTK+ native idiom would say to me that something
>    is wrong with the base layer.

OK, it is reassuring to know that you feel AtkObject is using a
reasonably familiar idiom, in which case I agree that additional
wrappers are an unnecessary complication.

I am looking forward to sitting down and ironing out any remaining
issues with you and also Havoc, Tim, and others.  However I'd really
like to get the naming-type stuff resolved beforehand since there are
a number of other accessibility issues that will need to be discussed,
and which are not as far along as ATK+ at the moment - things like
packaging the external library, and the way the SPI will interact with
GOAD, issues to do with widgets in other gnome libs, ...

All the best,

-Bill

> Regards,
>                                         Owen

-- 
--------------
Bill Haneman
Gnome Accessibility / Batik SVG Toolkit
Sun Microsystems Ireland