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

[Bolian Yin <bolian yin sun com>]

Hi Gilbert,

Thanks for the review.  I will make changes according the comments, except the last one.  I make that function public because I need it in the a11y code, which will come later.

Regards,
Bolian


Gilbert Fang wrote:

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
  
      

_______________________________________________
Evolution-patches mailing list
Evolution-patches lists ximian com
http://lists.ximian.com/mailman/listinfo/evolution-patches