Created attachment 123872 [details] [review]
proposed gtk patch

for gtk+;

New API to be able to do this.

ping - can this get a review?

Created attachment 129589 [details]
chooser-button-unselect-all.py

I don't think we need a new API; gtk_file_chooser_unselect_all (button) is all you need.  It seems to work fine for me in the attached program.

Or is ::unselect_all() not working for you?  We may need to make the docs more clear on this; a patch would be lovely :)

If you find that this doesn't work for you, we could totally use a test in gtk+/gtk/tests/filechooser.c --- that's the automatic tests for the file chooser.

Maybe I do something wrong, but it doesn't work for me when I use patch from bug #332496 and change new function to gtk_file_chooser_unselect_all. The file name is still there, even when I hit "no image" in the dialog. Any idea?

Ah, okay, with that it makes more sense.  From a quick reading of the code, I think you should just use gtk_file_chooser_unselect_all() from your "response" callback in Evolution, but make the callback stop the emission of the signal.

Otherwise, GtkFileChooserButton's own handler for "response" will also run after your own handler (see gtkfilechooserbutton.c:dialog_response_cb()), and this will reset the button to showing its old_file --- it has something like

   if (response == ACCEPT || response == OK)
       /* emit selection_changed */
   else if (priv->old_file)
       /* reset the button to showing old_file */
   else
       ...

Can you try your Evo patch with something like

static void
file_chooser_response (GtkDialog *dialog, gint response_id, struct chooser_button_struct *data)
{
	g_return_if_fail (data != NULL);
	g_return_if_fail (data->button != NULL);

	if (response_id == GTK_RESPONSE_NO) {
		gtk_file_chooser_button_select_none (data->button);
+               g_signal_stop_emission_by_name (dialog, "response");
	}
}

that will not close the dialog.

Hmm --- gtkfilechooserbutton.c:dialog_response_cb() hides the dialog and makes the combo box sensitive again.  Your "response" handler would need to do the same, but it has no way to access the combo box.

What if you use connect_after for your original "response" callback instead?  It will then run after the GtkFileChooserButton's.  You will need to call gtk_file_chooser_unselect_all() from your callback to make the button clear itself.  Something like

  g_signal_connect_after (dialog, "response", response_cb, blahblah);

  static void
  response_cb (GtkDialog *dialog, int response_id, gpointer data)
  {
     if (response_id == GTK_RESPONSE_NO)
        gtk_file_chooser_unselect_all (chooser_button);
  }

I'm sorry, I should mention, but then forgot: I tried and it didn't work too.

Federico, so, what's the final word on this, please? It takes too long for such a little, but useful, thing, from my point of view.

You never said why that didn't work.  Did you trace this in gdb?

I don't have time to actually try Evolution with your patch.  Or can you build a small, self-contained test case which I can try easily?

I'm pretty convinced that we don't need an extra API in GtkFileChooserButton.  If just calling gtk_file_chooser_unselect_all() doesn't work, it's just a bug somewhere.

(In reply to comment #12)
> You never said why that didn't work.  Did you trace this in gdb?
> 
> I don't have time to actually try Evolution with your patch.  Or can you build
> a small, self-contained test case which I can try easily?
> 
> I'm pretty convinced that we don't need an extra API in GtkFileChooserButton. 
> If just calling gtk_file_chooser_unselect_all() doesn't work, it's just a bug
> somewhere.
> 

heh, knowing why that didn't work I might fix the existing function(s), but as this is pretty long time ago I really do not remember what was the reason to add new API instead of fixing the old. I can try to reinvestigate. I just hoped that someone knowing the gtk+ better than me might have it fixed much much quicker.

Created attachment 136884 [details]
sel.c - test program

OK, this is a bit more complicated than I thought.  There *is* a bug in GtkFileChooserButton.

This test program replicates what your patch for Evolution wants to do.

First I tried a normal signal_connect to "response".  In the callback I would call gtk_file_chooser_select_none (button), hide the dialog, and stop the emission of the signal.  However, the button would still show the same filename.  This doesn't work because the button's "response" handler also does other stuff, *AND* it has the selection_changed signal blocked while the dialog is active.  So, the button never notices that the selection changed to nothing.

Then I tried to do

  1. create the dialog
  2. create the button
  3. connect to "response" on the dialog

so that our "response" handler would run *after* the button had run its own handler.  This is the version in the attached program.

It does not work because gtkfilechooserbutton.c:update_label_and_image() is not synchronous.  It queues a _gtk_file_system_get_info(), which will update the label *after* our response handler has run (i.e. after the idle loop).  Also, it's not cancelling the cancellable all the time; only if the list of selected files is not empty.

I'll start fixing this.

OK, thanks a lot.

(In reply to comment #14)
> It does not work because gtkfilechooserbutton.c:update_label_and_image() is not
> synchronous.  It queues a _gtk_file_system_get_info(), which will update the
> label *after* our response handler has run (i.e. after the idle loop).  Also,
> it's not cancelling the cancellable all the time; only if the list of selected
> files is not empty.
> 
> I'll start fixing this.

Federico, any update on this, please?

Created attachment 145798 [details] [review]
gtk+-bgo563010-filechooser-button-unselect-all.diff

Sorry, I never finished fixing this.  This patch is as far as I got.  I don't remember if I actually tested it... maybe you can pick things up from there?

Created attachment 145863 [details] [review]
proposed gtk patch ][

for gtk+;

I extended your patch slightly, to let it work as I expect. It works fine with the above attached sel.c test program. What do you think, Federico?

Review of attachment 145863 [details] [review]:

Excellent, thanks for debugging this!  I'm surprised that we didn't cancel that pending operation for show_and_select_files!

You'll be happy to know that that code path no longer exists in the "filesystemmodel" branch, which will get merged soon with master.

Can you please push your patch to master?  We'll deal with the merge later.  I hope that the branch will only need the changes to gtkfilechooserbutton.c.

I have commit rights to gnome's git, and as I was told that it's not restricted to some projects only, then I probably can commit to gtk+ too. I was hoping to include this change to a next stable release as well, so we can gain from it a bit. I mean to have it available at least for 2.30, as that's a place where my patch from bug #332496 will be included. What do you think?

Err, I'm wrong. I realized that I used too old gtk+, on the actual master is your patch the right change to do (mine added chunk is irrelevant in the actual master, as the code changed there).

I'm really confused now and I'm not going to commit anything at the moment. When I remove the chunk from gtk_file_chooser_default_unselect_all in the above latest patch then it works as expected.

OK, I've just pushed the patches to master and gtk-2-18.  I guess you'll be able to use them soon from gtk+-2.18.4.

Thanks for your help in debugging this, Milan :)