Re: [evolution-patches] gal patch - for 44222

[Mike Kestner <mkestner ximian com>]

On Thu, 2003-09-25 at 19:04, Suresh Chandrasekharan wrote:

> 	Attached pl. find a patch for "44222 task summary entry widget not 
> i18ned". Not only the now the these widgets can accept i18ned input, but 
> copy/paste also works fine. Pl. review it and lemme know.
> 	
> 	This one is created against evolution-1-4-branch of gal.

Thanks for the patch, and thanks for your patience with the slow review.

One general comment up front, please review the patch guidelines and
coding standards.  There are several places in this patch where you
don't put spaces around operators or between function parameters, for
example.  There's also an interesting placement of a closing paren on a
function call.  :)

Other comments follow:

@@ -425,14 +443,65 @@
 }
 
 static PangoLayout *
-build_layout (ECellTextView *text_view, int row, const char *text, gint
width)
+build_layout (ECellTextView *text_view, int row, const char *text, gint
width, gboolean include_preedit, gboolean create_new_layout)

================
I don't see a good reason for adding these addition parameters here. 
The code you've added to this function should be added to a new function
that is only called when needed, instead of making this already long
function longer by adding conditional parameters to execute new code. 
The need to add a create_new_layout parameter should have set an alarm
off.  When you make the primary purpose of a function optional by adding
a parameter, something is wrong.
================
@@ -511,7 +578,7 @@
 }
 
 static PangoLayout *
-generate_layout (ECellTextView *text_view, int model_col, int view_col,
int row, int width)
+generate_layout (ECellTextView *text_view, int model_col, int view_col,
int row, int width, gboolean include_preedit)
=================
Not much need for this to be done at this level either.  The
include_predit param should be removed here too and just call your new
function directly when necessary from the generate_layout callers.  I
suspect you added the code in build_layout because of the
pango_attr_list_splice call, but there's no reason I can see why that
can't be done elsewhere by using pango_layout_get_attributes instead of
splicing before the attrs are added to the layout.
=================
RCS file: /cvs/gnome/gal/gal/widgets/e-canvas.h,v
retrieving revision 1.17
diff -u -r1.17 e-canvas.h
--- gal/widgets/e-canvas.h      2 Apr 2003 05:46:33 -0000       1.17
+++ gal/widgets/e-canvas.h      25 Sep 2003 21:28:07 -0000
@@ -86,6 +86,7 @@
 
        GtkWidget            *tooltip_window;
        int                   visibility_notify_id;
+       int                   focus_in;
=================
What's the point of this?  It's set in one function, but never read in
your patch.

-- 
Mike Kestner <mkestner ximian com>