Re: [Nautilus-list] patch for directory view as list

["John Sullivan" <sullivan eazel com>]

Hi Matt,

Thanks for catching this problem and writing a patch. Your patch fixes the
problem (which, surprisingly, hadn't yet been reported in
bugzilla.eazel.com, I don't think). However, rather than just take your
patch and forget about the issue for now, I'd prefer that the related code
be cleaned up a little more. It's been in a funny state since the creation
of nautilus-clist. Specifically, there is no need to have both
nautilus_list_get_first_selected_row and
nautilus_clist_get_first_selected_row (and the same for _get_last_). The
_clist_ flavor is a private function used only once, in a place where the
public _list_ version would work just fine (there's already a local variable
holding the NautilusList). So we should remove the _clist_ flavor of these
two functions and leave just the _list_ flavor.

Also, since we don't use nautilus_gtk_clist_get_first/last_selected_row any
more, we should just delete those functions entirely. They will just take up
a little space and bit-rot away uselessly otherwise.

There are two answers to your question about where
nautilus_clist_get_first/last_selected_row belong. The first one is "nowhere
-- they should just be removed with their guts moved into
nautilus_list_get_first/last_selected_row", as I mentioned above. But if we
wanted them as separate functions for some reason, we would prefer not to
put them into the cut-n-paste code directory. The idea with the cut-n-paste
code directory is that its contents exist only because it is impossible to
get the behavior we need any other way than to start with a hunk of
cut-n-pasted code. This cut-n-pasted code should be modified as minimally as
possible, with the hope of some day eliminating it (e.g. by rolling bug
fixes into the original source from which the cut-n-pasted code came).
Usually, it is modified only to make critical bug fixes that can't feasibly
be done from a subclass, or to expose the minimal additional bits of API
required to allow a subclass to make more substantive changes. In this
particular case, the get_first/last_selected_row feature can easily be added
to a subclass already, so we wouldn't want to modify cut-n-paste code to
include it.

Matt, please let me know if you would like me to finish up this
first/last_selected_row issue, or whether you would like to do so. I'm happy
to take your patch and make the other changes I mentioned. And I'm equally
happy to let you do it. Since you found the problem, I'll let you make the
call.

John

on 10/15/00 11:58 PM, Matt Bissiri at bissiri eecs umich edu wrote:

> Hi,
> 
> when viewing a directory as list, up/down/pageup/pagedown keys do not
> work
> as expected.  This is happening with the latest (2000-10-16) from cvs.
> 
> Switch to "view as list", press up and down arrow keys,
> only the last, then the first, file is selected (instead of moving
> the selection up and down the list), and these messages are displayed:
> 
> ** CRITICAL **: file nautilus-gtk-extensions.c: line 133
> (nautilus_gtk_clist_get_first_selected_row): assertion `GTK_IS_CLIST
> (list)' failed.
> 
> ** CRITICAL **: file nautilus-gtk-extensions.c: line 161
> (nautilus_gtk_clist_get_last_selected_row): assertion `GTK_IS_CLIST
> (list)' failed.
> 
> Looking in libnautilus-extensions/nautilus-list.c around line 1207:
> static int
> nautilus_clist_get_first_selected_row (NautilusCList  *list)
> {
> return nautilus_gtk_clist_get_first_selected_row ((GtkCList
> *)list);
> }
> ^^^^^^^^^^^
> This cast is incorrect, since GtkCList was replaced with NautilusCList
> as the parent class of NautilusList (on 2000-10-03).
> 
> I've attached a patch.
> 
> Should `nautilus_clist_get_first_selected_row' and
> `nautilus_clist_get_last_selected_row' be in:
> cut-n-paste-code/widgets/nautilusclist/nautilusclist.[ch] instead?
> In this patch I left them in: libnautilus-extensions/nautilus-list.c
> because cut-n-paste-code/README said not to modify code under
> cut-n-paste-code/
> (despite the fact that cut-n-paste-code/widgets/nautilusclist is a
> modification of the original gtkclist code).
> 
> Matt
> 
> 
> 
> ? idl/Makefile.in
> ? idl/Makefile
> Index: ChangeLog
> ===================================================================
> RCS file: /cvs/gnome/nautilus/ChangeLog,v
> retrieving revision 1.2366
> diff -u -r1.2366 ChangeLog
> --- ChangeLog 2000/10/15 07:56:04 1.2366
> +++ ChangeLog 2000/10/16 06:32:12
> @@ -1,3 +1,18 @@
> +2000-10-16  Matt Bissiri  <bissiri eecs umich edu>
> +
> + * libnautilus-extensions/nautilus-list.c:
> + (nautilus_clist_get_first_selected_row),
> + (nautilus_clist_get_last_selected_row):
> + Now that NautilusList derives from NautilusCList instead of GtkCList,
> + do not call `nautilus_gtk_clist_get_first_selected_row' or
> + `nautilus_gtk_clist_get_last_selected_row'.
> + Instead add implementation using NautilusCList instead of GtkCList.
> + This fixes a bug where up/down/pgup/pgdown keys did not work properly
> + when viewing directory as list.
> + (nautilus_list_get_first_selected_row):
> + To avoid code duplication, replace the body of this function
> + with a call to `nautilus_clist_get_first_selected_row'.
> +
> 2000-10-15  Andy Hertzfeld  <andy eazel com>
> 
> * src/nautilus-throbber.c: (load_themed_image),
> Index: libnautilus-extensions/nautilus-list.c
> ===================================================================
> RCS file: /cvs/gnome/nautilus/libnautilus-extensions/nautilus-list.c,v
> retrieving revision 1.80
> diff -u -r1.80 nautilus-list.c
> --- libnautilus-extensions/nautilus-list.c 2000/10/13 06:21:12 1.80
> +++ libnautilus-extensions/nautilus-list.c 2000/10/16 06:32:13
> @@ -1202,15 +1202,43 @@
> }
> 
> static int
> -nautilus_clist_get_first_selected_row (NautilusCList  *list)
> +nautilus_clist_get_first_selected_row (NautilusCList *list)
> {
> - return nautilus_gtk_clist_get_first_selected_row ((GtkCList *)list);
> + NautilusCListRow *row;
> + GList *p;
> + int row_index;
> +
> + g_return_val_if_fail (NAUTILUS_IS_CLIST (list), -1);
> +
> + for (p = NAUTILUS_CLIST (list)->row_list, row_index = 0;
> +      p != NULL;
> +      p = p->next, ++row_index) {
> +  row = p->data;
> +  if (row->state == GTK_STATE_SELECTED)
> +   return row_index;
> + }
> +
> + return -1;
> }
> 
> static int
> nautilus_clist_get_last_selected_row (NautilusCList *list)
> {
> - return nautilus_gtk_clist_get_last_selected_row ((GtkCList *)list);
> + NautilusCListRow *row;
> + GList *p;
> + int row_index;
> +
> + g_return_val_if_fail (NAUTILUS_IS_CLIST (list), -1);
> +
> + for (p = NAUTILUS_CLIST (list)->row_list_end, row_index = NAUTILUS_CLIST
> (list)->rows - 1;
> +      p != NULL;
> +      p = p->prev, --row_index) {
> +  row = p->data;
> +  if (row->state == GTK_STATE_SELECTED)
> +   return row_index;
> + }
> +
> + return -1;
> }
> 
> static void
> @@ -3346,21 +3374,13 @@
> int
> nautilus_list_get_first_selected_row (NautilusList *list)
> {
> - NautilusCListRow *row;
> - GList *p;
> - int row_index;
> + NautilusCList *clist;
> 
> g_return_val_if_fail (NAUTILUS_IS_LIST (list), -1);
> -
> - for (p = NAUTILUS_CLIST (list)->row_list, row_index = 0;
> -      p != NULL;
> -      p = p->next, ++row_index) {
> -  row = p->data;
> -  if (row->state == GTK_STATE_SELECTED)
> -   return row_index;
> - }
> 
> - return -1;
> + clist = NAUTILUS_CLIST (list);
> + 
> + return nautilus_clist_get_first_selected_row (clist);
> }
> 
> /* Workaround for a bug in GtkCList's insert_row.
>