Re: GtkAction, Plugins and PyGtk

[Hans Breuer <hans breuer org>]

Hi Robert,
it seems as if again I wont find the time this weekend to apply your
patch to my local tree. So here is some feedback from reading your patch.

On 07.05.2006 23:07, Robert Staudinger wrote:
> On 5/4/06, Robert Staudinger <robert staudinger gmail com> wrote:
> > On 5/3/06, Hans Breuer <hans breuer org> wrote:
> > > On 03.05.2006 21:22, Robert Staudinger wrote:
> [...]
> > > So IMO the way to go would be to change the 
> > filter_register_callback() to
> > > something more appropriate to action based menus and change some few
> > > plug-ins. One of the problems action based menus are supposed to fix
> > > cleanly is http://bugzilla.gnome.org/show_bug.cgi?id=171397
> > >
> > > Having a proxy action seems to be a small price to pay ...
> >
> > Ok, i will look into that.
>
> A new patch is available now [1] which fixes the plugin issue by
> introducing a new call:
> dia.register_action (
>    action, # string
>    label, # string
>    path, # string
>    callback) # callable
>
> which is an enhanced version of dia.register_callback.
Looks fine. Probably we should just drop the old signature
and thus break the (few?) plug-ins not living in Dia's tree.

> If you could look at the patch that'd be great. Some feedback before
> tearing things apart when moving the diagram window to use
> GtkUIManager would be nice.
Some general remarks, more detailed patch review following.

- Apparently you are using a tab width of 4 while 8 is commonly used
- Maybe you could do the new development in some new files like
  app/dia-actions.[hc] to improve the readability of the patches
  and finally have better separation of functionalities than before?

RCS file: /cvs/gnome/dia/app/dia_embedd.c,v
retrieving revision 1.14
diff -a -u -p -r1.14 dia_embedd.c
--- dia/app/dia_embedd.c        17 Nov 2003 21:41:24 -0000      1.14
+++ dia/app/dia_embedd.c        7 May 2006 20:54:11 -0000

...

+/* TODO is this needed, what for?
   The hiding of some of our menu entries is necessary to
   adapt to the 'run-embeded' case, e.g. no File/(New|Open|Exit)
   because these would conflict with the embedder's menu entries.

But maybe we should just remove app/dia_embed.c

--- dia/lib/filter.h    4 Dec 2004 23:53:21 -0000       1.10
+++ dia/lib/filter.h    7 May 2006 20:54:15 -0000
[...]

If you change the API between plug-ins and core you should also
increase DIA_PLUGIN_API_VERSION in lib/plug-ins.h to avoid
crashing on older plug-ins possibly laying around.


--- dia-orig/data/dia-ui.xml    1970-01-01 01:00:00.000000000 +0100
+++ dia/data/dia-ui.xml 2006-05-07 19:10:25.000000000 +0200

To me a different naming conventions for actions looks more
natural, e.g. similar to what's done in The GIMP.
Instead of CamelCase ("FileNew") could we use all lower
as in "file-new" ? [Note: I don't know exactly the difference
between 'action' and 'name'. The GIMP people - who usually do
give good examples to follow - are using camel case for 'name'
but as described for 'action'.]


Thanks,
        Hans

-------- Hans "at" Breuer "dot" Org -----------
Tell me what you need, and I'll tell you how to
get along without it.                -- Dilbert