Merge lp:~3v1n0/libindicator/shortcut-path-key into lp:libindicator/13.10

[Marco Trevisan (Treviño)]

IndicatorDesktopShortcuts: add support to Path key for shortcut items

If the .desktop action provides a Path key, change working directory
to that before executing the action. Unfortunately GAppInfo doesn't
provide a such facility when creating command line applications, but
it does exactly like this internally.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:509
http://jenkins.qa.ubuntu.com/job/libindicator-ci/39/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/libindicator-saucy-amd64-ci/26/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/libindicator-saucy-armhf-ci/25/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/libindicator-ci/39/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:509
http://jenkins.qa.ubuntu.com/job/libindicator-ci/40/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libindicator-saucy-amd64-ci/27
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libindicator-saucy-armhf-ci/26

Click here to trigger a rebuild:
http://s-jenkins:8080/job/libindicator-ci/40/rebuild

review: Approve (continuous-integration)

[Ted Gould (ted)]

I don't think that the path can be translated, so this should be g_key_file_get_string().

review: Needs Fixing

[lp:~3v1n0/libindicator/shortcut-path-key]

510. By Marco Trevisan (Treviño) on 2013-08-09

IndicatorDesktopShortcuts: use g_key_file_get_string for path

[Marco Trevisan (Treviño) (3v1n0)]

> I don't think that the path can be translated, so this should be
> g_key_file_get_string().

Oh My... Right sorry. Blind copy&paste! :P

Fixed...

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:510
http://jenkins.qa.ubuntu.com/job/libindicator-ci/41/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libindicator-saucy-amd64-ci/28
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libindicator-saucy-armhf-ci/27

Click here to trigger a rebuild:
http://s-jenkins:8080/job/libindicator-ci/41/rebuild

review: Approve (continuous-integration)

[Charles Kerr (charlesk)]

Thanks for writing this patch. I see one minor problem and also have a question:

Needs Fixing:

1. This patch introduces a side-effect to the function: the cwd is still changed when the function returns. It would be safer to remove this side-effect by wrapping the block in

  char * current_dir = g_get_current_dir();

  ...

  g_chdir (current_dir);
  g_free (current_dir);

Comment only:

2. Putting four g_free() statements on a single line really itches. :-) Please use a newline between each.

3. Should the chdir() call in the patch be a g_chdir()? I think the answer is 'no' but I'm unsure.

review: Needs Fixing

[lp:~3v1n0/libindicator/shortcut-path-key]

511. By Marco Trevisan (Treviño) on 2013-08-22

IndicatorDesktopShortcuts: restore previous working dir if we changed it

[Marco Trevisan (Treviño) (3v1n0)]

> Thanks for writing this patch. I see one minor problem and also have a
> question:
>
> Needs Fixing:
>
> 1. This patch introduces a side-effect to the function: the cwd is still
> changed when the function returns. It would be safer to remove this side-
> effect by wrapping the block in
>
> char * current_dir = g_get_current_dir();
>
> ...
>
> g_chdir (current_dir);
> g_free (current_dir);

Yes, I thought about that... But I didn't do that as it was the same way as g_spawn* does for example... However, it also sounds better to me. So here it is!

> 2. Putting four g_free() statements on a single line really itches. :-) Please
> use a newline between each.

Yes, right... I just kept the old style.

> 3. Should the chdir() call in the patch be a g_chdir()? I think the answer is
> 'no' but I'm unsure.

It's just the same, and AFAIK we won't use this but in linux, it should be the same :)
(it also would need to include something else).

[lp:~3v1n0/libindicator/shortcut-path-key]

512. By Marco Trevisan (Treviño) on 2013-08-22

IndicatorDesktopShortcut: fix indentation

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:512
http://jenkins.qa.ubuntu.com/job/libindicator-ci/42/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libindicator-saucy-amd64-ci/29
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libindicator-saucy-armhf-ci/28

Click here to trigger a rebuild:
http://s-jenkins:8080/job/libindicator-ci/42/rebuild

review: Approve (continuous-integration)

[Charles Kerr (charlesk)]

>> 2. Putting four g_free() statements on a single line really itches. :-) Please use a newline between each.
>
> Yes, right... I just kept the old style.

Right, I didn't mean to imply you started it. IIRC I may have added a g_free() at the end of that line too. If so, I was wrong. :-)

[Charles Kerr (charlesk)]

review: Approve

=== modified file 'libindicator/indicator-desktop-shortcuts.c'
--- libindicator/indicator-desktop-shortcuts.c	2013-07-17 19:47:48 +0000
+++ libindicator/indicator-desktop-shortcuts.c	2013-08-22 16:58:11 +0000
@@ -571,6 +571,7 @@
 indicator_desktop_shortcuts_nick_exec_with_context (IndicatorDesktopShortcuts * ids, const gchar * nick, GAppLaunchContext * launch_context)
 {
 	GError * error = NULL;
+	gchar * current_dir = NULL;
 
 	g_return_val_if_fail(INDICATOR_IS_DESKTOP_SHORTCUTS(ids), FALSE);
 	IndicatorDesktopShortcutsPrivate * priv = INDICATOR_DESKTOP_SHORTCUTS_GET_PRIVATE(ids);
@@ -612,6 +613,22 @@
 		return FALSE;
 	}
 
+	/* If possible move to the proper launch path */
+	gchar * path = g_key_file_get_string(priv->keyfile, groupheader,
+	                                     G_KEY_FILE_DESKTOP_KEY_PATH, NULL);
+
+	if (path && *path != '\0') {
+		current_dir = g_get_current_dir();
+
+		if (chdir(path) < 0) {
+			g_warning("Impossible to run action '%s' from path '%s'", nick, path);
+			g_free(current_dir);
+			g_free(groupheader);
+			g_free(path);
+			return FALSE;
+		}
+	}
+
 	/* Grab the name and the exec entries out of our current group */
 	gchar * name = g_key_file_get_locale_string(priv->keyfile,
 	                                            groupheader,
@@ -625,8 +642,6 @@
 	                                            NULL,
 	                                            NULL);
 
-	g_free(groupheader);
-
 	GAppInfoCreateFlags flags = G_APP_INFO_CREATE_NONE;
 
 	if (launch_context) {
@@ -634,26 +649,36 @@
 	}
 
 	GAppInfo * appinfo = g_app_info_create_from_commandline(exec, name, flags, &error);
-	g_free(name); g_free(exec);
+	g_free(groupheader);
+	g_free(path);
+	g_free(name);
+	g_free(exec);
 
 	if (error != NULL) {
 		g_warning("Unable to build Command line App info: %s", error->message);
+		g_free(current_dir);
 		g_error_free(error);
 		return FALSE;
 	}
 
 	if (appinfo == NULL) {
 		g_warning("Unable to build Command line App info (unknown)");
+		g_free(current_dir);
 		return FALSE;
 	}
 
 	gboolean launched = g_app_info_launch(appinfo, NULL, launch_context, &error);
 
+	if (current_dir && chdir(current_dir) < 0)
+		g_warning("Impossible to switch back to default work dir");
+
+
 	if (error != NULL) {
 		g_warning("Unable to launch file from nick '%s': %s", nick, error->message);
 		g_clear_error(&error);
 	}
 
+	g_free(current_dir);
 	g_object_unref(appinfo);
 
 	return launched;