Confirmed on trunk. This could be a gnome-love bug.

Case 2: Should probably append the correct extension regardless
Case 3: Should probably save as PLS (default)

(In reply to comment #0)
> Case 1:
>    Playlist name: my.playlist.XXX
>    Playlist format: by extension
>    Action: save the playlist

Fair enough. It should also only select the "name" part of the filename, without the suffix, so that typing "foo" will automatically save it as "foo.pls" (or whatever the suffix is).

> Case 2:
>    Playlist name: my.playlist.without.extension or my.playlist.XXX
>    Playlist format: YYY
>    Action: save playlist "my.playlist.YYY" with YYY format

We shouldn't second guess the user. Switching playlist format should change the suffix if it's known.

> Case 3:
>    Playlist name: my.playlist.without.extension
>    Playlist format: by extension
>    Action: give an advice that you must add the extension to the playlist name
> or select the playlist format AND return to the "Save playlist" window

Just save it with the default format.

Created attachment 82348 [details] [review]
First patch

My current patch does the following things:
- case 2 is working. If the selected suffix is different from the one of the file, the selected suffix is added to the file and then saved
- case 3 is also working: filenames without a suffix and without a selected playlistformat are being saved as PLS

At the moment I have one question, regarding case 2. At the moment I don't do any checks to see if the file I want to create already exists. Thus, potentially my code can overwrite existing files and that might mean dataloss. I think I have to check this, but I am not sure how to do that. Can anyone offer some help, for example by pointing to the correct api? Thanks!

Known 'bugs' in this patch: the filename with suffix is still completely selected in the Save Playlist dialog (feature requested in #c2) + the new suffix of case 2 is added to the filename and not replacing the previous suffix.

(In reply to comment #3)
> Created an attachment (id=82348) [edit]
> First patch

Some issues:
 - Totem's coding style dictates that curly brackets are on the same line as the if statement, rather than the next (i.e. |if (blah) {|).
 - |char *tmp;| --- this should be a gchar, and declared at the top of the function.
 - It looks like you're not freeing |filename| directly after the |g_strconcat|, and thus leaking it

> At the moment I have one question, regarding case 2. At the moment I don't do
> any checks to see if the file I want to create already exists. Thus,
> potentially my code can overwrite existing files and that might mean dataloss.
> I think I have to check this, but I am not sure how to do that. Can anyone
> offer some help, for example by pointing to the correct api? Thanks!

If the behaviour before patching as regards this is the same as the behaviour with this patch, I'd file it as a separate bug, and link to the bug number here.

(In reply to comment #4)
> (In reply to comment #3)
> > Created an attachment (id=82348) [edit]
> > First patch
> 
> Some issues:
>  - |char *tmp;| --- this should be a gchar, and declared at the top of the
> function.

Actually we don't use gchar's in Totem, just char's.

Created attachment 82407 [details] [review]
Second patch

The attached patch solves both case 2 and case 3. However, there is still the problem of silently overwriting files if they have the same name and suffix. The Save Playlist dialog doesn't catch these situations, because the altering of the filename, suffix and mime type are done *after* entering the filename et cetera.

I am not sure if this is a good situation, but at the moment I have no good idea how to fix it. I tried to find the code that really saves the playlist, but I can't find it. Can someone point me to the correct location and file? And, does any of you have a good idea how to fix this? I was thinking of adding some code to the functions that are actually saving the file, to check if a file is being overwritten. But, that code would also catch the situation in which overwriting is allowed...

As far as I can see it's not possible to only select the filename and nog the suffix in the Save Playlist dialog, so I am unable to fix that.

(In reply to comment #6)
> Can someone point me to the correct location and file?

File: http://svn.gnome.org/viewcvs/totem/trunk/src/totem-playlist.c?view=markup
Function: totem_playlist_save_current_playlist_ext

During the last week I kept thinking of this bug and how to solve the possible dataloss situation and at this moment I don't think my current approach is the best solution.

If I add some code to the totem_playlist_save_current_playlist_ext function to check if a file with the specified name and suffix is already present, that will make it impossible to overwrite files; thus making it impossible to resave an existing playlist. That seems to me a situation we'd want to avoid, because user expect to be able to overwrite existing files if they choose to.

Some testing with other programs (gedit and gthumb) revealed to me that my current code is not what these programs do. Gthumb for example has a filechooserdialog that looks the same as the one totem is using: it has a input field to enter the filename and a dropdownbox to select a extension.

If an extension is selected in gthumb then the file is saved with the entered filename (even if that one has a suffix) and the selected mime type. So, it's possible to have a file named foo.jpg being a png image. No suffix is changed or added: only the user's input is used.

This seems a logic approach, because it does what the user actually asks from the application. What I am doing in my code at the moment, is altering the things the user entered and outputting things he didn't enter.

My proposal is, thus, to replicate gthumb's behaviour.

Sounds good to me. Second-guessing the user is always a bad idea.

Created attachment 82803 [details] [review]
Solves case 2 and 3

Once again I have taken another approach... this time it's a combination of my first code proposal and the reflection from #c8. This means that there are four options in my code:

File format has been selected, thus the file extension is based on that selection:
- Selected file format and suffix are the same
- Selected file format and suffix are not the same

No specific file format has been selected, extension is based on the file name:
- The playlist is saved with the default suffix, because the supplied suffix wasn't known
- The playlist is saved with the entered suffix

In all cases that my code alters the filename, there is a check in place to make sure that no files will get overwritten. In such a situation ' (number)' is added to the filename before the suffix, i.e. 'Playlist (2).pls'.

Lastly, in response to the last bullet point from #c4: filename is being freed in the caller function. If I free it, totem segfaults and outputs gibberish instead of the expected values of filename and tmp.

I don't know if it's possible, but I think it would be better if we could get a "Do you want to overwrite this file?" dialog, instead of appending numbers, which might confuse the user.

One way to do this (again, I'm not sure if it'll work) might be to get the URI and selected format from the dialog (where we currently have |gtk_file_chooser_get_uri|), then determine if we need to append an extension to the filename. If we do, we change the URI in the dialog using |gtk_file_chooser_set_uri|, then send it the "confirm-overwrite" signal, to make it display the "Do you want to overwrite this file?" dialog. Once that's sorted, we can act accordingly with the new filename.

As I said, I don't know if this is possible, but I think it would preferable to do something like this, as then we get to use the standard GtkFileChooserDialog overwrite confirmation dialog.

If it's not possible, it would probably be better to make and use our own overwrite confirmation dialog, rather than secretly appending numbers to the filename.

I have been coding lately and at this moment I have a GtkFileChooserDialog that changes the filename on the fly, when choosing another suffix using the combobox in the dialog. That works fine :) .

What doesn't work the way I want it, is this: when you click the Save I'd like to change the filename (if needed) to include the correct suffix. I connected a callback function to the Save and Cancel button and I am able to change the filename once more. But, I can't find how to make sure that the overwrite dialog is showed after changing the suffix of the filename and the new filename exists already.

I think it has something to do with emitting the correct signal, but I am not sure how to do that... or is it simply impossible to manually invoke the overwrite dialog if needed? Of course, I can create my own dialog, but I am not fond of that since the stock overwrite dialog is good enough.

Created attachment 83199 [details] [review]
Takes into account c#11

This patch takes into account the comments from c#11. The situation I created is as follows: the filename in the filechooser dialog can change in three ways:

1. using the combo box in the dialog, in that situation the suffix gets updated
2. the user enters a new filename
3. the user clicks the save button

When that last situation occurs, there is a check in place to see if the new uri (based on the path and the filename with the correct suffix) already exists. If it does, at the moment I only give a warning to the console, since I don't know how to show the overwrite dialog. If file doesn't exist yet, the normal save routine jumps in and saves the file.

I'd like to finish this patch, so if there is anyone who can tell me how to manually show the overwrite dialog via the filechooser, it would be greatly appreciated.

If you've tried sending the "confirm-overwrite" signal to the GtkFileChooserDialog (using |g_signal_emit_by_name|) and it didn't work, you might want to ask on one of the GNOME mailing lists, or in IRC.

Created attachment 83807 [details] [review]
My final take

I have asked on the gtk-app-devel how to manually emit the confirm-overwrite signal, but I haven't had any reaction. Some research learned me that the confirm-overwrite dialog is presented *before* the callback function is called. I have no idea why that is, since I set my function to be called before the default behaviour of the Save button.

So, that's why I created a new patch that does the following things:
- it changes the suffix of the entered filename when you add another mime type using the combobox
- it saves the playlist with the specified filename and the selected mime type
- if no mime type is selected, the mime type is based on the suffix.
- if no mime type is selected and the filename doesn't have a suffix, the file is saved as PLS

As far as I can see, this is the best I can come up with now, so I hope we can apply the patch and close the bug. Can someone take a closer look at my patch? Thanks in advance.

Note: there's some discussion on integrating a file format chooser with GtkFileChooser in bug #440431, which might mean that this work doesn't need to go into Totem; it could be useful in bug #440431 though.

In the end, I used the widget in bug 440431. Thanks for the hard work anyway Harm.

Let me know if the new behaviour matches your expectations (by filing new bugs!).

2008-12-10  Bastien Nocera  <hadess@hadess.net>

        * src/Makefile.am:
        * src/eggfileformatchooser.[ch]: Add the EggFileFormatChooser
        from bug 440431
        * src/totem-playlist.c (totem_playlist_save_playlist),
        (suffix_match_replace), (format_selection_changed),
        (totem_playlist_save_add_format_chooser),
        (totem_playlist_save_files): Use a better file format chooser,
        - if the extension isn't known, use "PLS"
        - when changing the file type, change the prefix of the saved file
        (Closes: #379608)