The Sound Menu

Merge lp://qastaging/~xavi-garcia-mena/indicator-sound/last-running-player-accounts-service into lp://qastaging/indicator-sound/15.10

  1. last-running-player-accounts-service
  2. Merge into trunk.15.10
Proposed by Xavi Garcia
Status: Merged
Approved by: Charles Kerr
Approved revision: 537
Merged at revision: 530
Proposed branch: lp://qastaging/~xavi-garcia-mena/indicator-sound/last-running-player-accounts-service
Merge into: lp://qastaging/indicator-sound/15.10
Diff against target: 2481 lines (+1148/-895)
16 files modified
data/com.canonical.indicator.sound.gschema.xml (+2/-13)
src/CMakeLists.txt (+22/-11)
src/accounts-service-access.vala (+235/-0)
src/main.c (+63/-61)
src/service.vala (+15/-8)
src/sound-menu.vala (+35/-13)
src/volume-control-pulse.vala (+12/-141)
tests/integration/indicator-sound-test-base.cpp (+29/-5)
tests/integration/indicator-sound-test-base.h (+4/-0)
tests/integration/test-indicator.cpp (+69/-12)
tests/notifications-test.cc (+568/-558)
tests/service-mocks/accounts-mock/AccountsServiceSoundMock.cpp (+15/-0)
tests/service-mocks/accounts-mock/AccountsServiceSoundMock.h (+4/-0)
tests/service-mocks/accounts-mock/com.ubuntu.AccountsService.Sound.Mock.xml (+1/-0)
tests/sound-menu.cc (+16/-16)
tests/volume-control-test.cc (+58/-57)
To merge this branch: bzr merge lp://qastaging/~xavi-garcia-mena/indicator-sound/last-running-player-accounts-service
Reviewer Review Type Date Requested Status
Charles Kerr (community) Approve
PS Jenkins bot (community) continuous-integration Approve
Review via email: mp+286897@code.qastaging.launchpad.net

Commit message

This branch sets the last running player using accounts service instead of gsettings.
It also includes a new class AccountsServiceAccess, to centralize all accesses to account service properties.

Description of the change

This branch sets the last running player using accounts service instead of gsettings.
It also includes a new class AccountsServiceAccess, to centralize all accesses to account service properties.

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

PASSED: Continuous integration, rev:532
http://jenkins.qa.ubuntu.com/job/indicator-sound-ci/312/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-sound-wily-amd64-ci/87
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-sound-wily-armhf-ci/88
        deb: http://jenkins.qa.ubuntu.com/job/indicator-sound-wily-armhf-ci/88/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)
lp://qastaging/~xavi-garcia-mena/indicator-sound/last-running-player-accounts-service updated
533. By Xavi Garcia

Fixed integration test

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

PASSED: Continuous integration, rev:533
http://jenkins.qa.ubuntu.com/job/indicator-sound-ci/313/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-sound-wily-amd64-ci/88
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-sound-wily-armhf-ci/89
        deb: http://jenkins.qa.ubuntu.com/job/indicator-sound-wily-armhf-ci/89/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)
lp://qastaging/~xavi-garcia-mena/indicator-sound/last-running-player-accounts-service updated
534. By Xavi Garcia

Removed log statement

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

PASSED: Continuous integration, rev:534
http://jenkins.qa.ubuntu.com/job/indicator-sound-ci/314/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-sound-wily-amd64-ci/89
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-sound-wily-armhf-ci/90
        deb: http://jenkins.qa.ubuntu.com/job/indicator-sound-wily-armhf-ci/90/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)
lp://qastaging/~xavi-garcia-mena/indicator-sound/last-running-player-accounts-service updated
535. By Xavi Garcia

Accounts service notification fix

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

PASSED: Continuous integration, rev:535
http://jenkins.qa.ubuntu.com/job/indicator-sound-ci/315/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-sound-wily-amd64-ci/90
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-sound-wily-armhf-ci/91
        deb: http://jenkins.qa.ubuntu.com/job/indicator-sound-wily-armhf-ci/91/artifact/work/output/*zip*/output.zip

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

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

A lot of this looks great. I like the improved tests and the refactoring of AccountsServices into its own class.

Several questions inline though, with a couple of serious issues eg the way GCancellable is used and my questions about the use of last_player_added in SoundMenu.

review: Needs Information
lp://qastaging/~xavi-garcia-mena/indicator-sound/last-running-player-accounts-service updated
536. By Xavi Garcia

Changed following Charles's suggestions

Revision history for this message
Xavi Garcia (xavi-garcia-mena) wrote :

Thanks for the review, Charles!

I assumed the existing code had passed already a code review in deep and did not refactored it. Big mistake.

I've fixed almost everything you commented. I think there's only one point remaining... please take a look at the inline comments.

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

PASSED: Continuous integration, rev:536
http://jenkins.qa.ubuntu.com/job/indicator-sound-ci/317/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-sound-wily-amd64-ci/92
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-sound-wily-armhf-ci/93
        deb: http://jenkins.qa.ubuntu.com/job/indicator-sound-wily-armhf-ci/93/artifact/work/output/*zip*/output.zip

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

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

Ah, I didn't realize that GCancellable issue was a carryover from old code. I'm glad it's fixed.

One last suggestion, I do think AccountsServiceAccess should have a private GCancellable member that gets passed to all these bus calls + a call to cancellable.cancel() in the AccountsServiceAccess destructor. This won't affect general use since the object's lifespan continues for the life of the process, but it's useful to prevent issues when shutting down the bus in unit tests.

Nice fix wrt the cascading signal emission.

lp://qastaging/~xavi-garcia-mena/indicator-sound/last-running-player-accounts-service updated
537. By Xavi Garcia

Added Cancellable to AccountsServiceAccess

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

PASSED: Continuous integration, rev:537
http://jenkins.qa.ubuntu.com/job/indicator-sound-ci/318/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-sound-wily-amd64-ci/93
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-sound-wily-armhf-ci/94
        deb: http://jenkins.qa.ubuntu.com/job/indicator-sound-wily-armhf-ci/94/artifact/work/output/*zip*/output.zip

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

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

Good stuff.

I won't block this patch further because this is a preexisting condition, but if you want to finish making AccountsServiceAccess' DBus calls cancellable there are still a handful of noncancellable calls:

accounts-service-access.vala:133: yield DBusProxy.create_for_bus()
accounts-service-access.vala:141: yield accounts_proxy.call()
accounts-service-access.vala:145: yield DBusProxy.create_for_bus()
accounts-service-access.vala:158: yield _user_proxy.get_connection ().call()
accounts-service-access.vala:181: yield Bus.get_proxy()

In (I think) all of these cases the cancellable is passed in as the invisible last argument, where vala has a C++-like default argument which is null.

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