See bug 639486 from which this bug was spawned for additional discussion.

[Mass-reassigning open atk bug reports for better trackability as requested in https://bugzilla.gnome.org/show_bug.cgi?id=653179 .
PLEASE NOTE:
If you have watched the previous assignee of this bug report as a workaround for actually getting notified of changes in atk bugs, you yourself will now have to add atk-maint@gnome.bugs to your watchlist at the bottom of https://bugzilla.gnome.org/userprefs.cgi?tab=email to keep watching atk bug reports in GNOME Bugzilla.
Sorry for the noise: Feel free to filter for this comment in order to mass-delete the triggered bugmail.]

Staking a claim on this one because I desperately want it done. :-)

Created attachment 241677 [details] [review]
Proposed patch.

I have made a pass at a patch.

Review of attachment 241677 [details] [review]:

A recent discussion on IRC bring this interface back, so I made a review of the patch.

In general looks good, but I have some comments/questions.

::: atk/atktablecell.c
@@ +46,3 @@
+ */
+gint
+atk_table_cell_get_column_extent (AtkTableCell *cell)

Joanmarie suggested that in most places, the terminology for this is not "extent" but "spans". In any case I will use my "non-native english speaker" card and let you both decide which is the best name for this.

@@ +64,3 @@
+ * @cell: a GObject instance that implements AtkTableCellIface
+ *
+ * Returns the column headers as an array of cell accessibles.

What is an array of column header cells? Or using different words, what is an "header cell"? Right now, we are saying that a cell is the accessible that you can find at a (row, column) position inside a table. This method seems to suggest that there are other kind of cells, but it is not clear (at leas to me).

Looking at IA2 (this cell interface is being based on that), it seems that IAccessibleTable2 doesn't have an equivalent to IAccessibleTable::columnheader, and the new functionality was moved to the cell. Does that mean that any cell should be able to provide the column header? Can two different cells from a given column return a different set of header cells?

@@ +88,3 @@
+ * @cell: a GObject instance that implements AtkTableCellIface
+ *
+ * Translates this cell accessible into the corresponding column index.

Do we really need one method to retrieve the column index and other method to retrieve the row index? I see really likely that you would be interested on both. What about a method to retrieve both at the same time?

@@ +108,3 @@
+
+/**
+ * atk_table_cell_get_row_extent:

See my previous comment about spans vs extent.

@@ +135,3 @@
+ * @cell: a GObject instance that implements AtkTableCellIface
+ *
+ * Returns the row headers as an array of cell accessibles.

See my previous comment about column header cells.

@@ +159,3 @@
+ * @cell: a GObject instance that implements AtkTableCellIface
+ *
+ * Translates this cell accessible into the corresponding row index.

See my previous comment about column index.

@@ +182,3 @@
+ * @cell: a GObject instance that implements AtkTableCellIface
+ *
+ * Returns a boolean value indicating whether this cell is selected.

Is this method really needed? We already have the state ATK_STATE_SELECTED

And a small question, as I'm not sure: if in the end this method makes sense, should we deprecate atk_table_is_selected()?

@@ +216,3 @@
+ */
+gboolean
+atk_table_cell_get_row_column_extents (AtkTableCell *cell,

See my previous comment about spans vs extents.

@@ +285,3 @@
+  *row_extents = atk_table_cell_get_row_extent (cell);
+  *column_extents = atk_table_cell_get_column_extent (cell);
+  *is_selected = atk_table_cell_is_selected (cell);

Not sure if we should provide a default implementation for this method. In any case, if we do, I think that we should mention that on the documentaion, so ATK implementor would be aware of it.

::: docs/atk-sections.txt
@@ +410,3 @@
+</SECTION>
+
+<SECTION>

This is not enough to bring AtkTableCell to Atk documentation. In any case, I have a patch that updates this and adds more documentation, that I will upload soon as independent patch.

Created attachment 268695 [details] [review]
Improve documentation for AtkTable/AtkTableCell

As mentioned on my previous review, AtkTableCell was not properly added to Atk documentation. This patch does that, adds more documentation for AtkTableCell and deprecate the index-based methods (that was also needed without AtkTableCell, but became more true after their addition).

(In reply to comment #5)

> What is an array of column header cells? Or using different words, what is an
> "header cell"? 

In the case of HTML, it's the thing marked up with <th> in a table row.

> Right now, we are saying that a cell is the accessible that you
> can find at a (row, column) position inside a table. This method seems to
> suggest that there are other kind of cells,

Indeed there are. :) Basically, given a cell at (row, column), we want to be able to get the titles/headers/typically-bold-text that describe that cell.

> Does that mean that any cell
> should be able to provide the column header?

Yes.

> Can two different cells from a
> given column return a different set of header cells?

Unlikely, but not unheard of. Consider a spreadsheet with two data areas. A spreadsheet, accessibility-wize, is one ginormous accessible table. But I've seen multiple functional tables on a single spreadsheet.

Created attachment 269354 [details] [review]
Revised patch.LECTED

Thanks for the review.

I guess you're right about the selected function being redundant with ATK_STATE_SELECTED, so I removed it.

extent -> span.

Also removed the doc changes so I don't conflict with your patch.

Review of attachment 269354 [details] [review]:

Looks better, but this seems like a half-made patch. Are you sure that this is the final patch? Look my following comment.

::: atk/atktablecell.h
@@ +67,3 @@
+gint        atk_table_cell_get_column_span       (AtkTableCell *cell);
+GPtrArray * atk_table_cell_get_column_header_cells (AtkTableCell *cell);
+gboolean     atk_table_cell_get_position           (AtkTableCell *cell);

This atk_table_cell_get_position seems a method to handle both get_column_index and get_row_index with one method (as I questioned on comment 5). But there is no get_position virtual method at the interface definition, atk_table_cell_get_position is not implemented at atktablecell.c, and in the same way, atk_table_cell_get_column_index and atk_table_cell_get_row_index (and their virtuals) are still present.

Created attachment 269425 [details] [review]
Revised patch.

Yeah, that patch was half-done. I'm surprised that it even compiled...

Review of attachment 269425 [details] [review]:

Looks good to me. Just a comment, but you are free to commit anyway.

::: atk/atktablecell.h
@@ +1,2 @@
+/* ATK -  Accessibility Toolkit
+ * Copyright 2001 Sun Microsystems Inc.

This copyright is different to the one used at atktablecell.c. Was that made on purpose? When you submitted the patch for AtkWindow, both header and source file has the same copyright assignment.