Rockbox

Maybe this should be simplified and not rely on timing when voicing?. i.e. Speak the time (or date) only when the cursor bar moves onto the time line, or when the user presses SELECT (or RIGHT).

+1 for simplifying and +1 for responding to OK by repeating the time
or date if selection is on Time or Date instead of recalculating the
disk free space. Maybe the time could talk unbidden whenever the
minute changes and the selection is on Time.
But that doesn’t simplify things I’m sure

Comment by Stephen Bavin (pondlife):

Here’s another attempt at the info screen simplification. This updates the time regularly while it’s visible/selected. The downside is that small LCDs which rely on scrolling will be compromised by the redraw.

The voicing now works only when an item is first selected. I’ve not added the “OK to repeat” option, but that’s fairly easy.

For some reason, the list code had 2 explicit talk_shutup() calls to stop voice output. This patch removes one of them, but I’d think that the list code shouldn’t be affecting voice output at all, so maybe the other one should be removed too.

Also, the gui_synclist_item_is_onscreen() call seemed to be out by one when I tested on the sim. I’ve fixed this, but it would be good if someone could try this on a target with a small screen and report back. Test without voice menus enabled for this one.

   info_screen_update.patch (13 KiB)

You had said:

For some reason, the list code had 2 explicit talk_shutup() calls to stop
> voice output. This patch removes one of them,

I’m not sure why you want to remove that. The talk_shutup()
in _gui_synclist_speak_item() is there to shut us up when browsing
quickly through items. When _gui_synclist_speak_item() is called, we will
be saying something new: the magic is that it may be delayed just a bit
when on repeat key or if moving rapidly. Say you’re browsing the files in
a folder, it’s saying a long filename, and you do a repeated DOWN: it
won’t try to speak all the files, it’ll wait until you stop and
settle. The talk_shutup() here is so it will stop saying the filename you
were on, all the while you’re holding DOWN.

The second talk_shutup() I see is on exiting
simplelist_show_list(). Usually you’ve cancelled or chosen something and
are getting out of the context, so it’s appropriate to shutup
there. Otherwise it may continue talking while you’ve exited the context,
say you’ve returned to WPS and nothing else is coming up to interrupt
you. Without the shutup, the user feels as though there button press
wasn’t registered.

but I’d think that the list
> code shouldn’t be affecting voice output at all, so maybe the other one
> should be removed too.

I’m not sure what you mean… Voice is wired into the list code. I don’t
see how this is causing the bug in the first place.

Hi Stephane,

I don’t think the GUI code should be directly involved in voicing. That should be handled only by the callback function.

When the info screen is refreshed (every second, or thereabouts)to display the current time, that results in another call to the speech callback, which then restarts the voicing of the current item - so you hear “Batt Batt Batt” rather than “Battery level…“. I added a test into the speech callback so it wouldn’t restart the voicing unless the selection changed, but this resulted in a single “Batt” - all the required speech was queued, but truncated by the talk_shutup() call.

I don’t notice extra speech without the talk_shutup() on my H300 - is this something that affects a scrollwheel target more? Each spoken filename should be clearing any previous queued speech anyway. I’ll play some more…

When the info screen is refreshed (every second, or thereabouts)to display
>the current time, that results in another call to the speech callback, which
>then restarts the voicing of the current item - so you hear “Batt Batt
>Batt” rather than “Battery level…“.

Well that’s the issue seems to me: you can’t refresh speech like that,
at least not at that rate.

I’d try taking out the gui_synclist_speak_item() call in the REDRAW case
of simplelist_show().

This refresh/REDRAW thing is a simplelist feature. In the general list
implementation, gui_synclist_speak_item() is called only when moving to a
new item.

Spurious calls to gui_synclist_speak_item() are problematic. If we’re
rely on the item number, then might as well not make that spurious call
in the first place. If our point is that the item may change and should
be announced on change, then this is hard to do in a generic way: we need
to know when it does change, and rate limit the thingso we have time to
speak. (And it doesn’t help that we have no reliable way of knowing when
we’re done speaking something, because of pcm buffering.)

I’d have to look through the code for other uses of that REDRAW feature,
but I doubt re-voicing can be done in a generic way, so that
gui_synclist_speak_item() on REDRAW seems suspicious to me.

I don’t notice extra speech without the talk_shutup() on my H300 - is this
>something that affects a scrollwheel target more? Each spoken filename
>should be clearing any previous queued speech anyway.

The new filename will clear the previous speech, but only when we do
decide to speak it. As long as you’re holding your DOWN button or
spinning your scroll wheel, we should be silent. That applies to menu
items and just about any list too, although the filename case is worse of
course because of the thumbnail loading. We need to fall silent as soon
as you move to a new item, yet not start speaking that item until we’ve
settled.

Hi Stephane,

I don’t know much about the liet handling. However, a redraw definitely does a talk_shutup() and calls the speech callback. Maybe I should not be attempting to refresh at all if speech is enabled.

The new filename will clear the previous speech, but only when we do
> decide to speak it. As long as you’re holding your DOWN button or
> spinning your scroll wheel, we should be silent. That applies to menu
> items and just about any list too, although the filename case is worse of
> course because of the thumbnail loading. We need to fall silent as soon
> as you move to a new item, yet not start speaking that item until we’ve
> settled.

With thumbnails, we wait for the user to settle anyway AFAIK. We only set a timer when a file is selected and that only triggers if the selection stays constant for long enough.

Hi again,

I don’t know much about the liet handling.

I am familiar with the voice integration for general lists, as I did
that. I am less vfamiliar with the simplelist construct.

However, a redraw definitely
>does a talk_shutup() and calls the speech callback.

Yes, that’s the problem. Just to be clear: we mean ACTION_REDRAW from the
action callback.

There is no way to tell whether there has been a substantive update to
the listed data that merits re-voicing, or whether this is a high
frequency redraw.

The info screens has the two kinds: disk scan, vs time update.

The gui_synclist_speak_item(&lists); in simplelist_show() on redraw is
even guarded by an if (action != ACTION_NONE) that appears to be stale
code: looking at older revs, that section used to be entered on
ACTION_NONE as well as ACTION_REDRAW, but no longer.

Maybe I should not be
>attempting to refresh at all if speech is enabled.

I’d start by trying to get it right without speech repetition. Attached
is my suggested fix.

I’ve grep’ed and looked through the code: the info screen is the only
list that is voiced and that uses ACTION_REDRAW. So until we have more of
these, it’s hard to generalize about appropriate voice refresh behavior.

It’s still possible to special case things, as I’m doing by having the
current item re-spoken when the disk scan finishes and returns
ACTION_REDRAW. Without that you don’t know when the scan has completed.

The new filename will clear the previous speech, but only when we do
» decide to speak it. As long as you’re holding your DOWN button or
» spinning your scroll wheel, we should be silent. That applies to menu
» items and just about any list too, although the filename case is worse of
» course because of the thumbnail loading. We need to fall silent as soon
» as you move to a new item, yet not start speaking that item until we’ve
» settled.
>
>With thumbnails, we wait for the user to settle anyway AFAIK. We only set a
>timer when a file is selected and that only triggers if the selection stays
>constant for long enough.

My point was that while we’re waiting for the selection to settle, we
need to be silent. That’s what that talk_shutup() is for.

You might look at the explanation in  FS#7774   FS#7775 .

   refresh-fix.diff (2.5 KiB)

If there is no further work on this, we could commit my above fix?

Go for it

Sorry, I didn’t notice that the scrolling had been broken.

We have several requirements to handle:
1) Long text scrolls correctly
2) The displayed time is updated in real-time (at least it it is selected)
3) Entries are voiced as they are first selected, but not repeatedly (except when returning from disk scan)

Because redraws are required for (2) and yet prevent (1), maybe we should always have the cursor bar enabled, and only update the time if it’s the selected item? We’d also need to ensure that ACTION_NONE doesn’t result in the list code performing a talk_shutup() and truncating the voice output.

I’d just like to say that the list code (and the talking) dont need to be touched if we went with my idea to move the time/date out of the info screen, heck even just out of the list in the info screen….

havnt got the code here, but iirc the list selector is only visible if voice is enabled so yes, the time should only talk if its actually selected.

Sorry for breaking the scrolling. I hadn’t realized that it was one of
the reasons for the 5seconds delay and that redrawing too fast would
prevent scrolling.

I suppose that 5seconds delay isn’t as pretty as it might be, but we
are asking a special case from this general purpose list component.

I’m not opposed to having a dedicated time screen, but I don’t see
what it buys me. I’d have to go two places to get the two most
interesting and changing pieces of info: battery and time. And you’d
need to make sure I won’t actually change the time accidentally.

What I find useful is something along the lines of  FS#8959  (which is a
bit out-of-date, but you’ll get the idea).

Anyway would someone sighted please confirm this fix? I’ll leave the
bug open this time .

BTW feel free to reduce the 5seconds delay to whatever looks good and
preserves proper scrolling. I have removed the
gui_synclist_speak_item() from the list redraw case, so redraw
frequency is no longer tied to speech.

As I said above somewhere, we can’t tell if the redraw is for a
important information update or a high-frequency visual thing… I
suppose we could add a new action for respeaking if we really wanted
that, but repeating the time isn’t particularly important in this case
and AFAIK this is the only complicated refresh case there is so it’s
hard to generalize.

I think the ultimate solution is to fix the redraw so it doesn’t reset the scroll position, i.e. allow redraw and scrolling. Until then, this is probably the best compromise.

A separate time screen is unneeded, IMHO.

I have an interesting compromise solution….
We could allow the get text callback to return NULL which would tell the list to not redraw that line, but yeah, another special case for a barely used screen?

No need to compromise or special case, “just” allow the redraw to leave the scroll position alone. That way there’s no conflict. Unless you can think of a place where we deliberately use a redraw to reset the scroll position?