Re: GTK+ Accessibility Proposed API

[Havoc Pennington <hp redhat com>]

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

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

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.

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.

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.

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.

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

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

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

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

 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.

 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?

 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.

 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?

 NUM_POSSIBLE_STATES is not namespaced.

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

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

 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. 

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

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

 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.

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

 Even if we keep the index-based API, suggest removing child_index
 from the signal, people can call get_index_in_parent.

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

 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.

 Note that non-GObject can implement interfaces, so the ref_accessible
 method should take a GaccessibleAccessible, not a GObject.

 Again I'd make it get_accessible() and not ref the return.
  
 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.

 gaccessible_get_index_in_parent(): the function pointer in the vtable
 takes a guint argument, this function does not.

 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?

 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.

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

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

 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.

 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.

 gaccessible_relation_new() should then take a GaccessibleRelationship
 as an arg, not guint.

 It isn't obvious to me what state_mask_get/set_name() are used for.

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.

 get_description() should likely return a string by reference,
 which means "G_CONST_RETURN gchar*" as the return type.

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

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.

 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.
 
 insert_text args should be "gint index, const gchar *text, gint len"
 where len can be -1 to insert a nul-terminated string.

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

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

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.

 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"

 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.

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

 get_text_range(): I'd just call it get_text()

 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"

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

 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.

 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.

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

 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.

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

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.

 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.

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

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.

 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?

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

 For all methods, "r" and "c" should be spelled out "row" and "column"

 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.

 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) 

 get_column_count should maybe be get_n_columns()

 ditto for get_row_count

 What does get_row_extent_at() do?

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

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

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

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

gaccessiblevalue.h:
 
 "Value" seems a bit vague, maybe "GaccessibleNumber"?

 s/accessible_value/value/

gtkaccessiblefactory.h:

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

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

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.

 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.

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

 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.

 Remember to remove inclusion of gtk/gtkdata.h, GtkData is a useless
 object I've been meaning to delete.

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

 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.

 get_bounds() would be get_extents() using our conventions.

 get_accessible_at() dangles an ambiguous preposition, suggest
 get_accessible_at_point().

 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.

 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.

 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.

 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? 

 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

 add_focus_listener() is a Java-ism, we'd just have a signal to
 connect to.

 component_contains() should take x, y, not a GdkPoint.

 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.

 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.

Havoc