Re: Comments on GTK+ patches from Atk tarball

[Owen Taylor <otaylor redhat com>]

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.

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

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

 * 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'm still getting the feeling that there is some confusion
   about the way that casting/wrapper functions/etc. work.

   [   
     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. Say, you wanted
     to call the interface 'AtkImplementor'. Then your header
     file looks like:

     typedef struct _AtkImplementor AtkImplementor;  /* dummy typedef */
     typedef struct _AtkImplementorClass AtkImplementorClass;
   
     struct _AtkImplementorClass
     {
       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:

    object = atk_implementor_ref_accessible (ATK_IMPLEMENTOR (widget));

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

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

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

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

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

Regards,
                                        Owen