Re: New treeviewized GtkFileSelection and GTK_SELECTION_MULTIPLE

[Manish Singh <yosh gimp org>]

On Thu, Feb 14, 2002 at 10:20:30PM -0500, Owen Taylor wrote:
> 
> > Patch is attached. The diff stuff logic isn't quite finished yet, but the
> > API is there and should be working.
> 
> Hmmm, some thoughts:
> 
>  - I think we should go for code simplicity - always maintain the   
>    selected list, use permanent signal connections, use struct
>    fields instead of object data.

Ok.
 
>  - Need function docs :-)

Right. :)
 
> > +void
> > +gtk_file_selection_set_select_multiple (GtkFileSelection *filesel,
> > +					gboolean          select_multiple)
> > +{
> > +  GtkTreeSelection *sel;
> > +  GtkSelectionMode mode;
> > +
> > +  g_return_if_fail (GTK_IS_FILE_SELECTION (filesel));
> > +
> > +  sel = gtk_tree_view_get_selection (GTK_TREE_VIEW (filesel->file_list));
> > +
> > +  mode = select_multiple ? GTK_SELECTION_MULTIPLE : GTK_SELECTION_SINGLE;
> > +
> > +  gtk_tree_selection_set_mode (sel, mode);
> > +
> > +  g_signal_handlers_disconnect_matched (sel, G_SIGNAL_MATCH_DATA, 0, 0, NULL,
> > +      					NULL, filesel);
> > +  if (select_multiple)
> > +    g_signal_connect (sel, "changed",
> > +		      G_CALLBACK (gtk_file_selection_multiple_changed),
> > +		      filesel);
> > +  else
> > +    g_signal_connect (sel, "changed",
> > +		      G_CALLBACK (gtk_file_selection_file_changed),
> > +		      filesel);
> > +}
> 
> if we leave it this way (and don't simplify to always maintain the
> selected list), then we need to obtain the selected list here in case
> an item is selected already.  

Nah, we'll simplify.
 
> > +gboolean
> > +gtk_file_selection_get_select_multiple (GtkFileSelection *filesel)
> > +{
> > +  GtkTreeSelection *sel;
> > +
> > +  g_return_val_if_fail (GTK_IS_FILE_SELECTION (filesel), FALSE);
> > +
> > +  sel = gtk_tree_view_get_selection (GTK_TREE_VIEW (filesel->file_list));
> > +  return !!(gtk_tree_selection_get_mode (sel) == GTK_SELECTION_MULTIPLE);
> > +}
> 
> he !! shouldn't be neecssary a comparison is always 0 or 1 for the result.

Ok.
 
> > +static void
> > +multiple_changed_foreach (GtkTreeModel *model,
> > +			  GtkTreePath  *path,
> > +			  GtkTreeIter  *iter,
> > +			  gpointer      data)
> > +{
> > +  GPtrArray *names = data;
> > +  gchar *filename;
> > +
> > +  gtk_tree_model_get (model, iter, FILE_COLUMN, &filename, -1);
> > +
> > +  g_ptr_array_add (names, filename);
> > +}
> > +
> > +static void
> > +free_selected_names (gpointer data)
> > +{
> > +  g_ptr_array_free ((GPtrArray *) data, TRUE);
> > +}
> 
> Looks like this leaks the file names. (gtk_tree_model_get dups
> the strings. Probably safe to leave them dup'ed, but then we
> need to free the names.
> 
> I might suggest that we use a "string vector instead of the
> array for storage so we can just use g_strfreev()
> 
>  g_ptr_array_add (array, NULL);
>  vector = g_ptr_array_free (array, FALSE);

Duh. Right. Brain fart on what free_segment does. Your suggestion is quite
workable.

> > +#if !defined(G_OS_WIN32) && !defined(G_WITH_CYGWIN)
> > +#define ms_strcmp(a, b) strcmp(a, b)
> > +#else
> > +#define ms_strcmp(a, b) g_strcasecmp(a, b)
> > +#endif
> 
> I think ms_strcmp() is confusing (n Unixe it has nothing to
> do with "MS"). Why not compare_filenames()?

Heh, MS == Multiple Select. I guess compare_filenames is clearer. :) On
a related note, should we be using strcoll instead of strcmp and friends?
I notice GNU fileutils ls uses it, which results in a different sort order
even under en_US (AaBbCc...)
 
> > +static void
> > +gtk_file_selection_multiple_changed  (GtkTreeSelection *selection,
> > +				      gpointer          user_data)
> > +{
> > +  GtkFileSelection *fs = GTK_FILE_SELECTION (user_data);
> > +  GPtrArray *new_names, *old_names;
> > +  gchar *filename, *old_filename;
> > +  gint i, index = 0;
> > +
> > +  new_names = g_ptr_array_sized_new (8);
> > +
> > +  gtk_tree_selection_selected_foreach (selection,
> > +				       multiple_changed_foreach,
> > +				       new_names);
> > +  /* nothing selected */
> > +  if (new_names->len == 0)
> > +    {
> > +      g_ptr_array_free (new_names, TRUE);
> > +      g_object_set_qdata (G_OBJECT (selection), quark_selected_names, NULL);
> > +      return;
> > +    }
> > +
> > +  if (new_names->len != 1)
> > +    {
> > +      old_names = g_object_get_qdata (G_OBJECT (selection),
> > +				      quark_selected_names);
> > +
> > +      if (old_names != NULL)
> > +	{
> > +	  /* FIXME: unfinished logic here, need to get the cases for
> > +	   * multiremoval correct
> > +	   */
> > +	  if (new_names->len > old_names->len)
> > +	    {
> > +	      if (ms_strcmp (g_ptr_array_index (old_names, old_names->len - 1),
> > +			     g_ptr_array_index (new_names, new_names->len - 1)) < 0)
> > +		index = new_names->len - 1;
> > +	      else
> > +		{
> > +		  for (i = 0; i < old_names->len; i++)
> > +		    {
> > +		      if (ms_strcmp (g_ptr_array_index (old_names, i),
> > +				     g_ptr_array_index (new_names, i)) != 0)
> > +			{
> > +			  index = i;
> > +			  break;
> > +			}
> > +		    }
> > +		}
> > +	    }
> > +	  else if (new_names->len < old_names->len)
> > +  	    {
> > +	    }
> > +	  else
> > +	    {
> > +	    }
> > +	}
> > +      else
> > +	{
> > +	  old_filename = g_object_get_qdata (G_OBJECT (selection),
> > +					     quark_last_selected);
> > +
> > +	  if (ms_strcmp (old_filename, g_ptr_array_index (new_names, 0)) == 0)
> > +	    index = new_names->len - 1;
> > +	  else
> > +	    index = 0;
> > +	}
> > +    }
> > +  else
> > +    index = 0;
> > +
> > +  g_object_set_qdata_full (G_OBJECT (selection), quark_selected_names,
> > +			   new_names, free_selected_names);
> > +
> > +  filename = g_ptr_array_index (new_names, index);
> > +  g_object_set_qdata_full (G_OBJECT (selection), quark_last_selected,
> > +      			   g_strdup (filename), g_free);
> > +
> > +  old_filename = filename;
> > +  filename = get_real_filename (filename);
> > +
> > +  gtk_entry_set_text (GTK_ENTRY (fs->selection_entry), filename);
> > +
> > +  if (filename != old_filename)
> > +    g_free (filename);
> > +}
> 
> Why do we need a complicated diff? Isn't the algorithm simply:
> 
>  "If the selection contains at least one name that wasn't selected
>   previously, set that as the entry text. Otherwise, leave the
>   entry text unchanged"

Not that simple:

Suppose there are files A, B, C, D, E in a directory. The User selects A,
then shift-clicks to E, selecting all five. Then shift-clicks on C. The
selection shrinks to A, B, C. All 3 of those were in the previous set,
but we want C to be in the entry now since it was the last one that is
clicked.

or:

User selects A then shift-clicks to E, then ctrl-clicks on C to deselect
it, then shift-clicks on E. Now C, D, and E are selected, but E is the
last one clicked, which was also in the previous set.

My code doesn't handle those cases well either, still pondering it.
 
> > +gchar **
> > +gtk_file_selection_get_selections (GtkFileSelection *filesel)
> 
> I'm not _positive_ this shouldn't be:
> 
>  gint gtk_file_selection_get_selections (GtkFileSelection   *filesel,
>                                          gchar            ***selections)
> 
> [ I always get annoyed when I get returned a char ** and have to count it. ]
> 
> Or:
> 
>  void gtk_file_selection_get_selections (GtkFileSelection   *filesel,
>                                          gchar            ***selections,
>                                          gint                n_selections);
> 
> But let's leave it the way you have it.
> 
> > +  GtkTreeSelection *sel;
> > +  GPtrArray *names;
> > +  gchar **selections;
> > +  gchar *filename, *dirname, *current;
> > +  gint i, j;
> > +
> > +  g_return_val_if_fail (GTK_IS_FILE_SELECTION (filesel), NULL);
> > +
> > +  sel = gtk_tree_view_get_selection (GTK_TREE_VIEW (filesel->file_list));
> > +
> > +  names = g_object_get_qdata (G_OBJECT (sel), quark_selected_names);
> > +
> > +  selections = g_new (gchar *, names->len + 2);
> > +
> > +  filename = g_strdup (gtk_file_selection_get_filename (filesel));
> > +
> > +  if (strlen (filename) == 0)
> > +    return NULL;
> > +
> > +  selections[0] = filename;
> > +  j = 1;
> > +
> > +  if (names != NULL)
> > +    {
> > +      dirname = g_path_get_dirname (filename);
> > +
> > +      for (i = 0; i < names->len; i++, j++)
> > +	{
> > +	  current = g_build_filename (dirname, g_ptr_array_index (names, i),
> > +				      NULL);
> > +
> > +	  if (ms_strcmp (current, filename) == 0)
> > +	    {
> > +	      g_free (current);
> > +	      selections = g_realloc (selections, names->len + 1);
> 
> Again, promoting code simplicity, I don't think it's worth saving the
> 4 bytes by decreasing the array size.   It's probably also clearer
> to write:
> 
>  count  = 0;
>  selections[count++] = filename;
>  
>  for (i = 0; i < names->len ; i++) 
>    {
>      [...] 
> 
>       if (ms_strcmp (current, filename) !== 0)
>         selections[count++] = currnet; 
>      else
>         g_free (current);
>    }
>  
>  selections[count] = NULL;

Ok, that sounds good. I should have a revised patch tomorrow or so.

-Yosh