Indeed.  I now have a test for this in autotestfilechooser; I'll have a fix ready soon.

*** Bug 351063 has been marked as a duplicate of this bug. ***

It seems to work fine here Gtk 2.10.1

Luca, see bug 351063.

*** Bug 352768 has been marked as a duplicate of this bug. ***

This should work fine in most cases with the latest gtk+ release.  There's one known problem though: if the user calls gtk_dialog_response() (again) from the response callback, the overwrite confirmation isn't run (because gtk_file_chooser_default_should_respond) isn't called.  This is probably a valid use of the API and should be fixed at some point (might have to rework the thing without using response-requested?)

I'd say this is definitely a valid use of the API since it's the *only*
way of allowing entering filenames without extensions. You can do that
in many apps, and the apps simply append the extension in their
response() callbacks. The only way of getting the changed filename
properly checked is running response() again.

We shouldn't allow recursion in ::response.  Mitch, what you really want is robust support for multiple file types and extensions in the chooser.  That's bug #135901.

Does this bug still happen, modulo the ::response thing?

I suppose a file type menu as in bug #135901 would have a "by extension"
entry and that would be active by default? Or would the menu follow the
user entering "foo.gif" even though the menu says "PNG"?

The problem here is that if you enter a filename without extension,
*and* no file type is selected from the menu, most apps would probably
want to save as their default file format, so GIMP appends ".xcf"
and just wants to run the overwrite-confirm logic again.

I don't see what's the problem with allowing to recurse in resoponse.
It did work absolutely fine in 2.8, so this is clearly a regression
that sould be fixed.

(In reply to comment #9)
> I suppose a file type menu as in bug #135901 would have a "by extension"
> entry and that would be active by default? Or would the menu follow the
> user entering "foo.gif" even though the menu says "PNG"?

I don't know; that's why the API / user interface for file types is not designed yet :)

> The problem here is that if you enter a filename without extension,
> *and* no file type is selected from the menu, most apps would probably
> want to save as their default file format, so GIMP appends ".xcf"
> and just wants to run the overwrite-confirm logic again.

In this case I would probably add a signal like "give-me-the-filename-plus-the-default-extension" that gets run *before* checking for overwrites.  Or something like that.  But it needs to be designed to fit well in the overall picture.

> I don't see what's the problem with allowing to recurse in resoponse.
> It did work absolutely fine in 2.8, so this is clearly a regression
> that sould be fixed.

If this were a normal regression I would agree, but I definitely don't think we should support recursing in ::response.

I don't know if there's an easy way to allow this, given the async nature of the checks for files.

Very well, I moved the second call to ::response to an idle function
(not recursive any more), and it still does nothing. There is simply
no way of delaying the second response call enough to have the file
chooser catch up with its internal state. On second call (with appended
extension) it simply overwrites the file without any warning.

I call this a *severe* regression, just saying "this is not allowed
any more" doesn't help a bit.

Created attachment 89066 [details] [review]
347883-minimal.diff

The problem is that GtkFileChooserDialog forgets to reset its internal response_requested flag, once the response triggered by file_chooser_widget_response_requested has been processed.

This patch contains the minimal changes needed to make the bug disappear.

Created attachment 89067 [details] [review]
347883-complete.diff

This patch is slightly more intrusive as I believe that response_requested should be reset everytime a response is received.

Created attachment 89069 [details]
testcase.c

Very good catch.  I think the patch is right, though I am not 100% sure if we should reset response_requested if !response_matters.  Maybe it should, since response_requested is used to probably only ignore the "stop emission condition" once (for a response that matters?).

It would probably be good to get that testcase converted to something that works automatically and get it integrated in autotestfilechooser.


I hope either federico or matthias can also closely look at the patch since this is all pretty confusing and then give approval.


Mitch, the testcase seems to be equivalent to gimp's behavior *before* you changed it to call response from and idle.  Does this patch also fix the case where gimp calls response from idle?  (I think it does, but would be good if you could quickly test).

The patch seems to work perfectly :-) And actually it even fixes the
original GIMP code that calls response recursively from the response
callback without any idle.

(when applying please note that both trun and 2-10 have this bug)

I'm not going to be of much use here, either trust your and mitch's judgement, or get federico to comment...

Comment on attachment 89067 [details] [review]
347883-complete.diff

Please commit to trunk and the stable branches.  Thanks for debugging this, Mathias :)

Is it possible to integrate the test into autotestfilechooser?

Commited to trunk and 2.10 branch. You said stable branch__es__. So which other branches shall see the patch? The test is integrated into autotestfilechooser on trunk.

trunk and 2.10 is all there is, thanks.