From: Richard Quirk <richard.quirk_at_gmail.com>
Date: Sat, 06 Oct 2012 19:12:59 +0200

On 10/06/2012 03:22 PM, Dominik Riebeling wrote:
> On Sat, Oct 6, 2012 at 2:16 PM, Richard Quirk <richard.quirk_at_gmail.com> wrote:
>> Thanks for reviewing that. Unfortunately I missed a set_sleep_timer in 327,
>> but did not spot the compiler output as I had 327 and 328 staged on the same
>> branch. Clean up here http://gerrit.rockbox.org/329 :-(
> Too late, since this broke pretty much all builds I fixed in the meantime.

Yeah saw that too late. Thanks, and sorry.

>
> However, there are some things I don't like about the original patch:
> 1. previously it's been set_sleep_timer, now it's
> set_sleeptimer_duration. Note that it changed from sleep_timer to
> sleeptimer.

This is mainly so the 2nd part follows the common convention. The other
menu settings that change setting xxx have set_xxx in the callback name.
The setting is called sleeptimer_duration.

> 2. set_sleeptimer_duration doesn't indicate in any way what's
> different to set_sleep_timer (which is still present). IMO it would be
> better to have functions indicating the unit used, i.e. change both
> and use set_sleep_timer_seconds and set_sleep_timer_minutes (or maybe
> abbreviated as _sec and _min).

True. Though there's still only 1 visible function externally.

>
> I didn't comment on that earlier since I'm usually not involved in
> that part of the code ...
>
>
> - Dominik
Received on 2012-10-06