Rockbox

Tasklist

FS#7774 - Voice callback for gui_synclist

Attached to Project: Rockbox
Opened by Stephane Doyon (sdoyon) - Friday, 14 September 2007, 02:19 GMT
Last edited by Jonathan Gordon (jdgordon) - Wednesday, 21 November 2007, 05:52 GMT
Task Type Patches
Category User Interface
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 1
Private No

Details

gui_synclist uses a get_name callback to return the text for each item or
line. There are also callbacks for icons and color. It seems to me it
would make sense to also use a callback for the voice: speaking out the
items.

Advantages:

-The text and voice callbacks can be unified or at least made to follow a
similar pattern, so they get their info in the same way. Should make it
easier to maintain things and to keep the spoken info in sync with the
text.

-No need to come up with an ad hoc way to handle speech in each
context. This is currently reinvented in each context in slightly
different ways: remembering the last spoken item and waiting until we
change to another one, or setting a flag based on the list actions and
whenn the list is updated... AFAICT, the logic seems to be pretty similar
for all the contexts I've checked out and it makes sense to generalize
that functionality.

-The file browser in tree.c has this elaborate "hover delay" concept to
avoid overloading the system with disk access and talk requests when the
user moves rapidly through a directory. This functionality would be
useful in other contexts (like the playlist viewer and bookmark screen),
and I think it can help slightly even in contexts without .talk clips. By
moving this hover functionality in the list itself, I can share the
code. I have also improved it a bit in the process, and simplified
tree.c.

I've converted several lists to use this callback method and it appears
to be a good fit in all cases: dirbrowse() in tree.c, id3 viewer, playlist
catalog, playlist viewer, bookmark screen, running_time screen, radio
presets list, and my playing-time patch. Those will be posted in
separate patches of course.

It works by registering a callback function, which will be called with
the same data argument as the get_name callback. Generally, one registers
the voice callback only if talk_menus_enabled(), although in special
cases one can test the appropriate settings inside the callback
itself. The callback causes the current item to be spoken
immediately. gui_synclist_do_button calls the callback when moving up or
down to a different item. The context owning the list should call
gui_synclist_speak_item() once initially to speak the first item, and
whenever the list is changed. As a rule of thumb,
gui_synclist_speak_item() should be called in the same places where
gui_synclist_draw() is called, except that you don't want to call it
spuriously.

If the list is empty, gui_synclist_speak_item() will say something to
that effect. That alleviates the need for P#6271 (Say something when
entering an empty directory), with the bonus that it applies to any list
and not just directory browsing.

Improved hover delay: The existing strategy was simply to just wait for
half a second before saying the current filename, each and every time. I
suspect that sighted users would never tolerate the gratuitous insertion
of a half second lag in response to a key press. The strategy I
implemented uses a similar (although shorter) delay whenever we find that
a repeat keypress is received, or when movement actions are received in
quick succession. If we move to a different entry less than a quarter of
a second since we've started speaking the previous one, or if we
encounter ACTION_STD_PREVREPEAT or ACTION_STD_NEXTREPEAT, or if
next_item_modifier>1, then the overload workaround kicks in. The first
thing it does is shutup the currently speaking utterance (previously it
would continue jabbering while you scroll down your big directory). It
then schedules the speaking of the item for a bit later, much like before
(it waits a quarter second, where previously half a second was used). But
the point though is that in the default case, there's no need to wait at
all: on entering the browser, and when moving up/down slowly enough to
actually hear the beginning of the name being spoken, no artificial delay
is introduced.

I have tested this on a Sansa and found it works rather well for me. I
would particularly like feedback from someone testing this with an iPod
click wheel.

One little complication with having the hover logic in the list itself,
is that for the postponed speaking of the name to happen on time, we must
reenter gui_synclist_do_button at the appropriate time. I have
implemented a convenience function called list_do_action() which combines
get_action and gui_synclist_do_button: if the speaking of an item
pending, it shortens the get_action timeout appropriately to coincide
with the scheduled time.
This task depends upon

Closed by  Jonathan Gordon (jdgordon)
Wednesday, 21 November 2007, 05:52 GMT
Reason for closing:  Accepted
Additional comments about closing:  this was commit a while ago wasnt it?
Comment by Mario Lang (mlang) - Monday, 17 September 2007, 14:20 GMT
This patch (and the items depending on it) is great, I'd like to help to get it included.
Two things I noticed: First of all, it did not apply to a fresh subversion checkout because the addition to english.lang (VOICE_EMPTY_LIST) conflicted.
This one was easy to resolve.
However, just today djgordon did a change that affects apps/tree.c and apps/gui/list.c which
breaks this patch.
Stephane, will you provide an up-to-date version?
Comment by Stephane Doyon (sdoyon) - Monday, 17 September 2007, 15:04 GMT
Sure I'll update it. Shouldn't be a problem, only I'll have to update the
patches that depend on this one as well. Give me a day or two.
(I'd like nothing better than to do it right now, but my employer
might not agree :-).)
Comment by Stephane Doyon (sdoyon) - Tuesday, 18 September 2007, 02:14 GMT
Here's an update taking into account the recent gui_synclist_do_button change.
It actually makes callers prettier, but it requires that I update
all of the patches I have depending on this.
Comment by Nils Wallménius (nls) - Friday, 21 September 2007, 22:00 GMT
I've looked at this patch and it looks good :-) it does however break hwcodec sim builds. (These sims don't build talk.c so) we get some undefined references to functions such as shutup(), you might want to define some empty macros or something see talk.h:86
Comment by Stephane Doyon (sdoyon) - Saturday, 22 September 2007, 01:52 GMT
Wonderful! Thank you very much for looking at this.

OK here's a patch to talk.h with more substantial dummy macros for the sim
that eliminate all warnings. And a new talk-list.diff with the added
dummy macro for shutup().
Comment by Nils Wallménius (nls) - Wednesday, 26 September 2007, 19:48 GMT
Has this patch been tested on a hard drive player? (With regard to the changes to the delay mechanism for talk clips.) Since these players need much longer time to load a clip than a flash based player.
Comment by Stephane Doyon (sdoyon) - Wednesday, 26 September 2007, 20:47 GMT
I believe I tested earlier versions of the patch on my X5, but admitedly
not as much. I can certainly give it another spin. And my testing would
have been with directory cache enabled, and so I could also try
with it disabled. I'll get back to you on that.

That being said, I've tried to eliminate the clip load time from the equation
by marking the time AFTER the clip has been loaded and enqueued.
In _gui_synclist_speak_item()
...
l->scheduled_talk_tick = 0; /* work done */
cb(sel, l->data);
l->last_talked_tick = current_tick;
...

The talk_file() calls from tree.c happen in the callback (cb()), and
so l->last_talked_tick takes the time after loading the clip into memory.
Comment by Stephane Doyon (sdoyon) - Thursday, 27 September 2007, 02:01 GMT
Tested on X5: works well for me.

Here's a version of the patch with the english.lang reject fixed up
(and an indentation fix), but no functional change.
Comment by Daniel Dalton (ddalton) - Thursday, 04 October 2007, 04:47 GMT
Does this say something when entering an empty directory?
Like
 FS#6271 

Thanks
Comment by Stephane Doyon (sdoyon) - Thursday, 04 October 2007, 14:25 GMT
>Does this say something when entering an empty directory?
>Like  FS#6271 .

Yes, as it says in the description:
>If the list is empty, gui_synclist_speak_item() will say something to
>that effect. That alleviates the need for P#6271 (Say something when
>entering an empty directory), with the bonus that it applies to any list
>and not just directory browsing.

And #6271 was closed, noting that it is superseded by this patch.

ddalton: This patch is part of the set I sent you a couple of times,
and which I believe you ran. Did you not test it? Does it work well for you?
Comment by Daniel Dalton (ddalton) - Thursday, 04 October 2007, 22:20 GMT
Sorry Sdoyon I didn't realise it was for empty dirs as well. I didn't go into one when I had the patch installed. Anyway I will try it out. Will I need an update? Are my set out of date?
Thanks.
Comment by Stephane Doyon (sdoyon) - Wednesday, 10 October 2007, 03:19 GMT
Here are patches to convert menus and option_screen to use the list
voice callback. For the menus I managed a nice cleanup: net 12 lines
deleted.
Comment by Stephane Doyon (sdoyon) - Wednesday, 10 October 2007, 03:26 GMT
Here's a combined patch for convenience of testing. It includes the
list voice callback infrastructure, and patches to use it in the file
browser ( FS#7775 ), menus and option_screen.

I'd like help testing this before committing.

Loading...