Re: GTK+ Accessibility Proposed API

[Bill Haneman <bill haneman ireland sun com>]

Havoc Pennington wrote:
> 
> Hi,
> 
> I've gone through the headers and the gtkwidget diffs, I haven't gone
> through the .c files yet. So here are a bunch of comments, some
> nitpicky, some obvious, etc. These are only my opinions, revising
> things before Owen and Tim comment could be dangerous. ;-)

Hi Havoc:

Thanks a lot for reviewing these headers.  As you probably guessed,
some of what you found are typos and goofs of various kinds which
I will fix as soon as I've finished this reply.  In a few cases I
think the problem is more one of explaining why we did things a
certain way.  Certainly we want to be as GTK-like (or at least
Gnome-ish ;-) in our conventions as we can.  Anyhow, in response
to your points:

> Directory layout: I think "gtk/a11y" isn't a good name for the directory
> holding the include files, gtk/ is bad because this is a GTK
> dependency not part of GTK, "a11y" is bad because it isn't the name of
> the library. Suggest just gaccessible/.

Absolutely.  You might have noticed that in the tarfile the directory
structure is different, we use "gaccess" for the "gaccessible"
headers.
But I had forgotten to change this in the #include directives when I
sent the tarball.

There are a couple of headers in the gtk+/gtk/ namespace, but those
are
the few that we think actually belong there. GtkAccessible is a
gtk-specific subclass of gaccessible (has an explicit internal
gtkwidget pointer) and the GtkAccessibleFactory is also a GTK-specific
construct (more on that later).

We wrestled with the namespace stuff for awhile, trying to strike a 
balance between consistency with GTK namespace conventions, and 
reasonable identifier name lengths... I have more to say on this issue 
below also.
> 
> Gaccessible: while I understand the pedantic reasoning that leads to the
> lowercase 'a', I think it's ugly, confusing, hard to type, etc., and
> also I think there's no way we would introduce a GAccessible if a
> Gaccessible was widely used. For GConf I just capitalized the 'c',
> which I know annoys Tim, but I like more.

Yep, we had GAccessible at first but we changed it in response to
feedback.  No strong opinions here about it.

> However, I don't think 'Gaccessible' is a good name for the namespace
> - it sounds like an interface. Suggest 'Gaccessibility,' or make up a
> name in the tradition of Pango. Somewhat prefer the latter, if we can
> think of something nice. Or just "accessibility" - no rule that stuff
> has to start with G.

It is sorta an interface...  Personally Gaccessibility is
just-over-the-edge
of my name length threshold.  A made-up name would be ok but we want
to 
make sure everyone knows what it's about, we haven't come up with 
anything as catchy as Pango yet.  In the "accessibility community" the
word "accessible" seems to be used frequently as a noun, maybe that's 
why it doesn't seem awkward as a name for a class to me.

Two reasons for not using "accessible" or "access" - for one, if
someone
sees an "access" subdirectory it's very likely that he or she will 
think it's about access control... and also we (Gnome Accessibility
folks) will have another namespace for the separate accessibility
interface
that will be a pan-toolkit, out-of-process API for assistive
technologies
to hook into, they won't use Gaccessible directly.  The plan is for
this
"other" API (really an SPI I guess) to be bridged to various
toolkit-appropriate
APIs. ideally including all of the following: the Java Accessibility
API, 
the window manager, this GTK/Gnome accessibility API, Mozilla's 
accessibility API, and the OpenOffice accessibility API.  Of course it
would
be cool if everyone decided to use our GObject-based API directly,
but things may not work out exactly that way ;-) and in some cases it
makes more sense for toolkits to use individual internal APIs (like
Java).
The plan is for this SPI to use Bonobo/CORBA.

So we would like to reserve the "Accessible" namespace for the global
desktop SPI, and the bridge from the GTK+ API to this one would
encounter
namespace clashes if we use "accessible" in the GTK+ API.

> You also use 'Gaccessible' as the name of an object - that doesn't
> work. It's as if GTK had an object called simply "Gtk". i.e. basically
> the name of the object is an empty string. ;-) It should be
> GaccessibleAccessible. But that's pretty lame, which is why renaming
> the namespace might be good.

Yeah, we want to avoid constructions like
gaccessible_accessible_get_accessible_child() as much as possible :-/
Our conclusion was that this small departure from convention was
a good compromise.  The short namespaces that come to mind
(gnome accessibility kit = "gak"), etc., are pretty ugly :-)
And also opaque, which is a liability.  Maybe it's time to
dig out the thesaurus...
 
> gaccessible.h:
>  For copyright notices, you should use the LGPL notice, but put the
>  actual copyright holder, in this case I assume that's Sun
>  Microsystems, instead of Peter/Spencer/Josh. This is a bug in many
>  current GTK copyright notices.

Thanks, I thought this was weird but didn't know the right thing to
do.


>  If the lib is Gaccessible, the include guards shouldn't have the
>  underscore in __G_ACCESSIBLE

OK, cool.

>  You include gobject/* directly, supposed to include glib-gobject.h or
>  something like that, I don't remember the exact header.

ditto.
 
>  The GaccessibleRole enum should have its members namespaced.
>  So GACCESSIBLE_ROLE_ALERT, etc.

<whinge> do we haaaf to ? </whinge>  ;-)
OK, we were hoping that the
ROLE and STATE prefixes would be good enough.
Verbosity again...
 
>  Do we need this enum to be extensible? What about widgets added in
>  other libraries or in enums? You might look at PangoAttributeType
>  or the recent patch I posted for GtkIconSize to see how to do
>  an "extensible enum" (GType is also such an enum).

You're absolutely right, I'll look at PangoAttributeType.

>  GaccessibleStateEnum should not have "Enum" in its name, and the
>  members should be namespaced GACCESSIBLE_STATE_ACTIVE
>  The states that correspond to widget states should probably be named
>  the same as we name them in GTK+; for example, "sensitive" rather
>  than "enabled", be sure "active" means GTK_STATE_ACTIVE, and so on.

OK I guess.  When we expose these in the SPI we'll rename them though, 
since AT vendors may be used to different conventions. 

>  For both these enums, I think a one-line comment above each member
>  explaining its meaning would not be out of line. Then it would be
>  easier for us to know if they correspond to some existing GTK term.
> 
>  As with the Role enum - does this one need to be extensible to handle
>  add-on widget collections such as libgnomeui?

Maybe, I'm not as sure about this one.
 
>  The type macros are not quite right in gaccessible.h, which I think
>  is because the object name is an empty string. However the same
>  mistake was then cut-and-pasted. So e.g. we should have
>  GACCESSIBLE_TYPE_HYPERTEXT, not TYPE_GACCESSIBLE_HYPERTEXT. Basic
>  rule is that all names start with the namespace.

Right.

>  I don't see any real need to forward declare the interfaces in this
>  file, why not just put those in the appropriate interface-specific
>  headers?

The convenience function declarations (for getting the subinterfaces,
near the bottom of the header) need them.  If we include the
interface-specific headers in gaccessible.h we get circular
dependencies.

>  NUM_POSSIBLE_STATES is not namespaced.

Oops, it's supposed to be static, should be in the .c file instead.
 
>  Instead of GaccessibleStateMask, just use GaccessibleStateEnum,
>  renamed to GaccessibleState. (I personally agree that bitmasks
>  should be done this way, with a typedef to guint, but it's
>  inconsistent with all of GTK; we just use the enum type for
>  the bitmasks).

Maybe I'm misunderstanding you here.  We need to be able to 
get a set of state flags from a minimal-sized element, so 
we are using bitfields.  I don't see how we can do this cleanly
otherwise.

This is one of those cases where we want to provide a little more
syntactic sugar, not everyone who will be writing to this interface
will be GTK+ people.  

>  The _parent member in both instance and class struct need not have an
>  underscore prefix.

OK.  

>  I could use some explanation of what Gaccessible is/does, and why it
>  contains a bunch of child Gaccessible. It seems you basically
>  have a tree of these things? My feeling is that the object should
>  have a more descriptive name, but I can't suggest that name since
>  I'm not sure what the object does.

It's hard to give a more descriptive name since the only thing
that Gaccessible's have in common is that they "_are_ accessible",
meaning that they implement a certain interface.  In a UI context
a Gaccessible is an accessible affiliate of a widget - it could
conceivably be the widget itself (though we are not doing things
that way in 2.0), it could be a proxy or adapter, the point is 
that it is in some way an agent for the widget which provides
access to the widget's functionality and state via a unified
interface.  We could say GaccessibleObject, but that really
doesn't provide any more information.  

The meaning of "children" of an accessible is like 
"children" of widgets, thus there is a hierarchy of
Gaccessibles just as there is a hierarchy of widgets.  
However there is not a 1:1 mapping, some widget children may be 
omitted from the accessibility tree if they do not perform a
compelling function from an accessibility point of view.
Examples might be border or layout widgets that sit between 
containers and visible widgets, probably GtkVBox will not
have an accessible, and a get_accessible_children() call on
GtkVBox's parent will return the children of GtkVBox.

(Sorry about the convoluted language of that last sentence)

>  We would conventionally say get_n_children() rather than
>  get_child_count(), I think. The _count() convention is sort of
>  a Java-ism.

Caught me yet again ;-)

>  ref_child should IMO be get_child and not increment the reference
>  count.

We use "ref" because sometimes the child will be a featherweight
object that has no widget and was created in response to the 
query.  It causes a little more pain for the caller but it's the
cleanest general solution we can see.  A good example is a
"table" (like a spreadsheet, not GtkTable) - each cell is
an accessible but you don't want to keep nXm accessible children
around for big tables.
 
>  I would make the "guint i" into "gint i", on the "unsigned ints are
>  Evil" principle, Owen will probably agree and Tim will probably
>  disagree.

Ugh, I just changed those from gint ;-)
Actually I don't like negative indices, guint helps protect 
against common indexing errors...

>  Perhaps avoid that issue though; I don't see why the interface
>  exposes indices. It's not inherently done as an array.
>  Why not just:
>   GList* get_children (); /* returns a list of Gaccessible */
>   Gaccessible* get_parent ();

Not good for sparse 2d arrays ("tables").   It means that when
traversing the children you keep at least N references at child
N instead of just one.

>  What is the meaning of the "children_changed" signal?
> 
>  changed_type passed to that signal should be an enum I'm guessing,
>  or there should be a comment explaining what sort of value it holds.

Yep, we I got lazy and didn't define the enum yet.  Will fix.
Children_changed means what you might guess, enum CHILD_ADDED,
CHILD_REMOVED, CHILD_CHANGED (mutated or edited).
 
>  Even if we keep the index-based API, suggest removing child_index
>  from the signal, people can call get_index_in_parent.

Yep.  We couldn't find a marshaller for 3 params anyhow ;-)
 
>  GaccessibleInterface: You're missing the "dummy typedef"
>  passed to methods on this interface (see GtkTreeModel
>  vs. GtkTreeModelIface). Of course the dummy typedef would be called
>  Gaccessible which is a different object, so that's a problem. ;-)

OK, will use GaccessibleIfaceImplementor or something.
 
>  This interface to me is "any instance which has_a accessibility
>  object." I would actually call that GaccessibleAccessible, i.e. a
>  thing that's accessible. Then perhaps rename current Gaccessible to
>  GaccessibleInfo, GaccessibleHandle, or something. No good ideas at
>  the moment. Of course the struct containing the vtable is
>  GaccessibleAccessibleIface, and GaccessibleAccessible would just be
>  a dummy typedef.

We're really trying to avoid that verbosity which would propagate
through the namespace.  
 
>  Note that non-GObject can implement interfaces, so the ref_accessible
>  method should take a GaccessibleAccessible, not a GObject.

Cool. we will do this.

>  Again I'd make it get_accessible() and not ref the return.

There are two aspect to our base accessibility entry point: there
is the object representation ("Gaccessible") and the interface that
says "can get an accessible object" ("GaccessibleIface").
In the case of GtkWidget, we know that we can always get a
Gaccessible, and that the Gaccessible is persistent and not
flyweight.  Thus:

gtk_widget_get_accessible(...)

However, to allow our model to be used outside
of GtkWidget, we have defined the interface GaccessibleInterface
which we can use to query objects for accessibility.  Then to
allow consistent use of the model we make GtkWidget implement
GaccessibleIface.  GaccessibleIface defines

gaccessible_object_ref_accessible(...)   which

I suppose in light of your comments should be just

gaccessible_ref_accessible(...)

However for convenience and efficiency we do
not use the interface reflection methods in our Gtk-specific
implementations, and we can use "get_accessible" and not have to
worry about unref-ing the return.  We don't have this luxury in
the general case, which is why we use "ref" in the Iface
declaration and convenience functions, since the accessible
returned may be flyweight.  

>  gaccessible_ref_accessible_child(): the "accessible" there seems
>  redundant, the name of the object (if it existed) would indicate
>  what kind of child it has.

This is intended to reduce confusion in the GtkAccessible case,
where the widget children do not correspond to the Gaccessible
children.

> 
>  gaccessible_get_index_in_parent(): the function pointer in the vtable
>  takes a guint argument, this function does not.
Oops... 
>  gaccessible_set_parent(): I think gaccessible_add() would be better,
>  i.e. make the method on the parent, as with gtk_container_add().
>  But again I'm unclear on why accessibles are in a tree structure,
>  so I may be wrong on this.
>  Can children be removed? What's the rationale for why or why not?

This is a seldom used feature, but when it is needed the model seems
to
be "set child's parent and send parent notification".  I'll confer
with some of the other architects.
 
> 
>  The GaccessiblePropertyChangeHandler typedef should be to a function
>  pointer not a GClosure; if using GClosure, don't use a typedef.
>  But in general GClosure is only used for language bindings, so if
>  connect/remove property change handler are C convenience functions,
>  using GClosure simply makes them less convenient.

Thanks, we are still learning about the GSignal handling stuff.

>  That said I tend to doubt that we need convenience functions, people
>  can just connect to notify on the properties.

Again, we need some syntactic sugar here since we anticipate that some
aspects of this API will be used by non-GTK+ programmers, in the
bridge(s)
to assistive technologies.
 
>  For the properties, they should not be namespaced. That is, they
>  should not begin with accessible_. Properties are just member
>  variables, right, you wouldn't namespace those with the class name
>  (because it's done automatically, as with properties).

In this case the reason is probably more mnemonic, I agree that it
is not necessary to avoid clashes.  It's because the "accessible name" 
of a component may be different from its "name", etc. 

>  The comment here talks about Gaccessible*Interface - for some
>  reason we have the convention to abbreviate these virtual
>  tables as "Iface", maybe since app programmers aren't supposed
>  to ever type the name, but anyway should be consistent.
>  Again see GtkTreeModel/GtkTreeModelIface for example.

Oops, I meant to fix that as well ;-) Great, fewer keystrokes!

>  The comment describes using interfaces directly by poking around in
>  the vtable, generally that's not done, since wrapper functions are
>  provided for each method (which you've done here of course).
>  The RELATION_CONTROLLED_BY etc. enum should be named and typedef'd,
>  rather than anonymous, and should have its members in the Gaccessible
>  namespace.
OK, thanks.
 
>  gaccessible_relation_new() should then take a GaccessibleRelationship
>  as an arg, not guint.
Right. 
>  It isn't obvious to me what state_mask_get/set_name() are used for.

This is for associating mask names with state enum bitmasks - and
using
strings here allows localization of state names.  So I can say
"get me the bitmask for the 'sensitive' state", and then return
a localized string to the AT which the user might receive, say, from
a talking screenreader:
"Button 'Save As' is inactive"
 
> gaccessibleaction.h:
> 
>  Copyright to Sun
> 
>  s/__G_ACCESSIBLE/__GACCESSIBLE/
> 
>  TYPE_GACCESSIBLE_ACTION -> GACCESSIBLE_TYPE_ACTION
> 
>  I don't see the purpose of the #ifdef _TYPEDEF_GACCESSIBLE_ACTION
>  guard, but if it's there it should be namespaced.
> 
>  I'd expect get_count() to be called get_n_actions() instead, in
>  GTK's own special world.
OK
>  get_description() should likely return a string by reference,
>  which means "G_CONST_RETURN gchar*" as the return type.
Right. 
>  accessible_action: the comment makes it sounds like this is purely
>  for notification. Properties shouldn't be used for notification,
>  only for conceptual "member variables." So you might just want a
>  signal, unless you can set the actions via this property.
No, these are actually used for indicating the "functional
capabilities"
of widgets.  So a dialog box might have a actions "OK", "cancel",
"apply changes", at a high level of abstraction, or a button might
have
a "click" action, toggle buttons might have "on" and "off"...and
a smart app, that uses the accessibility API, can provide some
additional information about what those actions do.
 
>  As mentioned above, should be called just "action" not
>  "accessible_action".
> 
>  Well, for that matter if it refers to the whole list of actions,
>  it should be plural, "actions"

I think the reason for namespacing this was to make sure there would
never
be a clash if the widget itself chose to implement the accessibility
methods
internally.  We'll think about it...

> gaccessibleeditabletext.h:
> 
>  Repeat first few comments for gaccessibleaction.h (copyright,
>  include guards, type macros, _TYPEDEF_ thing, Iface convention)
> 
>  The methods on this object don't take an instance as one of the
>  arguments, so they will be tricky to implement. ;-) This is true
>  of the vtable and also the wrapper functions.

Oops again.
 
>  startIndex, etc. are named in javaCaps, should be start_index
> 
>  suggest that insert_text_at_index simply be insert_text for
>  consistency with GtkEntry, GtkTextBuffer.
OK 
>  insert_text args should be "gint index, const gchar *text, gint len"
>  where len can be -1 to insert a nul-terminated string.

Why not always use null-terminated strings?
Really I don't like negative indices, they give me hives. 

>  You have cut/delete/paste, shouldn't you also have copy?

I guess so, looks like we missed it. :-)

> 
>  You can't call a method "delete" because it's a C++ keyword; suggest
>  delete_text to parallel insert_text, also consistent with
>  GtkTextBuffer.

Yep, thanks.
> gaccessibletext.h:
> 
>  Repeat first few comments as before.
> 
>  Potentially big issue here: I think GaccessibleTextDelimiter, and
>  getting words/sentences/etc. using delimiters, is fundamentally
>  broken for i18n purposes. The algorithm we use for word/sentence/etc.
>  boundaries is in pango/pango/break.c,
>  http://cvs.gnome.org/lxr/source/pango/pango/break.c, delimiters have
>  no hope of getting this right. Unless a TextDelimiter is a more
>  sophisticated object than I'm thinking, such as a semantic object
>  rather than e.g. a list of delimiter chars.

The plan was to have something pretty sophisticated, I agree with you
that
single-char delimiters don't cut it for i18n.  I will look at
pango's break.c  This is one of a few loose ends still hanging.

>  For all methods: we're trying to use the convention that "index"
>  means "bytes" and "offset"/"position" means "chars". I'm guessing
>  that chars is most appropriate here (and GtkTextView can't implement
>  this interface really unless it's chars, though I could fix that
>  fairly easily at some cost in memory usage). If it's chars,
>  should probably use "offset"

Yep, will look at the editable text interface again with the same
thing
in mind.

>  get_character_after_index: should just be get_character(), and return
>  the character at that index, i.e. first character for index 0.
>  Should return a gunichar, not a UTF-8 encoded string.

I used gchar* for consistency across all the text "get" methods.
Will reconsider.

> 
>  get_caret_index: we never call it a caret, though we aren't
>  especially consistent about it. GtkTextView variously says
>  "insert" or "cursor", GtkEntry uses "position", GtkText just
>  "point". We probably should call it a caret I guess ;-)

Yep, let'e keep caret, it's more standard in text layout
nomenclature.
 
>  get_text_range(): I'd just call it get_text()

Hmm, I was trying to keep more semantic/usage info in
the method names.  I would expect get_text() potentially to get the
whole
buffer.

>  get_row_col_at_index: this seems a bit clunky to me, GtkTextBuffer
>  uses "line" and "line_offset", I guess "column" is a bit nicer than
>  "line_offset"

I think we'd rather keep row and column. 

>  get_character_bounds should perhaps fill in a GdkRectangle, and we'd
>  usually call it get_character_extents() I think.

Extents, OK.  But we tried to avoid all explicit Gdk dependencies.
Maybe we missed one, I will grep again ;-)

>  You have a comment "why not use accessible_selection" for
>  get_selected_text, I think the reason is that GaccessibleSelection
>  is for a list of items, and is probably really inefficient for
>  characters.

Yep - except that each item might be a null-terminated string, 
and we might want non-contiguous selection of text.  I still 
think this function is redundant with GaccessibleSelection but it
probably does no harm.

>  However, I would suggest replacing this function with:
>    get_selection_bounds (GaccessibleText *text, gint *start, gint *end);
>  then people can get the selection by getting its bounds, then
>  calling get_text(). Also, I'd replace
>    get_selection_end()/get_selection_start() with the single
>    get_selection_bounds() method.

We'd like to keep the first one atomic.  Your second point is a 
good one, we'll discuss.

>  Aren't you missing a function to get the current number of characters
>  in the buffer?

I'm not sure how useful that is for us, it may have little to do
with what's shown on-screen.  We can traverse to the end of the
buffer with the methods we have already.

>  For all these methods, -1 should be allowed to indicate "last
>  character in the buffer", e.g. get_text (text, 0, -1) returns all
>  text in the buffer.

Aaaauuuugghhhh ....
ack 
ack

can we pleeze just do get_n_chars instead ;-)
 
>  Same comments as before on the accessible_text and accessible_caret
>  properties; remove accessible_, and they should only be properties if
>  you can set them.  (Setting these two seems reasonable, though.)

In this case I think you're right, they should be signals instead.
 
> gaccessiblehypertext.h:
> 
>  Same first few comments as with the other headers.
> 
>  Whatever GaccessibleHyperLink turns out to be, it'll probably be
>  a struct or object, so passed around as a pointer.
> 
>  I'm assuming GaccessibleHyperLink will have a way to get the name,
>  target, and other info about the link.

Yep

>  Conceivably in C it's easier to have methods on GaccessibleHypertext
>  that get that info, instead of fooling with a tiny
>  GaccessibleHyperLink object that will get memory-leaked all the time.

I'll have to discuss this with the folks who designed the Java 
one, to see how the AT vendors are using it.  

>  Should probably be Hypertext and Hyperlink or HyperText and
>  HyperLink, rather than a mixture.
Yep.

> gaccessibleicon.h:
> 
>  Same first few comments, blah blah.
> 
>  G_CONST_RETURN on get_icon_description I think, if you are reasonably
>  sure there will be a description stored somewhere to return a
>  reference to; else the string would be copied so not const.

Yep, we should change seceral of these gchar* methods
to use G_CONST_RETURN.

>  Most of the other interfaces seem to lack "setters", such as
>  set_icon_description(); why does this one have a setter while there's
>  e.g. no set_hyperlinks(), set_actions(), and that kind of stuff?

Implementation.  In a lot of cases it's difficult to implement and
may not be necessary, we are including the ones that experience has
shown assistive technologies will need.

>  set_icon_description(): the gchar* should be const.
> 
>  get_icon_width(), get_icon_height(): our usual convention would be:
>    get_icon_size (GaccessibleIcon *icon, gint *width, gint *height);
> 
> gaccessibleselection.h:
> 
>  Same first few comments.
> 
>  The methods are named incorrectly I think, add_selection() sounds
>  like it adds a selection, but you are adding _to_ the selection.

Don't see the problem, you add a (partial) selection to an additional 
selection.  Could use append/prepend but they are ugly.  If anyone
else objects we can try add_to_selection()...or maybe
just selection->add()

>  It seems odd to me to write selection->add_selection().
>  Maybe selection->add_item(), selection->clear(),
>  selection->get_item(), selection->get_n_selected(),
>  selection->item_is_selected(), selection->remove_item(),
>  selection->select_all().
> 
>  As you note in a comment, convenience wrappers missing.
> 
> gaccessibletable.h:
> 
>  (Same first few comments, I'll stop saying this now I guess ;-)
> 
>  I believe this class is to access what we'd call a "sheet", that is,
>  a spreadsheet-like widget, while GtkTable is a container widget used
>  to line up widgets in dialogs and such. So conceivably it should be
>  renamed to GaccessibleSheet, though I'm not sure it's a big deal.  If
>  it's also used for say GtkTreeView then I could very well be on
>  crack.

Yep, it'll be used for GtkTreeView also ;-)
Maybe that explains some of the other apparent oddities.
 
>  For all methods, "r" and "c" should be spelled out "row" and "column"

OK

>  I'd change ref_at() to get_item() or get_cell() and use the same name
>  (item, cell) for the other methods also when appropriate.

Gotta be ref, 'cause it's flyweight.

>  I don't understand the get_index_at, get_row_at_index methods. Why
>  have flat index space in addition to the grid indices? Do these map
>  to Gaccessible indices? (It's dawning on me that maybe the purpose of
>  Gaccessible having children is to do these "compound" accessible
>  objects, such as a table with cells - is that right? If so and we
>  revise Gaccessible as I suggested to not use indices, these methods
>  would become I guess get_position (table, Gaccessible *child, gint
>  *row, gint *column)

Indices have proved to be important in the past to ATs.  I found the
flat index space really weird at first also, especially in a 
spreadsheet, but I am assured that it's "really" what the AT folks
want.

>  get_column_count should maybe be get_n_columns()
> 
>  ditto for get_row_count
OK 
>  What does get_row_extent_at() do?

For cells that span multiple rows/columns you need these methods.
HTML tables are the driving force behind a lot of the eccentricity
here.

>  I don't understand the get_column_header() method, why does
>  it return another table?

Again this one is an HTML table thing.  I agree that it looks weird,
we will confer on whether it's absolutely necessary.
 
>  For get_selected_rows() (and get_selected_columns()) we might
>  consider:
>    void get_selected_rows (GAccessibleTable *table,
>                            gint            **rows,
>                            gint             *n_rows);
> 
>  The methods starting with is_ should probably have the is_ moved
>  just before the participle, that is, column_is_selected().

We use the convention that all methods returning boolean begin with
"is", just as methods that return persistent object or properties
begin with "get" and setters begin with "set".

>  The wrapper functions are missing the "g" at the front, they are just
>  "accessible_table_ref_at()" etc.

oops again.

>   This interface again has "setters", what are those used for? (I can
>   imagine an accessibility app setting the text in a cell I guess, but
>   why would a user change say a column title? Or, even if there are
>   cases where they would, don't we need a mechanism to indicate
>   whether or not they can? Since most column titles won't be
>   modifiable by the user.)

In most cases the setters are for the application, not the user.
Otherwise an app that uses some nifty icon for a column title will
have trouble providing an alternate description of the column.
 
> gaccessiblevalue.h:
> 
>  "Value" seems a bit vague, maybe "GaccessibleNumber"?
> 
>  s/accessible_value/value/

Really I can't see

gaccessible_accessible_value_get_current_value(...)

surely this isn't what you mean ??

> 
> gtkaccessiblefactory.h:
> 
>  My only question here is what the factory is for - it doesn't look
>  like it does anything. ;-)

The factory creates accessibles.  That's the hook we are using to
get Gaccessible instances for the GtkWidgets, and how
we can add the implementation after GTK+-2.0 is frozen.  We register
a factory instance with a widget class (and its subclasses, unless
overridden) and that factory instance create the widget's Gaccessible
when gtk_widget_get_accessible(...) is called.

> gtkaccessible.h:
> 
>  Doesn't this need get_widget() and set_widget() methods? (they don't
>  need to be in the vtable, just "final" methods that can't be
>  overridden).

We don't want to expose set_widget, and for get_widget we are
currently
just using the widget member.  Should only be used by the
implementation
internals, probably.

We can add gaccessible_get_widget() easily enough, maybe it will be
useful.
 
> GtkWidget changes:
> 
>  Suggest storing the GtkAccessible in object data instead of
>  in an instance member, so no space is used unless you're using
>  the accessibility feature.

The accessibility feature will always be used, by every widget (except
maybe some non-visible widgets, as previously described).
However the heavyweight part of the implementation will not always be
present.  Again, we really need this to be highly visible and
exposed, which is not the case with object data.
 
>  There's no reason to put GtkAccessibleFactory in the class struct;
>  for what Java/C++ would call a class/static variable, we just use
>  static globals in the .c file.

Not sure this will work with subclasses... the factory instance
needs to be both inheritable and overridable.  

> 
>  Since you make GtkWidget implement the GaccessibleInterface which has
>  the get_accessible() method, I don't think it's right to introduce
>  another get_accessible() method in GtkWidget, or the wrapper
>  function gtk_widget_get_accessible().

See above.  We don't want to deal with refs all the time in the 
GtkWidget-specific code, but can't rely on persistent objects from
other implementors of GaccessibleIface.  On the other hand we want
GtkWidget to implement GaccessibleIface so that in cases where an
agnostic client wants to connect to "accessibles" it can use the
same interface for GtkWidget as for other implementors.

>  gtk_widget_class_set_accessible_factory() could just be
>  gtk_widget_set_accessible_factory (GtkAccessibleFactory *factory),
>  which sets the global variable. This is the same as a static C++
>  function, i.e. I'd call it as Gtk::Widget::set_accessible_factory
>  (factory), with no instance (the class is just a namespace here).
> 
> gtkaccessiblecomponent.h:
> 
>  I could use some docs on what this object represents and when/how
>  it's used.

This is the subinterface that represents the "onscreen GUI component" 
aspect of an accessible.  It's needed in particular by screen
magnifiers.
 
>  Remember to remove inclusion of gtk/gtkdata.h, GtkData is a useless
>  object I've been meaning to delete.

Yep, thanks for catching that.

>  'typedef gint GaccessibleCursorType' isn't something we'd normally
>  use - it should be an enum or an int, not an enum-looking int. ;-)

Right, should be enum.

>  contains() is just syntactic sugar for getting the extents and seeing
>  if the point is in the extents, so there's no reason to virtualize
>  it; just write a wrapper function that calls get_extents()
>  then checks if it's in the extents.

We want to be able to handle that cases where the bounds are
not rectangular.
 
>  get_bounds() would be get_extents() using our conventions.
OK
>  get_accessible_at() dangles an ambiguous preposition, suggest
>  get_accessible_at_point().
OK 
>  get_location() is get_position() with our conventions.
> 
>  get_location_on_screen() would return the position in root window
>  coordinates. Suggest maybe calling the parameters root_x root_y to
>  clarify that.
OK
>  If set_cursor_type() changes the mouse pointer, it's not
>  implementable without some larger GTK changes that I can describe if
>  you want, I was trying to implement gtk_widget_set_cursor() the other
>  day. If it does something else, probably you should explain it so
>  we can figure out how to implement.

I'll talk to some folks who have worked with AT implementors. 
Apparently 
this is something they need, probably for screen magnifiers and low
vision situations.  Maybe we can figure something out...

>  set_enabled() would be set_sensitive() in GTK terms, and has the
>  prototype for get_enabled(). Maybe you mean get_enabled()?
>  If not then get_enabled() seems to be missing.

Prototype is wrong.  This is a messy one, you can do the
get() via the GaccessibleState, but for implementation reasons there
can't be a set_state().  I will confer as to why set_enabled() is on
our requirements list.
 
>  set_location() should be set_position(), but can't be implemented for
>  most GtkWidgets, since they are inside containers that determine
>  their position. So at least there needs to be a way to query whether
>  this operation is possible?

OK, I guess a boolean return... will check on degree of need.

>  For set_visible(), the arg should be named "setting" or "visibility"
>  instead of "b"; also, again not sure this makes sense to implement.
>  Why would users hide a widget? Probably yet another case of me not
>  knowing what the method is intended to do. ;-)
> 
>  We'd call it grab_focus() not request_focus() in GTK
OK 
>  add_focus_listener() is a Java-ism, we'd just have a signal to
>  connect to.

I disagree, we need syntactic sugar here.  Don't worry, it
will be harmless :-)

>  component_contains() should take x, y, not a GdkPoint.
Oops.
>  ditto for component_get_accessible_at()
> 
>  You seem to have a set_bounds() wrapper but no set_bounds() in the
>  vtable.
> 
>  We'd call it set_extents() not set bounds.

Again maybe a clash of implementability vs. requirements. 
Maybe another boolean return ;-)

>  As with set_position(), not sure how we'd implement this for widgets
>  in containers.
> 
> OK, I'm tired of typing, you're tired of reading. ;-) Hopefully the
> above is constructive. When I'm not understanding how the API is used,
> please point that out.
> 
> Thanks for working on this stuff, should be a nice addition to GTK.

Thanks for all the time to review it.  I'll try to take your
comments on board in the next couple of days and post a new tarball,
maybe to the Gnome Accessibility Project pages.  

Also we have decided for extensibility's sake to virtualize all but
one of the Gaccessible methods, that will be a minor change.  Lastly I
need to complete the wrappers for all the subinterface methods.  I
am not sure I'll have time to do this next week, as we have a 
demo soon after... but we feel pretty confident about completion of
the 
API and wrappers before GUADEC.


Best regards,

Bill

> Havoc
> 
> 
> 
> 
> _______________________________________________
> gtk-devel-list mailing list
> gtk-devel-list gnome org
> http://mail.gnome.org/mailman/listinfo/gtk-devel-list

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