Re: Comments on GTK+ patches from Atk tarball

[Owen Taylor <otaylor redhat com>]

Bill Haneman <bill haneman ireland sun com> writes:

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

There is no guarantee that GTK_MODULES modules will get run
before the initialization of all classes.

A possible solution would be to use the base_init member GTypeINfo
to clear the class field when inheriting, and search though
parent classes yourself, but I think not using the class structures
is probably better.

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

My feeling is that if you are relying on the widget/accessible
association being static, then you should enforce that in the
implementation of gtk_widget_get_accessible(), and have
gtk_widget_get_accessible():

 - Check if widget->accessible is already set
 - If not call, gtk_widget_create_accessible() and store the result

I don't like the idea of forcing everybody who wants to override
get_accessible() to reimplement this behavior, and I don't think
variation from this behavior is allowable if you are exporting
a non-ref-ing gtk_widget_get_accessible().
 
> >  * 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 ;-)

Well, the AtkObjectIface AtkIfacImplementor situation, etc, was one 
thing I was thinking of; also atk_object_get_value_interface(), etc.

Also, comments like:

 *  The usage model for obtaining these interface instances is:
 *    ATK_<interfacename>_GET_IFACE(GObject *accessible),
 *    where accessible, though specified as a GObject, is
 *    the AtkObject instance being queried.
 *  There may be additional macros, thus:
 *    if (ATK_TEXT_GET_IFACE(accessible))
 *      gint pos = ATK_TEXT_GET_CARET_POSITION(accessible);

While to conform to GTK+ convention, public uses of interfaces
should uniformly go through the wrapper functions.

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

Well, I'd have to be admit that we aren't entirely consistent;
GtkEditable and GTypePlugin use "class", while the tree interfaces,
GtkTreeModel, GtkTreeSort, GtkTreeDND, use Iface.

So, we clearly have to fix that up. The thinking behind using
*Class is that we should uniformly use <blah>Class for the 
vtable for the <blah> type.

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

I just wanted to make sure that what you saw as being painful
was the memory management, since it also seemed like you
might be finding using interfaces painful.

[...]

> >  * 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+ ;-)

Note that direct accessing of structure members is almost universally
getting, not setting. And this holds true even when the structure
members are used from derived classes.
 
> 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.

friend/protected functions are done in GTK+ by simply making the
same as public functions and documenting there use.

But if the only function of GtkAccessible is to store a single
pointer for implementation use, I don't think people implementing
custom types of AtkObject really finding reimplementing that
a chore.
 
> 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.  

I have some doubts about the idea of switching between different
AtkObject implementations at runtime and expecting state to
transfer. I think if preservation of state is important and you
can't just always use the same implementation, the state should
be stored on the widget, either as object data or with accessors.

(though you can look at GtkIMMultiContext for an example of
a somewhat similar situation where switching of IMContext 
implementations for a running widget is implemented.)

[...]
 
> >  * 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.

A GtkSheetView widget along the lines of GtkTreeView is possibility
for a future version of GTK+, but no, we don't have that currently.
 
> >  * 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

OK, naming issue #1. Don't call it ATK+. :-) The + in GTK+ is a
historical accident, and we have to keep on explaining it.

Regards,
                                        Owen