The Sound Menu

Merge lp://qastaging/~mitya57/indicator-sound/lp1502480 into lp://qastaging/indicator-sound/15.10

  1. lp1502480
  2. Merge into trunk.15.10
Proposed by Dmitry Shachnev
Status: Merged
Approved by: Sebastien Bacher
Approved revision: 536
Merged at revision: 536
Proposed branch: lp://qastaging/~mitya57/indicator-sound/lp1502480
Merge into: lp://qastaging/indicator-sound/15.10
Diff against target: 53 lines (+22/-6)
1 file modified
src/service.vala (+22/-6)
To merge this branch: bzr merge lp://qastaging/~mitya57/indicator-sound/lp1502480
Reviewer Review Type Date Requested Status
Charles Kerr (community) Approve
Alkis Georgopoulos (community) Approve
Mystic-Mirage (community) Approve
Alberts Muktupāvels Approve
Sebastien Bacher Pending
Ted Gould Pending
Lars Karlitski Pending
Review via email: mp+290144@code.qastaging.launchpad.net

Commit message

Support multiple desktop names in $XDG_CURRENT_DESKTOP.

Description of the change

According to the Desktop Entry specification:

If $XDG_CURRENT_DESKTOP is set then it contains a colon-separated list of strings. In order, each string is considered.

This branch adds support for proper detecting of Unity-derived desktops. It is mainly needed for GNOME Flashback session, which uses unity-control-center and XDG_DESKTOP_NAME=GNOME-Flashback:Unity.

To post a comment you must log in.
Revision history for this message
Alberts Muktupāvels (muktupavels) :
review: Approve
Revision history for this message
Mystic-Mirage (mystic-mirage) :
review: Approve
Revision history for this message
Alkis Georgopoulos (alkisg) wrote :

It looks fine to me.

In shell, to avoid the loop, I do:
if echo ":$XDG_CURRENT_DESKTOP:" | grep -q ":Unity:"; then
    echo yes
else
    echo no
fi

Performance-wise it's fine, it needs only one malloc (for strcat) instead of many (for split), but I'm not sure how readable it is.

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

Not a blocker, but I think the standalone ``is_unity()'' function makes sense here as well as in indicator-datetime.

Two reasons:
1. readability: (is_unity() && (Environment.find_program_in_path ("unity-control-center") != null))
2. the ability of is_unity() to cache its value s.t. it doesn't have to re-evaluate each time it's called

review: Approve
lp://qastaging/~mitya57/indicator-sound/lp1502480 updated
537. By Dmitry Shachnev

Move Unity detection into a separate function, and add caching

Revision history for this message
Dmitry Shachnev (mitya57) wrote :

Splitted the code into a separate function and added caching, please check. I'm not fluent in Vala, so if that could be done simplier, please let me know.

Revision history for this message
Charles Kerr (charlesk) wrote :

LGTM.

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