The Sound Menu

Merge lp://staging/~3v1n0/indicator-sound/bamf-player-activation into lp://staging/indicator-sound/13.04

  1. bamf-player-activation
  2. Merge into trunk.13.04
Proposed by Marco Trevisan (Treviño)
Status: Merged
Approved by: Lars Karlitski
Approved revision: 351
Merged at revision: 347
Proposed branch: lp://staging/~3v1n0/indicator-sound/bamf-player-activation
Merge into: lp://staging/indicator-sound/13.04
Prerequisite: lp://staging/~3v1n0/indicator-sound/gtk-application-player-activate
Diff against target: 243 lines (+100/-19)
6 files modified
configure.ac (+2/-0)
debian/control (+1/-0)
src/Makefile.am (+3/-1)
src/metadata-menu-item.vala (+1/-1)
src/player-activator.vala (+91/-15)
src/player-controller.vala (+2/-2)
To merge this branch: bzr merge lp://staging/~3v1n0/indicator-sound/bamf-player-activation
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve
Lars Karlitski (community) Approve
Mathieu Trudel-Lapierre Abstain
Review via email: mp+156920@code.staging.launchpad.net

This proposal supersedes a proposal from 2013-04-02.

Commit message

PlayerActivator: Use BAMF to find the windows to activate with timestamp

Improved the"old" GtkApplicationPlayer, using BAMF as a fallback method to
activate an application's windows. Basically we try to get the windows of the
selected application and when found we focus them using the activation
timestamp.

Description of the change

Added a new fallback method to focus the windows of the player that we need to raise using the event timestamp, if we're not using a GtkApplication based player, we try to get the player windows from Bamf.
At that point we can focus them with the activation event timestamp.

This will work well for banshee, Spotify, Youtube and any other local or web-player.

I've added a new dependency to libbamf3. If this is a problem for packaing stuff, I can provide a version that uses bamf directly via DBus, but I'd really prefer to use the library.

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

PASSED: Continuous integration, rev:351
http://jenkins.qa.ubuntu.com/job/indicator-sound-ci/12/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-sound-raring-amd64-ci/1
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-sound-raring-armhf-ci/1

Click here to trigger a rebuild:
http://s-jenkins:8080/job/indicator-sound-ci/12/rebuild

review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:351
http://jenkins.qa.ubuntu.com/job/indicator-sound-13.04-ci/1/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-sound-13.04-raring-amd64-ci/1
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-sound-13.04-raring-armhf-ci/1

Click here to trigger a rebuild:
http://s-jenkins:8080/job/indicator-sound-13.04-ci/1/rebuild

review: Approve (continuous-integration)
Revision history for this message
Mathieu Trudel-Lapierre (cyphermox) wrote :

This is quite complex and I would have rather it not been including a rename of the file, since it's not necessary to rename to fix the bug..

Abstain. For the debian/control file it seems fine, too, and Jenkins is happy with it.

review: Abstain
Revision history for this message
Mathieu Trudel-Lapierre (cyphermox) wrote :

In other words, let's get someone else to review the code just to be safe.

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

Mathieu, yes I've asked a review already to Larsu.

However, about the file renaming, it's a file that it's not in trunk yet... I've just a file I've added in the prerequisite branch that I've renamed to be more general.

So, see these two branches as one split in two for reviewing.

Revision history for this message
Lars Karlitski (larsu) wrote :

Looks good to me, thanks Marco.

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) :
review: Approve (continuous-integration)

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