Re: New treeviewized GtkFileSelection and GTK_SELECTION_MULTIPLE

[Owen Taylor <otaylor redhat com>]

> 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.

 - Need function docs :-)

> +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.  

> +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.

> +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);

> +#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()?

> +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"

> +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;

Regards,
                                        Owen