Re: CList "keyfocus"

[Tim Janik <timj gtk org>]

On Sun, 28 May 2000, Mudiaga Obada wrote:

> Here is a patch that adds scrolling a list 
> by entering the first letters of the row text.
> (Does this have a name?)
> 
> It is not complete. Event chain might 
> be broken and i10nalized text doesn't work.

event chain might be broken? can you extend on that?

> Two functions are of interest:
> 
> * gtk_clist_find_row_from_text 
> I think this function should be extended and
> added to the CList API.

that's probably a good idea, though not in its current form.

> * gtk_clist_moveto_with_focus
> It seems to me that this function is of more
> usage than gtk_clist_moveto to the world outside 
> the widget.  
> Also an external candidate or am I missing something?

hm, if i'm not mistaken there's currently no function exported by clist
to adjust its focussed row, so this is probably a worthwhile addition.
though, it should probably be called gtk_clist_focus_to() (yes, _moveto
should have been called _move_to, there's no word moveto listed in my
dictionary).


gtk_clist_find_row_from_text() should probably take a
       gint (*strncmp) (const gchar *, const gchar *)
argument to let the user decide whether he wants
strncasecmp() or strncmp() (and to support customized versions).


in general, i don't think we should provide default key-press-jump-focus
(or whatever you wanna call it ;) especially since the column you
want to match varies from application to application.
the proper way to implement this is probably by adding a new signal
to clist that provides the key focus string built up over time,
users can then take appropriate actions (like moving the focus or
beeping if no match is found) in such a handler.
also, i don't see the usefullness in aborting after a certain timeout,
rather the key focus string should be reset if a non-letter key is
pressed (cursor-up/down) or a mouse event occours such like button
presses.
on the additional signal, i'd suggest te following prototype:

void (*key_focus_change) (GtkCList *clist,
                          gchar    *key_focus,
                          gboolean *reset_focus);

if *reset_focus == TRUE after emission, reset the key_focus string
to "".
for completeness, such a signal as well as focus_to() should
probably also be supplied for GtkList.


nonetheless, commentary on all patch parts:

> Index: gtkclist.c
> ===================================================================
> RCS file: /cvs/gnome/gtk+/gtk/gtkclist.c,v
> retrieving revision 1.167
> diff -u -b -B -r1.167 gtkclist.c
> --- gtkclist.c	2000/05/12 15:25:38	1.167
> +++ gtkclist.c	2000/05/28 20:39:36
> @@ -27,6 +27,7 @@
>  
>  #include <stdlib.h>
>  #include <string.h>
> +#include <gdk/gdki18n.h> 
>  #include "config.h"
>  #include "gtkmain.h"
>  #include "gtkclist.h"
> @@ -55,6 +56,10 @@
>  /* used for auto-scrolling */
>  #define SCROLL_TIME  100
>  
> +/* used for keypress string concat */
> +#define KEYFOCUS_TIMEOUT 180
> +
> +
>  /* gives the top pixel of the given row in context of
>   * the clist's voffset */
>  #define ROW_TOP_YPIXEL(clist, row) (((clist)->row_height * (row)) + \
> @@ -453,6 +458,15 @@
>  				       gint              y,
>  				       GtkCListDestInfo *dest_info);
>  
> +/* Private for now */
> +static
> +gint
> +gtk_clist_find_row_from_text (GtkCList *clist,
> +			      const gchar *match, gint start_row, gint column);
> +static 
> +void
> +gtk_clist_moveto_with_focus(GtkCList *clist, gint row, gint column);
> +

please align argument types and argument names vertically in prototypes.

>  static GtkContainerClass *parent_class = NULL;
> @@ -991,6 +1005,9 @@
>    clist->compare = default_compare;
>    clist->sort_type = GTK_SORT_ASCENDING;
>    clist->sort_column = 0;
> +
> +  clist->keyfocus_string = NULL;
> +  clist->keyfocus_lasttime = 0;
>  }
>  
>  /* Constructors */
> @@ -2958,6 +2975,38 @@
>      move_vertical (clist, row, row_align);
>  }
>  
> +/*
> + * Like gtk_clist_moveto - But sets target as new row with focus
> + * (?) Anybody using gtk_clist_moveto() from outside this widget?

yes, usually in contexts like SELECTION_SINGLE, moveto (clist->selection->data)

> + */
> +static void 
> +gtk_clist_moveto_with_focus(GtkCList *clist, gint row, gint column)

argument alignment.

> +{
> +
> +  g_return_if_fail (clist != NULL);
> +  g_return_if_fail (GTK_IS_CLIST (clist));
> +  
> +  if (row < -1 || row >= clist->rows)
> +    return;
> +  if (column < -1 || column >= clist->columns)
> +    return;
> +
> +  gtk_clist_freeze(clist);
> +	
> +  clist->focus_row = row;
> +
> +  if (ROW_TOP_YPIXEL(clist, row) < 0)
> +    gtk_clist_moveto(clist, row, column, 0, 0);
> +  else if (ROW_TOP_YPIXEL(clist, row) + clist->row_height >clist->clist_window_height)
> +    gtk_clist_moveto(clist, row, column, 1, 0);
> +
> +  gtk_clist_thaw(clist);
> +
> +}
> +
> +
> +
> +

one newline to seperate functions is sufficient.

>  void
>  gtk_clist_set_row_height (GtkCList *clist,
>  			  guint     height)
> @@ -4864,6 +4913,43 @@
>      }
>  }
>  
> +/*
> + * TODO: rc enable/disable via setting of keyfocus_timeout
> + */
> +static void
> +handle_keyfocus (GtkCList *clist, GdkEventKey *event)
> +{
> +
> +  char *str;
> +  int row;
> +  	
> +  if (clist->keyfocus_string != NULL) 
> +    if ( event->time > clist->keyfocus_lasttime + KEYFOCUS_TIMEOUT) 
> +      {
> +        g_free(clist->keyfocus_string);
> +        clist->keyfocus_string = NULL;

i don't see you freeing this in the finalize() handler.

> +      }	 
> +
> +  if (clist->keyfocus_string == NULL) 
> +    clist->keyfocus_string = g_strdup(event->string);

GNU coding style seperates paranthesis from function/macro names.


>  static gint
>  gtk_clist_key_press (GtkWidget   *widget,
>  		     GdkEventKey *event)
> @@ -4887,6 +4973,12 @@
>  	return gtk_container_focus (GTK_CONTAINER (widget),
>  				    GTK_DIR_TAB_FORWARD);
>      default:
> +      if (!(event->state & (GDK_MODIFIER_MASK ^ GDK_SHIFT_MASK)) 

it is sufficient to use 
  if ((event->state & ~GDK_SHIFT_MASK) == 0
here.

> +          && gdk_iswalnum(event->keyval)) 
> +        {
> +          handle_keyfocus(GTK_CLIST (widget), event);
> +          return TRUE;
> +        }
>        break;
>      }
>    return FALSE;
> @@ -7894,4 +7986,68 @@
>        
>        clist->button_actions[button] = button_actions;
>      }
> +}
> +
> +
> +/*
> + * TODO: 
> + * - make public (?)
> + * - regex version? no.
> + * - binary search on sorted list
> + * - if (column == -1) 
> + *     column = clist->sort_column, or 1st non GTK_CELL_PIXMAP column
> + */
> +static
> +gint
> +gtk_clist_find_row_from_text (GtkCList *clist,
> +			      const gchar *match, gint start_row, gint column)
> +{
> +  GList *list;
> +  GtkCListRow *clist_row;
> +  gint n;
> +  gchar *text;
> +  gint mlen;
> +    
> +  g_return_val_if_fail (clist != NULL, -1);
> +  g_return_val_if_fail (GTK_IS_CLIST (clist), -1);
> +
> +
> +  if (start_row < 0 || start_row >= clist->rows)
> +    return -1;
> +  if (column < 0 || column >= clist->columns)
> +    return -1;
> +  
> +  mlen = strlen(match);
> +  
> +  for (n = start_row, list = g_list_nth(clist->row_list, n); list; n++, list = list->next) {

put newlines before opening braces, list = g_list_nth (clist->row_list, n)
may go on its own line.

> +
> +    clist_row = ROW_ELEMENT (clist, n)->data;
> +
> +    switch (clist_row->cell[column].type)
> +      {
> +      case GTK_CELL_TEXT:
> +        text = GTK_CELL_TEXT (clist_row->cell[column])->text;
> +        if (!g_strncasecmp(text, match, mlen))
> +          {
> +            {
> +	      return n;
> +            }
> +          }

is this a brace nesting contest? ;)
honestly, i dare claiming that
  if (!g_strncasecmp (text, match, mlen))
    return n;
will do just as good.


> -- 
> Mudiaga Obada

---
ciaoTJ