Created attachment 74943 [details] [review]
patch which adds the gnome-canvas a11y code formerly in gail to libgnomecanvas

Thanks for providing this patch. I'm going to take a detailed look at it on the plane tomorrow.

Hey Bill,

  these files look like they have been just moved from gail (which isn't a problem as I would have started the same way). But I think you agree that the code in Gnome should always be in a shape as good as possible. Especially this code (which is likely to be copied by me into my cairo based canvas) should be reusable as easy as possible and to achieve this (along with some more consistency with the rest of the stack) I'd like to address some glitches with the current code:

1. Please use G_DEFINE_TYPE() or G_DEFINE_TYPE_WITH_CODE() to define GObject types. This makes the current code easier, will give you »parent_class« features for free and is less likely to contain errors (such a macro might also be updated by the glib maintainers and GNOME canvas would get these updates for free then).

2. Please use G_BEGIN_DECLS and G_END_DECLS in the header files (might require additional including of <glib/gmacros.h>) to be more consistent with the rest of the platform.

3. Please don't use statements like this:
> typedef struct _GailCanvas GailCanvas;
...
> struct _GailCanvas
> {
>   GailWidget parent;
> };
A simpler solution would be this:
> typedef GailWidget GailCanvas;
(which would also apply to ...Class as well)

4. Can you please add documentation to the functions that are accessible (like gail_canvas_item_new()), this would make both the review and the quality of the code better.

5. As this code doesn't seem to be public API, can you please add a file that explains both the purpose of the code (which is quite obvious) and how this code is supposed to be used by applications?

(I'm not yet finished reading your patch, so maybe skip 5. - if already done - but at least be prepared for more to come - even though I doubt it)

There was just one more:

6. Please don't export instance members in the header files (like in the following snippet):
> struct _GailCanvasText
> {
>   GailCanvasItem parent;
>   GailTextUtil *textutil;
> };

Please use a private structure in the C-File. If textutil needs to be accessed from other pieces than gailcanvastext.c, please provide an API for working with this member.

Thanks.

Hi Sven;

A couple of comments:

1) the use of internal struct members was common in gtk+ when these Gail classes were originally written.  Changing that now may do considerable violence to the gail code, though we could peck away at it class-by-class.

2) gailwidget.h needs to be exported for this patch to work, and gailwidget's struct definitely requires internal members that are directly accessed.  Fixing that is too big a diff for gail to be practical.

We have never been asked to document internal-only methods, in particular constructors like gail_canvas_item_new() which are NEVER called outside of the library.  I don't think it's really appropriate since the docs would just say "do not call this method, it is an implementation detail required by GailCanvas".

I disagree about using typedef instead of ...
> struct _GailCanvas
> {
>   GailWidget parent;
> };
since the above is a pattern which is consistently repeated throughout Gail (sometimes with added structs, sometimes not).  Since we don't guarantee ABI stability of GailCanvas, new internal structs could be added at any time, which would invalidate the typedef.

best regards

Bill

to follow up point-by-point:

#1 and #2 are OK by me (though personally I find the macros obfuscating); #3 is not; I don't see the value in #4; #5 and #6 are OK.

OK; revisiting;

#1: existing gnome-canvas code doesn't use these macros.  I think that 
    it makes more sense for you to add them if/when you move this code to the new   
    cairo canvas.
#2: sure, OK, though these macros didn't exist when gail was written either.
#3: patch is consistent with existing gnome-canvas code 
     (see GnomeCanvasLineClass in gnome-canvas-line.h)
#4: methods are used only by the factory instances.
#5: there's no public API here, I guess we need to say that?
#6: Not sure how to handle this one most elegantly. Making a new priv struct to hold this single member seems like overkill to me.

Created attachment 76554 [details] [review]
patch incorporating suggestions

> I disagree about using typedef instead of ...
> > struct _GailCanvas
> > {
> >   GailWidget parent;
> > };
> since the above is a pattern which is consistently repeated throughout Gail
> (sometimes with added structs, sometimes not).  Since we don't guarantee ABI
> stability of GailCanvas, new internal structs could be added at any time, which
> would invalidate the typedef.

Good point. Convinced.

And for point 4, can you at least tell me how this code is supposed to work? As no application is supposed to use that code, how is it used? (I'm just not familiar with gail and I just like to get some information about it.)

Now commenting on the new patch. It would be great if you could use G_DEFINE_TYPE() (former point #1), then it looks okay (except for a missing dependency on gail in the .pc.in file).

Hi Sven:

I'll add some docs to the exported methods (at least the key ones) to give at least a hint how the system works (with pointers to the atk docs if I can figure out how to include non-volatile references to other modules' api docs)

As for G_DEFINE_TYPE, I notice that none of the other classes in libgnomecanvas use these macros, so the existing patch is consistent with the rest of libgnomecanvas.  Since libgnomecanvas is expected to be supplanted by a new canvas Real Soon Now, perhaps it doesn't make sense to add these macros now.  Besides, for technical reasons we cannot use G_DEFINE_TYPE or G_DEFINE_TYPE_WITH_CODE in gailcanvas.c (though we could potentially use it for the other classes).

I believe the gail dependency is gone from the new patch; I will double-check.  

> Since libgnomecanvas is expected to be supplanted by a new canvas Real Soon
> Now, perhaps it doesn't make sense to add these macros now.

That was exactly the reason why I requested using these macros. Once the new canvas is there, we could easily copy the files from gnome canvas into the new canvas (but, I'm fine if you don't agree on this one).

Small wrap up:
1. Do you think it makes sense to use G_DEFINE_TYPE() where possible?
2. Can you please add a small piece of documentation on how this code is supposed
   to be used? (eg. gailcanvasfactory)
3. Please check for the dependecy on gail and either remove it or also add it to
   the .pc.in file (as it doesn't appear in public headers it should go into the
   private requirements field).
4. Please also add the gail* files to the doc/reference/Makefile.am to prevent
   gtk-doc from scanning these files.

Hi Sven: Thanks for feedback.  I'll make the suggested changes above and revise the patch - could take me about a week though as I'm travelling. - Bill

Bill, any news on this?

On holiday, virtually no internet access ATM.  however you're welcome to make the changes to my patch yourself if you prefer - otherwise I'll try to revise 2nd wk in Jan.

Created attachment 80329 [details] [review]
patch that incorporates all suggestions

Need assistance figuring out why gtk-doc scan recurses forever with the new patch.  It seemed to be working fine until the last 'make clean'; 'make' cycle.  ?

Created attachment 80501 [details] [review]
fixed patch incorporating all suggestions plus ChangeLog

I found the bug that was causing gtk-doc-scan issues.  Sven, does the attached patch look OK to you?
I did not document anything that was explicitly excluded in the gtk-doc-scan headers, since this is private API.  A few symbols are exported for purely technical reasons, as they always were in the previous gail implementation.

I think this patch should coexist reasonably with legacy gail, i.e. the factories for gnomecanvas as defined by gail should be replaced by the libgnomecanvas ones transparently.  If this doesn't work as expected, we can either change the namespace of the methods or remove the gnomecanvas code from gail and release the two new tarballs in sync (so libgnomecanvas HEAD would require gail HEAD).  If we don't have to do the latter, it's better IMO even though some loaders may complain about duplicate symbols.

Sven: ping?
(BTW my above notes about duplicate symbols are theoretical, I haven't observed any problems on my system).

Herzi, ping?

Sorry, will look at the patch on the way to my parents.

Hi Sven, we are looking forward to move gail into gtk+ and libgnomecanvas as soon as possible. And the gtk+ is blocked by libgnomecanvas. Can you take a look at the patch when you get time?

Sorry, for waiting so long. I just forgot about this patch. It's committed to trunk now.

It looks like the dependency on gail was not so clever right now. Please remove the canvas stuff from gail-TRUNK and also the canvas dependency from gail so we don't get a circular dependency by having the canvas depend on gail and gail on the canvas.

I'm reverting the commit now and will commit it back after gail is fixed (and please also change the dependencies for gail in jhbuild).

OK, working on the patch. Maybe we need the make a little change. We usually use gail.c to handle the focus event. Since gail will not depend on gnomecanvas, we have to do this in gailcanvas.c. Working on the patch.

Created attachment 89047 [details] [review]
patch to fix focused_item problem

Gail need to get gnomecanvas->focused_item when focus changed. Since gail will not depand on libgnomecanvas, we can not use gnomecanvas->focused_item directly. This patch make gail can get the focused_item by g_object_get_data.

If the patch is OK, I will remove the libgnomecanvas dependency as soon as possible. (our patch is ready)

Hi Sven, is the patch OK? If it is OK I think we still can catch up with gnome 2.19.3, I will commit gail patch first.

2007-06-04  Sven Herzberg  <herzi@gnome-de.org>

        Patch from Li Yuan:

        * libgnomecanvas/gnome-canvas.c: (gnome_canvas_item_grab_focus):
        store the focused item in the "focused_item" object data of the canvas
        (required for gail to drop the dependency on libgnomecanvas)

Will you release libgnomecanvas tonight for gnome 2.19.3? If you will, I think I can apply the gail part of patch and you apply the gnomecanvas part. What do you think?

It's too late here... I'd like to release tarball first and will commit the patch tomorrow.

I have committed the gail part of patch. And filed a bug for jhbuild: http://bugzilla.gnome.org/show_bug.cgi?id=444257

OK, the patches has been committed. You can commit your patch now.

Hi Sven, I guess it is the time to commit libgnomecanvas part of patch?

Sven, ping...

I can't release tarballs (I have no ssh-account on master). Maybe kjartan can...

It's OK. Can you commit the patch first?

Hi Sven, it would be great if you commit the patch. Because I have commit the gail part of patch for several weeks. People who build gail from trunk may miss some functions.

Shouldn't the file

/libgnomecanvas/libgnomecanvas-2.0.pc.in

contain

Requires.private: pango pangoft2 gail

instead of:

Requires.private: pango pangoft2 gail-1.0

??
Gail-1.19.5 installs only gail.pc.

(In reply to comment #38)
> Shouldn't the file
> 
> /libgnomecanvas/libgnomecanvas-2.0.pc.in
> 
> contain
> 
> Requires.private: pango pangoft2 gail
> 
> instead of:
> 
> Requires.private: pango pangoft2 gail-1.0
> 
> ??
> Gail-1.19.5 installs only gail.pc.
> 

This was bug 455605.

Not sure whether bug #456513 indicates a problem with the added gail support, or is a problem that this new gail stuff fixed, or whether 2.19.1 just caught the gail migration at a bad time.

Created attachment 92325 [details] [review]
patch to add a new property

Just realized that all applications can modify gnome_canvas->focused_item. So we can not get the right focused_item just by g_object_set_data/g_object_get_data. I think install a new property is a good idea. This is a change of API and the API freeze will be on July 30,. Herzi, can you help me to review the patch? It is really urgent because all object in gnome_canvas can not be accessible now.

I don't see how this patch solves any problem. Can you add some more explanations? (also it would be great if these things get reported as new bugs - as gailcanvas is merged into gnome canvas).

Applications like Evolution use gnome_canvas and set gnome_canvas->focused_item (e-canvas.c:e_canvas_item_grab_focus).
We can not add g_object_set_data to every application which set focused_item. So sometime gail can not get the right focused_item by calling g_object_get_data(because we only set data in gnomecanvas now). A good solution is property. So gail can get focused_item by calling g_object_get_property which fetches focused_item directly from gnomecanvas.

I will file a new bug next time. Do you want a new bug for this patch?

Herzi, any comments on the patch?

2007-07-26  Sven Herzberg  <herzi@gnome-de.org>

        * libgnomecanvas/gnome-canvas.c: don't store the focused item in the
        data section of the canvas widget, but in a GObject property of it
        (Patch by Li Yuan, fixes 363103)

I can make a tarball if that's wanted at this time?

(In reply to comment #46)
> I can make a tarball if that's wanted at this time?

Go ahead if you feel like it :-)

Thank you very much Herzi and Kjartan :)