Re: [evolution-patches] Addressbook patch to fix #55056 (Summary:keyboard navigation is disable )

[Chris Toshok <toshok ximian com>]

On Mon, 2004-04-05 at 18:17 -0400, sh wrote:
> Hi, Chris
> 
> Thank you for your  review my patch.
> I have used the new function (eab_editor_raise) to instead of the old
> functions.
> 
> Would you like to spend a little time to review it again?
> 

comments below, mostly minor.  Once these are addressed the patch is
fine to commit.

Chris

> Plain text document attachment (patch_for_55056_b2.diff)
> Index: ChangeLog
> ===================================================================
> RCS file: /cvs/gnome/evolution/addressbook/ChangeLog,v
> retrieving revision 1.1630
> diff -u -r1.1630 ChangeLog
> --- ChangeLog	31 Mar 2004 03:36:50 -0000	1.1630
> +++ ChangeLog	31 Mar 2004 09:38:44 -0000
> @@ -1,3 +1,12 @@
> +2004-03-31  Hao Sheng  <hao sheng sun com>
> +
> +	* gui/widgets/e-minicard.c :
> +	(e_minicard_event) : implement keyboard navigation(TAB/shift+TAB)
> +	(activaite_editor) : add a new function to activiate contact editor
> +	for support "Enter" key
> +
> +	Fixes #55056
> +
>  2004-03-30  Chris Toshok  <toshok ximian com>
>  
>  	[ fixes bug #34777 ]
> Index: gui/widgets/e-minicard.c
> ===================================================================
> RCS file: /cvs/gnome/evolution/addressbook/gui/widgets/e-minicard.c,v
> retrieving revision 1.112
> diff -u -r1.112 e-minicard.c
> --- gui/widgets/e-minicard.c	24 Mar 2004 20:18:49 -0000	1.112
> +++ gui/widgets/e-minicard.c	31 Mar 2004 09:38:44 -0000
> @@ -519,6 +519,46 @@
>  }
>  
>  static gboolean
> +activiate_editor(GnomeCanvasItem *item)
> +{
> +	EMinicard *e_minicard;
> +	e_minicard = E_MINICARD (item);
> +
> +	if (e_minicard->editor) {
> +		 eab_editor_raise (e_minicard->editor);
> +	} else {
> +		EBook *book = NULL;
> +		if (E_IS_MINICARD_VIEW(item->parent)) {
> +			g_object_get(item->parent, "book", &book, NULL);
> +		}
> +                                
> +		if (book != NULL) {
> +			if (e_contact_get (e_minicard->contact, E_CONTACT_IS_LIST)) {
> +				EContactListEditor *editor = eab_show_contact_list_editor (book, e_minicard->contact,
> +											   FALSE, e_minicard->editable);
> +				e_minicard->editor = G_OBJECT (editor);
> +			}
> +			else {
> +				EContactEditor *editor = eab_show_contact_editor (book, e_minicard->contact,
> +										  FALSE, e_minicard->editable);
> +				e_minicard->editor = G_OBJECT (editor);
> +			}
> +			g_object_ref (e_minicard->editor);

i know the original code had this as well, but isn't this going to leave
us with too many refs on the editor?  Can you try leaving this line out
and seeing if we crash (and leave it in and see if we leak?)

> +
> +			g_signal_connect (e_minicard->editor, "editor_closed",
> +					  G_CALLBACK (editor_closed_cb), e_minicard);
> +
> +			g_object_unref (book);
> +		}
> +	}
> +
> +	return TRUE;
> +}
> +
> +
> +
> +
> +static gboolean
>  e_minicard_event (GnomeCanvasItem *item, GdkEvent *event)
>  {
>  	EMinicard *e_minicard;
> @@ -601,38 +641,63 @@
>  		break;
>  	case GDK_2BUTTON_PRESS:
>  		if (event->button.button == 1 && E_IS_MINICARD_VIEW(item->parent)) {
> -			if (e_minicard->editor) {
> -				eab_editor_raise (e_minicard->editor);
> -			} else {
> -				EBook *book = NULL;
> -				if (E_IS_MINICARD_VIEW(item->parent)) {
> -					g_object_get(item->parent,
> -						     "book", &book,
> -						     NULL);
> -				}
> -
> -				if (book != NULL) {
> -					if (e_contact_get (e_minicard->contact, E_CONTACT_IS_LIST)) {
> -						EContactListEditor *editor = eab_show_contact_list_editor (book, e_minicard->contact,
> -													   FALSE, e_minicard->editable);
> -						e_minicard->editor = EAB_EDITOR (editor);
> -					}
> -					else {
> -						EContactEditor *editor = eab_show_contact_editor (book, e_minicard->contact,
> -												  FALSE, e_minicard->editable);
> -						e_minicard->editor = EAB_EDITOR (editor);
> -					}
> -					g_object_ref (e_minicard->editor);
> -
> -					g_signal_connect (e_minicard->editor, "editor_closed",
> -							  G_CALLBACK (editor_closed_cb), e_minicard);
> -
> -					g_object_unref (book);
> -				}
> -			}
> -			return TRUE;
> +		return activiate_editor(item);
>  		}
>  		break;
> +	case GDK_KEY_PRESS:
> +		 if (event->key.keyval == GDK_Tab ||
> +			event->key.keyval == GDK_KP_Tab ||
> +			event->key.keyval == GDK_ISO_Left_Tab) {
> +			
> +			GList *list, *focus_list;
> +			EFocus has_focus = E_FOCUS_NONE;

this variable (has_focus) isn't used anywhere..  can get rid of it.

> +			EMinicardView *view = E_MINICARD_VIEW(item->parent);
> +			EReflow *reflow = E_REFLOW(view);
> +      
> +			if (reflow == NULL) {
> +				return FALSE;
> +			}
> +		                 
> +			if (event->key.state & GDK_SHIFT_MASK) {
> +				if (event->key.state & GDK_CONTROL_MASK) {
> +					return FALSE;
> +				} else {

please use:

}
else {

instead of

} else {

likewise for the rest of the occurences of it in this patch (including
above in activate_editor).

> +					int row_count = e_selection_model_row_count(reflow->selection);
> +					int model_index = e_selection_model_cursor_row (reflow->selection);
> +					int view_index = e_sorter_model_to_sorted (reflow->selection->sorter, model_index);
> +					
> +					if (view_index == 0) {
> +						view_index = row_count-1;
> +					} else {
> +						view_index--;
> +  					}

for this if/else you don't need the braces at all.  given the previous
formatting suggestion, I'd suggest losing them.

> +					model_index = e_sorter_sorted_to_model (E_SORTER (reflow->sorter), view_index);
> +					e_canvas_item_grab_focus(reflow->items[model_index], FALSE);
> +					return TRUE;
> +				}
> +			} else {
> +				if (event->key.state & GDK_CONTROL_MASK) {
> +					return FALSE;
> +				} else {
> +					int row_count = e_selection_model_row_count(reflow->selection);
> +					int model_index = e_selection_model_cursor_row (reflow->selection);
> +					int view_index = e_sorter_model_to_sorted (reflow->selection->sorter, model_index);
> +
> +					if (view_index == row_count-1) {
> +						view_index = 0;
> +					} else {
> +						view_index++;
> + 					}

same here.

> +					model_index = e_sorter_sorted_to_model (E_SORTER (reflow->sorter), view_index);
> +					e_canvas_item_grab_focus(reflow->items[model_index], FALSE);
> +					return TRUE;
> + 				}
> +			}                                                                                                    
> +		} else if (event->key.keyval == GDK_Return ||
> +				event->key.keyval == GDK_KP_Enter) {
> +				return activiate_editor(item);
> + 			}
> + 		break;
>  	default:
>  		break;
>  	}