libindicator

Merge lp://qastaging/~larsu/libindicator/new-indicator-file-format into lp://qastaging/libindicator/13.10

  1. new-indicator-file-format
  2. Merge into trunk.13.10
Proposed by Ted Gould
Status: Merged
Approved by: Ted Gould
Approved revision: 491
Merge reported by: Lars Karlitski
Merged at revision: not available
Proposed branch: lp://qastaging/~larsu/libindicator/new-indicator-file-format
Merge into: lp://qastaging/libindicator/13.10
Prerequisite: lp://qastaging/~larsu/libindicator/call-ido-init
Diff against target: 295 lines (+109/-43)
6 files modified
README (+35/-0)
libindicator/indicator-ng.c (+63/-34)
tests/com.canonical.indicator.no-such-service (+0/-1)
tests/com.canonical.indicator.test (+3/-1)
tests/test-indicator-ng.c (+5/-5)
tools/indicator-loader.c (+3/-2)
To merge this branch: bzr merge lp://qastaging/~larsu/libindicator/new-indicator-file-format
Reviewer Review Type Date Requested Status
Ted Gould (community) Approve
PS Jenkins bot (community) continuous-integration Needs Fixing
Review via email: mp+165738@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2013-05-24.

Commit message

IndicatorNg: update indicator file format

The old file format had some shortcomings:

(1) It was impossible to efficiently reuse a menu for different profiles,
because the profile name was implicit in the object path. The only way to do
this was to export the same menu twice. Now, object paths have to be set
explicitly in the indicator file.

(2) The well-known dbus name of a service and the name of its service file were
similar but slightly different (com.canonical.indicator.test vs
com.canonical.test.indicator), which caused some confusion on when to use
which. Now, the file name *is* the bus name, and the `BusName` key has been
dropped.

The new file format is documented in README.

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

FAILED: Continuous integration, rev:491
http://jenkins.qa.ubuntu.com/job/libindicator-ci/17/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/libindicator-saucy-amd64-ci/4/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/libindicator-saucy-armhf-ci/3/console

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

review: Needs Fixing (continuous-integration)
Revision history for this message
Lars Karlitski (larsu) wrote : Posted in a previous version of this proposal

Note: Jenkins fails because -Werror is stupid.

I fixed this as part of another merge request on libindicator (lp:~larsu/libindicator/call-ido-init). Let's merge that one first.

Revision history for this message
Ted Gould (ted) wrote :

Resubmitted to make it explicitly depend on that branch so that Jenkins knows to land them in order.

Revision history for this message
Ted Gould (ted) wrote : Posted in a previous version of this proposal

It seems odd to me to check state using g_assert() instead of g_return_if_fail(). Both generate critical errors, but one doesn't crash the whole panel service. I don't see the benefit of crashing the whole panel service.

review: Needs Information
Revision history for this message
Ted Gould (ted) wrote :

It seems odd to me to check state using g_assert() instead of g_return_if_fail(). Both generate critical errors, but one doesn't crash the whole panel service. I don't see the benefit of crashing the whole panel service.

review: Needs Information
Revision history for this message
Ted Gould (ted) wrote : Posted in a previous version of this proposal

Uhg, wrong tab :-(

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

FAILED: Continuous integration, rev:491
http://jenkins.qa.ubuntu.com/job/libindicator-ci/19/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/libindicator-saucy-amd64-ci/6/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/libindicator-saucy-armhf-ci/5/console

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

review: Needs Fixing (continuous-integration)
Revision history for this message
Lars Karlitski (larsu) wrote :

Ted, which assert are you referring to? I think all the ones I put in there are only for checking internal consistency of the object, which I wouldn't feel comfortable turning into a g_return_if_fail (especially since we don't have fatal criticals enabled).

Revision history for this message
Ted Gould (ted) wrote :

On Mon, 2013-05-27 at 16:36 +0000, Lars Uebernickel wrote:

> Ted, which assert are you referring to? I think all the ones I put in
> there are only for checking internal consistency of the object, which
> I wouldn't feel comfortable turning into a g_return_if_fail
> (especially since we don't have fatal criticals enabled).

I guess I agree with why they're there, I just don't think a library
should crash it's parent process. Instead can we put them as criticals
and enable fatal criticals for the test suite? That should protect
things while making sure a real running system doesn't end up in a bad
state.

Revision history for this message
Ted Gould (ted) :
review: Approve
lp://qastaging/~larsu/libindicator/new-indicator-file-format updated
492. By Lars Karlitski

indicator-ng: fix crash

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