Re: GTK+ Accessibility Proposed API

["Padraig O'Briain" <Padraig Obriain Sun COM>]

Havoc's comments address three separate areas:

1) Namespace issues for Accessibility code
2) Issues with integration of Accessibility code with GtkWidget
3) issues with the propsoed Accessibility API.

I would like to address only the first issue and make a revised proposal which 
is below. I would like to get agreement on this quickly so we can move on to the 
other issues. Can I assume that that the proposal is accepted if I do not get 
objections by close of business on Tuesday?

Padraig


There will be a project called ATK (Accessibility Toolkit) and thus a
directory atk at the same level of glib, pango and gtk+. This project will,
when installed, put include files in the directory $installdir/include/atk and a 
library libatk.so in $installdir/lib.

This project will be a depenency for gtk in the same way that glib and pango
are, so building this project will be necessary before building gtk+.

The source files for this project will be:

atkobject.c|h (used to be gaccessible.c|h)
atk<InterfaceName>.c|h, e.g. atkcomponent.c|h (used to be 
gaccessiblecomponent.c|h)

The includes of "gtk/a11y/...> will be replaced by the one include of
<atk/atk.h>

The actual implementations of the accessible objects for the GTK widgets
will be in a project called AGTK (Accessibility for GTK); there has been a 
proposal for GAEL (GTK+ Accessibility Enabling Library) but I am not sure 
whether we should aim for cleverness over boring. This project will
produce a shared library libagtk.so which will be a GTK module. This module
will be loaded if one wishes to enable Accessibility for a GTK+ application.


> X-Unix-From: hp redhat com  Sat Mar 10 04:35:20 2001
> Delivered-To: gtk-devel-list gnome org
> X-Authentication-Warning: icon.labs.redhat.com: hp set sender to hp redhat com 
using -f
> To: Bill Haneman <Bill Haneman Sun COM>
> Cc: gtk-devel-list gnome org
> Subject: Re: GTK+ Accessibility Proposed API
> User-Agent: Gnus/5.0807 (Gnus v5.8.7) Emacs/20.7
> MIME-Version: 1.0
> X-BeenThere: gtk-devel-list gnome org
> X-Loop: gtk-devel-list gnome org
> X-Mailman-Version: 2.0beta5
> List-Id: Development of GTK+ <gtk-devel-list.gnome.org>
> 
> 
> 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
> 
> 
>  
>  
> 
> _______________________________________________
> gtk-devel-list mailing list
> gtk-devel-list gnome org
> http://mail.gnome.org/mailman/listinfo/gtk-devel-list