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

Good idea, should be fairly easy to implement.

All the code that needs to be touched for adding this feature is in capplets/appearance/theme-installer.c. The entrypoint to installing a theme is gnome_theme_installer_run () which will open the file dialog and subsequently initiate theme installation.

Created attachment 97023 [details] [review]
proposed patch

Allows multiple selections from a file chooser dialog and then iterates through the returned GSList.

Thanks for your patch.

I've got a couple of comments:

> @@ -629,9 +631,15 @@
> 
> 	if (gtk_dialog_run (GTK_DIALOG (dialog)) == GTK_RESPONSE_ACCEPT)
> 	{
>-		filename_selected = gtk_file_chooser_get_filename (GTK_FILE_CHOOSER (dialog));
>-		gnome_theme_install_from_uri (filename_selected, parent);
>+		filename_list = gtk_file_chooser_get_filename (GTK_FILE_CHOOSER >(dialog));

Did you actually try your patch? You are calling the wrong function here. At the very least this should cause a compiler warning, and it's quite likely to make the app crash.

>+		while (filename_list != NULL)
>+		  {
>+		    filename_selected = (gchar*)filename_list->data;
>+		    gnome_theme_install_from_uri (filename_selected, parent);
>+		    filename_list = filename_list->next;
>+		  }
> 		g_free (filename_selected);

Why are you only freeing the last filename in the list?

>+		g_free (filename_list);

filename_list is always NULL here, so this won't do any good. You need to keep a pointer to the original list and free that. You also need to use g_slist_free instead of g_free.

> 	}

Now, if the issues I mentioned are fixed, this should work. However, it would be nice to add a few more changes. What's not so nice about this solution is that you will get a separate confirmation window for each theme you install in this way (in fact, we already have that problem now if there are several themes inside one theme package). It would be much nicer to get only one notification listing all the themes that have been installed (and possibly errors that have occurred).

Ideas for what that window could look like are most welcome, and this would make a great extension of this enhancement request.

Thanks for the feedback, Jens. I'm new to this and eager to learn. Here is a revised patch per your comments.  I've tested it and works, but it spits out an "Apply new theme" dialog for each theme. Also, it doesn't work when you drag-and-drop multiple files. How do I fix this? 

I'd like to come up with a way to qeue success and error messages and then output a dialog displaying this information. What would be the best way to do this? 

Created attachment 97082 [details] [review]
revised patch

>+		g_slist_free (filename_list);

This is still wrong. filename_list is NULL at that point, because you've already walked the list to the end.

> Also, it doesn't work when you
> drag-and-drop multiple files. How do I fix this? 

Drag and drop is handled in theme_drag_data_received_cb() in appearance-themes.c. As you can see, "uris" really is a list, but we're currently only looking at the first element of it.

> I'd like to come up with a way to qeue success and error messages and then
> output a dialog displaying this information. What would be the best way to do
> this? 

It would probably a good idea to think about how the finished dialog should look, first. In any case, you'll have to refactor the theme-installer module quite a bit. You could pass a list to gnome_theme_install_from_uri(), and instead of popping up those dialogs, the function simply appends the names of the installed themes to that list. After you're done with the loop, you can then display what has been installed and ask the user whether he wants to activate one of those themes. Something like that has to be done for errors that happen during installation as well.

Created attachment 97115 [details] [review]
This patch uses g_slist_foreach instead of a while loop

Jens is this an acceptable solution?

Created attachment 97123 [details] [review]
With a while loop?

or is this a better solution?

Neither. ;-)

Both of your solutions have the same problem. g_slist_free only frees the list structure itself, not the list items.

Now, you could do

g_slist_for_each (list, install, ...);
g_slist_for_each (list, g_free, ...);
g_slist_free (list);

but that would mean you'd walk the list twice which is rather unnecessary in this case (it's probably a valid assumption that our list is going to be very small so it doesn't matter that much, but better not get into any bad habits).

So, in this case, it's better to use an open-coded loop, free the list items in the loop, and the list itself afterwards.

Oh, and please try to keep variable names to a sensible length and stick to the coding style (ie. where to put braces etc.).

Created attachment 97138 [details] [review]
one more attempt

I followed the documentation at
http://developer.gnome.org/doc/tutorials/gnome-libs/glib.html

If this isn't right, then I'm incredibly confused. Maybe you could let me know when you'll be on IRC and you could help me out some more?

Yes, that looks better (except that you didn't quite follow the coding conventions (spaces before opening braces)).

Care to do the drag and drop stuff as well? Feel free to drop by on the #control-center IRC channel if you need help (I'm fizz, but others can probably help you out, too).

Created attachment 97177 [details] [review]
inline with coding style

I'm sorry about that. I didn't know what you meant at first ( I thought you meant curly braces.) I found the coding style documentation and updated my .emacs accordingly. This shouldn't be a problem from now on.

I do plan on working on a drag and drop patch. As well as a patch so that each theme doesn't pop up a window on install completion, but rather qeues until the end.

(In reply to comment #14)
> I found the coding style documentation and updated my
> .emacs accordingly. This shouldn't be a problem from now on.

It's not always quite that simple. Different parts have been written by different people at different points in time and as such are sometimes using different coding styles. ;-)
You should always try to adapt to the part of the code you're editing.

> I do plan on working on a drag and drop patch. As well as a patch so that each
> theme doesn't pop up a window on install completion, but rather qeues until the
> end.

Great, looking forward to it.

Benjamin, I've made a few changes in svn trunk to get rid of the annoying "one window per installed theme" behaviour. I've only fixed this for multiple themes in one package, though. Doing it properly for multiple packages looks like it would require a thorough rewrite of the core installer functions.

It's probably still gnome-love material, but a bit more work than I originally envisioned.

Jens, I've been working on this problem for little while now, and it is a bit more work than I realized, too. What I've done so far is have gnome_theme_install_from_uri take a list of themes and then copy those themes to a temporary directory. Then transfer_done_cb extracts the files if necessary and calls gnome_theme_install_real on each theme. gnome_theme_install_real reports back to transfer_done_cb whether the theme was installed successfully. Successes and failures are recorded and then presented to the user once the installation of all files is completed.

At present I'm designing a dialog to apply the themes after they've been installed. I'd like to employ radio buttons. Using one for each theme, the user would then be able to select the theme she wants to apply from the list of all installed themes, or keep the current if she prefers. 

What do you think? Is this the direction you want to go, or would you rather disable application of themes from a dialog after installing them?

You will probably find that the radio button approach doesn't work so well.

Consider the case where a user installs an icon theme, 2 gtk themes, and 2 metacity themes. With a radio button, you will only be able to apply one of those themes, even though, for example, apllying the icon theme, one of the gtk and one of the metacity themes would be perfectly fine. Now, you could make one radio button group per type of theme, but then what if one of the installed themes is a metatheme that includes icon, gtk, AND metacity? Plus, if the user installs multiple themes he might want to get an idea of how they look like before he decides which of them to apply, so you'd need previews.

Basically, you'd be reimplementing the theme selection in the capplet. You'll probably agree that that's not a good idea. So, I think it makes sense to ask the user whether to apply the theme if he installed exactly one theme. In the other cases, I'd just pop up a message telling him that the new themes have successfully installed (and maybe list the names of the new themes, but I'm not sure of that, either, so I'd go with the simple solution for now, just like I did for multiple themes in one package).

Also make sure that you stay in sync with svn trunk because that's where such a patch would ultimately be applied.

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

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

Mass move to gnome-tweak-tool, for theme handling bugs.

*** This bug has been marked as a duplicate of bug 656604 ***