Created attachment 74794 [details] [review]
control-center-media-key-support.patch

Adds support for default media player when using the extra Media button on keyboard (or remote). 
I couldn't test it because the media player tab never showed up ;)

As I mentioned in bug 373939, I don't think this is really necessary. Binding the key to an arbitrary command should solve this issue (bug 114796).

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

Perhaps a more relevant comment is that I can't see any uses outside of the keyboard shortcuts for a "preferred media player". If there are other possible uses for a "preferred media player", then please feel free to re-open this bug.

(In reply to comment #3)
> Perhaps a more relevant comment is that I can't see any uses outside of the
> keyboard shortcuts for a "preferred media player". If there are other possible
> uses for a "preferred media player", then please feel free to re-open this bug.

Re: the GNOME Media Center thread on d-d-l, this option could be used to determine which music player the user prefers to use the database of. Even if some players use a common database (wishful thinking), some users might prefer an application that doesn't. The integrated media center could use this GConf key to determine which application to nick the database from, given a plugin existed for it.

Luca, does this look ok to you? If so, please commit to 2.19 ASAP, so that we have as many new features as possible in soon to get enough testing

I will try to resync the patch against the SVN trunk tonight...

Created attachment 84907 [details] [review]
gcc-media-gconf-stuff.patch

Required schemas and keybindings for media key
 * Add Media key support for the keybinding applet
 * Add required schema for media keybinding support in gnome-control-daemon
 * Fix small typo in the gnome-settings daemon schemas 
 * Change "Sound" to "Multimedia" in keybinding capplet

Created attachment 84912 [details] [review]
gcc-default-media-app.patch

Adds default media application support do default applications and also fixes two bugs:
  * Terminal execution arguments were not read (misplaced braces)
  * Mnemonic widget for web_browser_command_label widget was wrong

It should be also noted that this patch introduces a lot of duplicate code, but because it is RFC anyway, it can be cleaned and optimized later...

There will be one more patch against gnome-settings-daemon...

Created attachment 84915 [details] [review]
gcc-gnome-settings-daemon-media.patch

Add media key handler to the gnome-settings-daemon.

Thats all folks ;)

I looked at the patch (gcc-gnome-settings-daemon-media.patch). It seems good, but I would change just a very few things. Note that I didn't test the patch yet, just looked at it from bugzilla, I hope to find some time tomorrow to make a deeper analysis :)

in .glade file
1. I don't know if it is a good idea to add comments in glade file, Maybe it is better to remove them.

in .xml.in file
2. I use Rhythmbox, I can't find it in the xml file :)

3. why did you use the same gnome-mime-application-x-executable icon for all the apps?

in gnome-da-capplet.h
4. you added /* Multimedia tab */ comment, remove it or add similar ones to other groups of widgets (but I'm for removing it, it is clear enough even without the comment)

in gnome-da-item.c
5. after gnome_da_media_item_free you put two spaces insted of one :)

in gnome-da-item.h
6. here I would prefer to have something like this (or even two structs with same fields, but I would avoid a typedef of _GnomeDAMailItem to GnomeDAMediaItem), like in your previous patch (more or less :)

typedef struct _GnomeDASimpleItem GnomeDAMailItem;
typedef struct _GnomeDASimpleItem GnomeDAMediaItem;

struct _GnomeDASimpleItem {
    GnomeDAItem generic;
    gboolean run_in_terminal;
};

in gnome-da-capplet.c
7. why did you use stock_gtk-about icon instead of gnome-audio (the sound_image icon commented in the file, remove this field or rename it to media_player_image and)

8. why did you add this?

+#if 0
     if (g_file_test (GNOMECC_GLADE_DIR "/gnome-default-applications-properties.glade", G_FILE_TEST_EXISTS) != FALSE) {
 	capplet->xml = glade_xml_new (GNOMECC_GLADE_DIR "/gnome-default-applications-properties.glade", NULL, NULL);
     }
     else {
+#endif
 	capplet->xml = glade_xml_new ("./gnome-default-applications-properties.glade", NULL, NULL);
-    }
+//    }

I often run the capplet from source dir for testing

9. why did you move the gconf_client_get inside the if?

-    value = gconf_client_get (capplet->gconf, DEFAULT_APPS_KEY_TERMINAL_EXEC_ARG, NULL);
     if (value)
     {
+    value = gconf_client_get (capplet->gconf, DEFAULT_APPS_KEY_TERMINAL_EXEC_ARG, NULL);

Ops, sorry, of course I looked at gcc-default-media-app.patch, not gcc-gnome-settings-daemon-media.patch :)

Created attachment 84946 [details] [review]
gcc-default-media-app-v2.patch

> in .glade file
> 1. I don't know if it is a good idea to add comments in glade file, Maybe it is better to remove them.
It is XML, so it should be fine..

> in .xml.in file
> 2. I use Rhythmbox, I can't find it in the xml file :)
OK, added. But I didn't have it installed last night ;)

> 3. why did you use the same gnome-mime-application-x-executable icon for all the apps?
Changed to app-specific icons.

> in gnome-da-capplet.h
> 4. you added /* Multimedia tab */ comment, remove it or add similar ones to
> other groups of widgets (but I'm for removing it, it is clear enough even
> without the comment)
Removed.

> in gnome-da-item.c
> 5. after gnome_da_media_item_free you put two spaces insted of one :)
Killed the extra s/space/newline ;)

> in gnome-da-item.h
> 6. here I would prefer to have something like this (or even two structs with
> same fields, but I would avoid a typedef of _GnomeDAMailItem to
> GnomeDAMediaItem), like in your previous patch (more or less :)
Done..

> in gnome-da-capplet.c
> 7. why did you use stock_gtk-about icon instead of gnome-audio (the sound_image
> icon commented in the file, remove this field or rename it to
> media_player_image and)
Fixed, now I use gnome-audio (although it needs some Tango-ification)

> 8. why did you add this?
Removed this so nothing to see here ;)

> 9. why did you move the gconf_client_get inside the if?
Heh.. I actually spotted a reverse variant, but fixed it in wrong place ;)
Thanks for catching :D

> I looked at the patch (gcc-gnome-settings-daemon-media.patch). It seems good,
> but I would change just a very few things. Note that I didn't test the patch
> yet, just looked at it from bugzilla, I hope to find some time tomorrow to 
> make a deeper analysis :)

I actually haven't tested the gnome-settings-daemon part myself yet, simply because the keybindings capplet comes up empty :( Anyway, most of the stuff has been addressed now.. 

I would also look into "TODO GConfPropertyEditor" after this has been merged, plus this GnomeDaxxxItem thing can be cleaned up a bit more...

I'm sorry to bother you again :)

I'm testing the patch right now and it works quite well, the only missing feature is that if I manually change the gconf entry via gconf-editor from one player to another, the capplet doesn't reflect my new choice, while I can do that for other categories. You can do this via gconf_client_notify_add (look in gnome-da-capplet.c:main).

Feel free to remove the " { "sound_image",       "gnome-audio"      }," entry from icons[] array, it is unused now since you renamed the widget to media_player_image. When a new Tango-styled gnome-audio icon will be added, we will have a nice icon too :)

In gnome-da-capplet.c:show_dialog you added two empty lines, instead on one, right after capplet->media_player_terminal_checkbutton = glade_xml_get_widget () :)

In gnome-da-capplet.c:terminal_checkbutton_toggled_cb there is a space after the "}", right before our else :)

As for the comments in glade file I'm sure they are allowed, but since .glade files are opened with glade editor, those comments will never be read by anyone, and when those widgets will be re-enabled comments will still be present. Anyway, if maintainers are ok whith this, I'm ok too.

> plus this GnomeDaxxxItem thing can be cleaned up a bit more...

I can't agree more with you :)

Thanks for all your work Priit.

Created attachment 84959 [details] [review]
gcc-default-media-app-v3.patch

> I'm testing the patch right now and it works quite well, the only missing
> feature is that if I manually change the gconf entry via gconf-editor from one
> player to another, the capplet doesn't reflect my new choice, while I can do
> that for other categories. You can do this via gconf_client_notify_add (look in
> gnome-da-capplet.c:main).
Works now, thanks for spotting :)

> Feel free to remove the " { "sound_image",       "gnome-audio"      }," entry
> from icons[] array, it is unused now since you renamed the widget to
> media_player_image. When a new Tango-styled gnome-audio icon will be added, we
> will have a nice icon too :)
Removed...

> In gnome-da-capplet.c:show_dialog you added two empty lines, instead on one,
> right after capplet->media_player_terminal_checkbutton = glade_xml_get_widget
> () :)
> In gnome-da-capplet.c:terminal_checkbutton_toggled_cb there is a space after
> the "}", right before our else :)
Done and done..

> As for the comments in glade file I'm sure they are allowed, but since .glade
> files are opened with glade editor, those comments will never be read by
> anyone, and when those widgets will be re-enabled comments will still be
> present. Anyway, if maintainers are ok whith this, I'm ok too.
Nothing to lose there anyway..  
It was quite hard to figure out why these specific widgets were not visible because glade lacks this sort of visual notification. Eventually I had to edit this by hand..

And do not worry about bothering.. I actually enjoy this kind of review process :)

(Last time I tried to implement something bigger was for 2.12.. and it's still sitting in the bugzilla :P)

The patch now looks good to me. If you have an account feel free to commit to SVN (head), otherwise ask me or someone alse to that for you (but I will only be able to do it tomorrow). Just add a ChangeLog entries where requested and fix the little indentation issue with the .xml.in file (just 2 spaces indentation, like all other entries) before committing the patches.
Thanks for taking care of this.

In trunk now... Closing :)

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

Ops, Priit, I spotted an error in your patch. In do_media_action you retrieve the eject_command instead of the correct one. Please fix it as soon as possible.

"/desktop/gnome/applications/media/exec" is right place?

Looking at your patch it seems so :) btw, I think you should also handle the needs_term case.

Created attachment 85052 [details] [review]
gcc-gsd-needs-terminal.patch

Ok, seems that needs_terminal setting wasn't checked at all for browser and mailer...

IMHO, it needs a bit reworking, but I couldn't find better way now.

I spotted just a couple of style issues, like a missing space between function name and opening "(", but for a code review I would wait for one of the maintainers of gnome-settings-daemon.

(In reply to comment #21)
> Created an attachment (id=85052) [edit]
> gcc-gsd-needs-terminal.patch

Apart from the style issues Luca mentioned:

+	cmd_term = gconf_client_get_string (acme->conf_client,
+				   "/desktop/gnome/applications/terminal/exec", NULL);
+	if ((cmd_term == NULL) || (strcmp (cmd_term, "") == 0)) 
+		return NULL;

Leaking cmd_term if "". Same with cmd_args.

+		term = get_term_command(acme);
+		if ((term == NULL) || (strcmp(term, "") == 0)) {

Check for strcmp == "" is not necessary here (and would leak again).

		if ((term == NULL) || (strcmp(term, "") == 0)) {
+			acme_error(_("Could not get default terminal."
+				   "Verify that your default terminal setting points to valid application."));
+			return;

Same leak here. The error message is missing a space.

+	if (term != NULL) {
+		exec = g_strdup_printf("%s%s", term, cmd);
+		g_free (exec);

Huh? Also, don't you need a space in the "%s%s"?

+	if ((command == NULL) || (strcmp (command, "") == 0))
+		return;

Leaking once again.

Created attachment 85095 [details] [review]
gcc-gsd-needs-terminal-v2.patch

Ok, I have plugged now all these " if ((command == NULL) || (strcmp (command, "") == 0)) " one byte leaks and fixed the mentioned style issues.

Indentation is still problematic in places. If you have multiline commands,
the convention used in cc code is to indent the second (and following) line(s)
by the position of the opening bracket, e.g.

        g_printf ("long line lala blalbla %s blabla %s %s",
                  some_string, some_other_string, last_string);


+	if ((cmd_term == NULL) || (strcmp (cmd_term, "") == 0)) 
+		return NULL;

Still leaks. In at least some of these cases you would get somewhat cleaner
code if you turned the logic around, i.e. instead of

        if ((string == NULL) || (strcmp (string, "") == 0)) {
               g_free (string);
               return;
        }

        // .... do something else

       g_free (string);

use

        if ((string != NULL) && (strcmp (string, "") != 0)) {

                // do something else

        }

        g_free (string);




+       if (need_term) {
+		term = get_term_command (acme);
+		if (term == NULL) {
+			acme_error (_("Could not get default terminal. "
+				   "Verify that your default terminal setting points to valid application."));
+			return;
+	}

Seems to lack a closing }. The error message would benefit from adding "... to a valid...", I think.

 		msg = g_strdup_printf
 			(_("Couldn't execute command: %s\n"
 			   "Verify that this command exists."),
-			 cmd);
+			 exec);

Since you're now exposing the complete command including args, changing the message to
something like "Verify that this is a valid command." would be a good idea.

Created attachment 85400 [details] [review]
gcc-gsd-needs-terminal-v3.patch

Third try..
  * Mem leak should be plugged now
  * Fixes indenting
  * Changes error msg

do_{unknown,help,mail,media,www}_action still look to me like they'd benefit from turning the empty string logic around.

Otherwise looks good.

(One really really tiny nitpick: in get_term_command cmd_args is only used in a limited scope; that's where the variable should be declared.)

Created attachment 85404 [details] [review]
gcc-gsd-needs-terminal-v4.patch

Patch with turned around logic and limited scope variables ;)

Tsk, tsk, tsk.

You introduced a few cases of g_free() (note missing space!) in this last patch.
If you fix that, I think I've harrassed you long enough. ;-)

Created attachment 85502 [details] [review]
gcc-gsd-needs-terminal-v5.patch

Fixes the two g_free() instances ;)

Nice :)

Committed revision 7429.

2007-03-29  Priit Laes  <amd@store20.com>

    * gnome-settings-multimedia-keys.c: (get_term_command), (execute),
    (do_unknown_action), (do_help_action), (do_mail_action),
    (do_media_action), (do_www_action), (do_exit_action), (do_eject_action),
    (do_action): Fix launch application in terminal support. (closes bug
    #362578).

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