On 10/06/2012 11:54 AM, Jonathan Gordon wrote:
> On 6 October 2012 05:47, Richard Quirk <richard.quirk_at_gmail.com> wrote:
>> A couple of patches to do with the default sleep duration setting, the first
>> adds a "set_sleeptimer_duration" function that will be used in the second.
>> My use case is: I have restart sleep timer when switching on set to "yes",
>> and reset timer when touching any buttons set to "yes". At night, I set the
>> sleep timer to 15 minutes, and fall asleep at some point listening to music
>> (maybe switching it back on if I don't fall asleep right away). Then in the
>> morning I set the timer to a large value that it'll probably never have
>> chance to reach during the day. That way I don't have to touch any other
>> setting to activate/deactivate the sleep timer.
> 327 looks sensible enough.. 328 doesn't seem like the correct fix
> though, need to understand the issue more to comment.
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 :-(
I'll describe the issue in 328 as I see it. I agree that losing the
functionality is not ideal and there's hopefully a better way to do it.
The callback in apps/menus/settings_menu.c, sleeptimer_duration_cb, is
set at the menu level and is called when you perform any action on the
General Settings > Startup/Shutdown menu. When you select the setting
Default Sleep Timer, the code calls the sleeptimer_duration_cb with
ACTION_ENTER_MENUITEM and so the current setting is stored in the static
variable. When you finish with the settings and leave, the callback is
called once again with EXIT. If you have changed the value of the
global_settings.sleeptimer_duration then the actual running timer value
is updated via the call to set_sleeptimer_duration (or set_sleep_timer
as it has been up til now)
Now I originally added just the set_sleeptimer_duration callback in to
the INT_SETTING for the timer settings in apps/settings_list.c. That
works, but the callback there is called each time you change the
selection in the settings screen. So moving from 5 mins to 10 mins calls
set_sleeptimer_duration with 10. If you cancel the menu it calls the
callback again with the original value, i.e.
set_sleeptimer_duration(5);. That meant that the menu-level callback in
apps/menus/settings_menu.c was pointless, since the settings-level
callback would already have changed the value, resetting the sleep timer
you had running. After realizing that, I just removed this code since it
Basically AFAICT the shortcuts are "lightweight" entries that just
change the settings values, they do not really point to the same menu
entry in sub menus. In this case, you do not get a call to the
Startup/Shutdown menu-level callback when you have a shortcut to the
timer settings. I guess the real fix would be to have to code do
something like that, but that sounds like a pretty major change to me.
Received on 2012-10-06