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

[Suresh Chandrasekharan <Suresh Chandrasekharan Eng Sun COM>]

Thanks for the review and good comments. I will make these changes and and will 
repost the patch by early next week.

Regards,
Suresh


>Date: Thu, 02 Oct 2003 12:49:34 -0500
>From: Mike Kestner <mkestner ximian com>
>Subject: Re: [evolution-patches] gal patch - for 44222
>To: Suresh Chandrasekharan <Suresh Chandrasekharan Eng Sun COM>
>Cc: evolution-patches ximian com, sceri-evolution sun com
>MIME-version: 1.0
>Content-transfer-encoding: 7bit
>X-BeenThere: evolution-patches lists ximian com
>Delivered-to: evolution-patches ximian com
>X-Spam-Status: No, hits=-37.8 required=5.0	
tests=EMAIL_ATTRIBUTION,IN_REP_TO,PATCH_UNIFIED_DIFF, 
QUOTED_EMAIL_TEXT,RCVD_IN_OSIRUSOFT_COM,REFERENCES, 
REPLY_WITH_QUOTES,USER_AGENT_XIMIAN	version=2.53
>X-Spam-Level: 
>X-Spam-Checker-Version: SpamAssassin 2.53 (1.174.2.15-2003-03-30-exp)
>X-Mailman-Version: 2.0.13
>List-Post: <mailto:evolution-patches lists ximian com>
>List-Subscribe: <http://lists.ximian.com/mailman/listinfo/evolution-patches>, 
<mailto:evolution-patches-request lists ximian com?subject=subscribe>
>List-Unsubscribe: <http://lists.ximian.com/mailman/listinfo/evolution-patches>, 
<mailto:evolution-patches-request lists ximian com?subject=unsubscribe>
>List-Archive: <http://lists.ximian.com/archives/public/evolution-patches/>
>List-Help: <mailto:evolution-patches-request lists ximian com?subject=help>
>List-Id: patches against evolution go here <evolution-patches.lists.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>
>
>_______________________________________________
>Evolution-patches mailing list
>Evolution-patches lists ximian com
>http://lists.ximian.com/mailman/listinfo/evolution-patches

Thanks & Regards,
Suresh