Re: [evolution-patches] seek review patch for bug 48095: week view jump button should be focusable (in the tab loop)

[Gilbert Fang <gilbert fang sun com>]

Hi, Bolian

  It is my honour to help it.  :-)

  It looks okay, I have only  a few comments:


excerpt from the patch:
+++ calendar/gui/e-week-view.c	3 Sep 2003 09:15:57 -0000
> @@ -382,6 +382,8 @@
>  		g_signal_connect (week_view->jump_buttons[i], "event",
>  				  G_CALLBACK (e_week_view_on_jump_button_event), week_view);
>  	}
> +	week_view->focused_jump_button = -1;
> +
>  	gdk_pixbuf_unref (pixbuf);

It seems the magic number "-1"  means the initial state.  Yes, the
original codes also have such use. But I wonder if we can define it as
some macro which is composed of  meaningful strings.

+	else if (event->type == GDK_KEY_PRESS) {
> +		/* return, if Tab, Control or Alt is pressed */
> +		if ((event->key.keyval == GDK_Tab) ||
> +		    (event->key.state & (GDK_CONTROL_MASK | GDK_MOD1_MASK)))
> +			return FALSE;
> +		/* with a return key or a simple character, jump to the day */
> +		if ((event->key.keyval == GDK_Return) ||
> +		    ((event->key.keyval >= 0x20) &&
> +		     (event->key.keyval <= 0xFF))) {
> +			e_week_view_jump_to_button_item (week_view, item);
> +			return TRUE;
> +		}
> +	}

I am not familiar with GDK, but I guess the keyval 0x20 and 0xFF may
have some macro definition like GDK_Space  asscoiated with them.

 	gboolean editable = FALSE;
> +	static gint last_focus_event_num = -1, last_focus_span_num = -1;
> 
> 

Just an reminder, static variables may have bad effects in multi-thread
programs.

+					     gint day);
> +void e_week_view_jump_to_button_item (EWeekView *week_view, GnomeCanvasItem *item);
> +
> 

Another reminder, if this function is never called in another complile
unit -- *.c files, we may define it as static funciton in e-week-view.c


Generally, the patch looks fine.

Best regards
Gilbert 

On Fri, 2003-09-12 at 10:03, Bolian Yin wrote:
> Hi Gilbert,
> 
> Can you help to review this patch. 
> 
> JP Rosevear wrote:
> > On Wed, 2003-09-03 at 05:49, Bolian Yin wrote:
> >   
> > > Hi,
> > > 
> > > Please review the patch.
> > > 
> > > This patch add the jump button in week view in the tab loop. When focus 
> > > comes to a jump button,
> > > press any printable key or Space/Enter, goto the day view.
> > >     
> > Approved for HEAD pending Sun hacker review.
> > 
> > -JP
> >   
>