libindicator

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

  1. shortcut-path-key
  2. Merge into trunk.13.10
Proposed by Marco Trevisan (Treviño)
Status: Merged
Approved by: Charles Kerr
Approved revision: 512
Merged at revision: 509
Proposed branch: lp://staging/~3v1n0/libindicator/shortcut-path-key
Merge into: lp://staging/libindicator/13.10
Diff against target: 81 lines (+28/-3)
1 file modified
libindicator/indicator-desktop-shortcuts.c (+28/-3)
To merge this branch: bzr merge lp://staging/~3v1n0/libindicator/shortcut-path-key
Reviewer Review Type Date Requested Status
Charles Kerr (community) Approve
PS Jenkins bot (community) continuous-integration Approve
Ted Gould (community) Needs Fixing
Review via email: mp+179149@code.staging.launchpad.net

Commit message

IndicatorDesktopShortcuts: add support to Path key for shortcut items

Description of the change

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.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

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)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

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)
Revision history for this message
Ted Gould (ted) wrote :

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

review: Needs Fixing
lp://staging/~3v1n0/libindicator/shortcut-path-key updated
510. By Marco Trevisan (Treviño)

IndicatorDesktopShortcuts: use g_key_file_get_string for path

Revision history for this message
Marco Trevisan (Treviño) (3v1n0) wrote :

> 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...

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

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)
Revision history for this message
Charles Kerr (charlesk) wrote :

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://staging/~3v1n0/libindicator/shortcut-path-key updated
511. By Marco Trevisan (Treviño)

IndicatorDesktopShortcuts: restore previous working dir if we changed it

Revision history for this message
Marco Trevisan (Treviño) (3v1n0) wrote :

> 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://staging/~3v1n0/libindicator/shortcut-path-key updated
512. By Marco Trevisan (Treviño)

IndicatorDesktopShortcut: fix indentation

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

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)
Revision history for this message
Charles Kerr (charlesk) wrote :

>> 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. :-)

Revision history for this message
Charles Kerr (charlesk) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches