indicator-power

Merge lp://staging/~charlesk/indicator-power/lp-1234458 into lp://staging/indicator-power/14.04

lp-1234458
Merge into trunk.14.04

Proposed by Charles Kerr on 2014-03-05

Status:

Merged

Merged at revision:

233

Proposed branch:

lp://staging/~charlesk/indicator-power/lp-1234458

Merge into:

lp://staging/indicator-power/14.04

Diff against target:

1086 lines (+524/-299)

5 files modified

src/device.c (+311/-195)
src/device.h (+21/-16)
src/service.c (+63/-46)
tests/Makefile.am (+1/-1)
tests/test-device.cc (+128/-41)

To merge this branch:

bzr merge lp://staging/~charlesk/indicator-power/lp-1234458

Low

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration		Approve on 2014-03-13
Lars Karlitski (community)		2014-03-05	Approve on 2014-03-13
Review via email: mp+209371@code.staging.launchpad.net

Commit message

Change the implementation of the title text / title accessible text / menuitem text to follow the spec changes in https://wiki.ubuntu.com/Power?action=diff&rev2=44&rev1=43#Title and update the tests accordingly.

Description of the change

Merge Review:

> Ensure the project compiles and the test suite executes without error

Yes

> Ensure that non-obvious code has comments explaining it

Lots of comments. Most of the spec's new text is in device.c's comments...

> If the change works on specific profiles, please include those in the merge description.

Is not profile-specific

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-05:

PASSED: Continuous integration, rev:230
http://jenkins.qa.ubuntu.com/job/indicator-power-ci/84/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-power-trusty-amd64-ci/21
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-power-trusty-armhf-ci/21
        deb: http://jenkins.qa.ubuntu.com/job/indicator-power-trusty-armhf-ci/21/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

Revision history for this message

Lars Karlitski (larsu) wrote on 2014-03-13:

Today I learned the word 'inestimable'.

I bet you saw this question coming: Why the static sizes for strings? It's unlikely that any language will need more than 128 bytes (in some places I saw 64?) for those labels, I would feel much better if you'd just use g_strdup_printf().

There are already some (very minor) bugs related to that. For example, get_brief_time_remaining() doesn't check the 'size > 0' precondition and might write a \0 into the first byte.

Also, the readability of some functions would benefit from using GString to construct strings. get_accessible_time_remaining() has two blocks that create almost the same string except for the last part. Is this so that the full strings are translatable? I thought we tried not to put format specifiers into translatable strings. Are there languages that would have the "to charge" before the time?

review: Needs Information

lp://staging/~charlesk/indicator-power/lp-1234458 updated on 2014-03-13

231. By Charles Kerr on 2014-03-13: in the new indicator_power_device_get_*() functions, use heap-allocated strings rather than relying on g_snprintf().

Revision history for this message

Charles Kerr (charlesk) wrote on 2014-03-13:

> For example, get_brief_time_remaining() doesn't check the 'size > 0' precondition and might write a \0 into the first byte.

get_brief_time_remaining() is a private function; all the sanity checks have already been run by the public-facing API before get_brief_time_remaining() is reached.

Still, I agree with your main point -- there's no compelling reason for static sizes here.

Fixed r231

> Also, the readability of some functions would benefit from using GString to construct strings. get_accessible_time_remaining() has two blocks that create almost the same string except for the last part. Is this so that the full strings are translatable? I thought we tried not to put format specifiers into translatable strings.

I'd be happy to learn from this if I'm wrong, but that's not my understanding at all. IIUC it's preferable to give whole strings to the translators when it's reasonable to do so, and to not second-quess what they'll need. Since get_accessible_time_remaining() only had four combinations, I went ahead and wrote them all out long-form.

lp://staging/~charlesk/indicator-power/lp-1234458 updated on 2014-03-13

232. By Charles Kerr on 2014-03-13: remove INDICATOR_IS_POWER_DEVICE(device) tests from private functions
233. By Charles Kerr on 2014-03-13: copyediting: add const, fix misaligned whitespace

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-13:

FAILED: Continuous integration, rev:231
http://jenkins.qa.ubuntu.com/job/indicator-power-ci/89/
Executed test runs:
FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-power-trusty-amd64-ci/26/console
FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-power-trusty-armhf-ci/26/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Lars Karlitski (larsu) wrote on 2014-03-13:

> > For example, get_brief_time_remaining() doesn't check the 'size > 0'
> precondition and might write a \0 into the first byte.
>
> get_brief_time_remaining() is a private function; all the sanity checks have
> already been run by the public-facing API before get_brief_time_remaining() is
> reached.

Ah, I didn't notice that.

> Still, I agree with your main point -- there's no compelling reason for static
> sizes here.
>
> Fixed r231

Cool, thanks.

> I'd be happy to learn from this if I'm wrong, but that's not my understanding
> at all. IIUC it's preferable to give whole strings to the translators when
> it's reasonable to do so, and to not second-quess what they'll need. Since
> get_accessible_time_remaining() only had four combinations, I went ahead and
> wrote them all out long-form.

Yeah. It's not that important. I just wanted to mention it.

Thanks!

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-13:

FAILED: Continuous integration, rev:233
http://jenkins.qa.ubuntu.com/job/indicator-power-ci/90/
Executed test runs:
FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-power-trusty-amd64-ci/27/console
FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-power-trusty-armhf-ci/27/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-13:

FAILED: Continuous integration, rev:231
http://jenkins.qa.ubuntu.com/job/indicator-power-ci/91/
Executed test runs:
FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-power-trusty-amd64-ci/28/console
FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-power-trusty-armhf-ci/28/console

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

review: Needs Fixing (continuous-integration)

lp://staging/~charlesk/indicator-power/lp-1234458 updated on 2014-03-13

234. By Charles Kerr on 2014-03-13: in the new indicator_power_device_get_*() functions, use g_return_val_if_fail(foo, NULL) rather than g_return_if_fail(foo)
235. By Charles Kerr on 2014-03-13: when expecting a NULL string, use EXPECT_EQ(NULL, str)