Rockbox

Tasklist

FS#10322 - Abortable splash

Attached to Project: Rockbox
Opened by Thomas Martitz (kugel.) - Friday, 12 June 2009, 13:06 GMT
Last edited by Thomas Martitz (kugel.) - Friday, 12 June 2009, 13:15 GMT
Task Type Patches
Category User Interface
Status Assigned
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Version 3.2
Due in Version Undecided
Due Date Undecided
Percent Complete 0%
Votes 0
Private No

Details

This adds asplash and asplashf (and vasplashf for internal use) which normal splashes but can be aborted with button presses.

Additionally, they return the action received (for CONTEXT_STD) so that the caller may handle aborts differently depending on the button.

Also I converted a few annoying splashes to use it (when loading themes, wps, fonts etc and the resume failed splash)
This task depends upon

Comment by Jonathan Gordon (jdgordon) - Saturday, 13 June 2009, 00:08 GMT
you seem to be overengineering....

my plan was to have ALL splashes either have 0 timeout, or a constant 3s (or something). There is no reason (and probably a bad idea) to have the splash return its value (although it should call the default_event_handler() for SYS_ events.

also, that first hunk looks like it shold be done seperatly of the rest of the patch.
Comment by Thomas Martitz (kugel.) - Saturday, 13 June 2009, 15:02 GMT
- "you seem to be overengineering...."
Why?

- "my plan was to have ALL splashes either have 0 timeout, or a constant 3s (or something)"
It seems that your and my plans don't match

- "There is no reason (and probably a bad idea) to have the splash return its value"
Why? It could be used to ask the user a question or to only remove the splash if a specific button was pressed. does it hurt anywhere?

- "(although it should call the default_event_handler() for SYS_ events."
I will look into that.

- "also, that first hunk looks like it shold be done seperatly of the rest of the patch."
Yes, probably.
Comment by Scott Berry (scottbb) - Sunday, 14 June 2009, 02:07 GMT
hey Tom and all,

Just my two cents. i feel that this particular patch will cause many problems for people who are not sighted. I feel that this patch would be best not applied at this time. I apologize but Tom's patch would be better where there is no place to have a question or something. Remember for us blind users everything you patch like this will then need to be spoken. i apologize if i sound rude but I am just making a point.
Comment by Jonathan Gordon (jdgordon) - Sunday, 14 June 2009, 05:37 GMT
Scott, dont worry, There will be no change to blind users... (if Thomas does it properly...) the only tihng in the splash which will change is the time it is shown for. Actually, if every splash has a consistant timeout I would tihnk that would make it better for blind users. (Also, I imagine that having a beep when it exits is a good idea for you also)
Comment by Thomas Martitz (kugel.) - Sunday, 14 June 2009, 12:15 GMT
The splashes should be spoken before you're even able to cancel, I theoretically see no problem here. Have you tested the patch, did you notice problems with voicing?

I think there are some situations you want a splash not to be cancelled (for example, the "Cancelled" one itself or the "Please reboot to enable", or some which report failure).
Comment by Jonathan Gordon (jdgordon) - Sunday, 14 June 2009, 17:47 GMT
what is the logic behind not allowing thoe splashes to be cancelled? The whole point is that if you know what the splash says and are expecting it, why should you need to wait?
Comment by Thomas Martitz (kugel.) - Sunday, 14 June 2009, 17:48 GMT
The point that not every splash is expected maybe? Especially for new users, some splashes are vital to be read.
Comment by Jonathan Gordon (jdgordon) - Sunday, 14 June 2009, 17:53 GMT
then its the users fault if they cancel it before reading it.
Comment by Thomas Martitz (kugel.) - Sunday, 14 June 2009, 17:53 GMT
I definitely disagree. You can cancel it accidentally quite easily.
Comment by Scott Berry (scottbb) - Sunday, 14 June 2009, 19:34 GMT
You know what would also help too is to have at the beginning Rockbox say Rockbox started and at the end Rockbox stopped. I could probably come up with some wav files like that also. also consistent time outs yep they would be good with a beep or something like Rockbox is ready.
Comment by Scott Berry (scottbb) - Sunday, 14 June 2009, 19:55 GMT
Thomas no i haven't tried it yet. I am kind of waiting for you guys to get all the patches for the Clip to be a little more stable. I would like to at least see that the boot loader and the main program be put in a beta package like you do the auto configurations. Then have that updated each day through a script.
Comment by Jonathan Gordon (jdgordon) - Tuesday, 16 June 2009, 05:17 GMT
This is the how it should be done
Comment by Frank Gevaerts (fg) - Tuesday, 16 June 2009, 22:12 GMT
Unless I'm reading things wrong, both patches here throw away events in at least some cases. That will lead to more bugs like FS#9957.
Comment by Thomas Martitz (kugel.) - Tuesday, 16 June 2009, 22:23 GMT
I don't quite understand. How does it throw events away?

It could probably check for SYS_EVENTS if you mean then (my patch aborts on any event, so that's no problem).
Comment by Frank Gevaerts (fg) - Wednesday, 17 June 2009, 08:23 GMT
Basically, whenever you read the button queue (which get_action() does), you have to check for SYS_EVENTs and handle them. Neither patch does this currently. Calling default_event_handler() is probably sufficient, although I'm not sure if that's safe in all places where splashes are used.
Comment by Steve Bavin (pondlife) - Wednesday, 17 June 2009, 08:27 GMT
Here's an attempt to dismiss based purely on a button press, without eating any events...only tested on sim and no idea if it'll work for touchscreen targets.
Comment by Frank Gevaerts (fg) - Wednesday, 17 June 2009, 08:48 GMT
That will dismiss the splash when *any* event comes in, some of which are not triggered by the user (e.g. SYS_BATTERY_UPDATE or SYS_IAP_PERIODIC)
Comment by Steve Bavin (pondlife) - Wednesday, 17 June 2009, 08:56 GMT
Huh, those also add to the button queue? That's not what I'd expect... ;-)
Comment by Steve Bavin (pondlife) - Wednesday, 17 June 2009, 09:33 GMT
OK, this one ignores SYS_ events - it should probably also ignore scrollwheel events, but I don't have a scrollwheel target.
Comment by Thomas Martitz (kugel.) - Wednesday, 17 June 2009, 18:10 GMT
Ignoring SYS_ events is what Jonathan's patch also did (get_action() returns SYS_* when one comes in, which is ingored due to "== ACTION_STD_CANCEL"). I'm not entirely sure what your last patch tries to achieve.

Also, apps/ code really shoudn't use firmware/ functions in that way IMO.
Comment by Steve Bavin (pondlife) - Wednesday, 17 June 2009, 18:14 GMT
I'm not very familiar with this area, but IIUC, Jonathan's patch actually read the queue. I want all events (including the SYS_* ones ) to wait in queue until they can be processed.

This patch is just to see if the UI/idea is sound - I'll wrap the test into buttton.c/.h for the real thing.
Comment by Thomas Martitz (kugel.) - Wednesday, 17 June 2009, 18:30 GMT
I think the SYS_ event should/could be handled in get_action() itself (i.e. calling default_event_handler() there, maybe returning something special in such a case.
Comment by Jonathan Gordon (jdgordon) - Wednesday, 17 June 2009, 18:48 GMT
well, its a general problem and I'm starting to think that those SYS_ events should go in a seperate queue
Comment by Steve Bavin (pondlife) - Thursday, 18 June 2009, 06:44 GMT
Either way, get_action would remove the button press, no?
Comment by Thomas Martitz (kugel.) - Thursday, 18 June 2009, 10:10 GMT
I can't really see how it would be desirable to not eat the button. It seems illogical.

On the desktop you also need to click away dialog boxes, so I would actually expect that a button press is needed to get rid of the splash only.
Comment by Steve Bavin (pondlife) - Thursday, 18 June 2009, 10:24 GMT
Read the ML thread and IRC for details.

Advantages:
(1) You can abort a splash without waiting..
(2) There is no timing issue - especially useful if you can't see the screen. (The alternative option of using the cancel key might result in the underlying screen being cancelled if you press the key just as the splash times out, for example.)
(3) Corollary of (2) - it allows use of a timeout for those users who don't realise that splashes can be aborted with a keypress.
(4) It works without any keymap implications - no need to restrict to a particular button.
(5) The code changes are simple/limited.
(6) The voice and visual UI are consistent

Disadvantages:
(1) Users might not realise that the keypress will be used. - but I think they'd get used to it
(2) Users might dismiss a splash too easily - in which case, forget this change completely.

Feel free to add more and argue it out.
Comment by Alexander Levin (fml2) - Saturday, 20 June 2009, 16:57 GMT
Just my two cents: I like the idea of splashes that disappear when a button is pressed -- and that button press should be processed as if there were no splash at all. Just think of splashes as modeless dialogs (this to reply to Thomas' "On the desktop you also need to click away dialog boxes")
Comment by alex wallis (alexwallis646) - Wednesday, 24 June 2009, 13:42 GMT
Hi Scott, just to respond to your point made earlier in this task
"You know what would also help too is to have at the beginning Rockbox say Rockbox started and at the end Rockbox stopped. I could probably come up with
some wav files like that also. also consistent time outs yep they would be good with a beep or something like Rockbox is ready."


I don't think we really need the voice saying rockbox is ready or rockbox is stopped. This is because when we switch off the player we already get the voice saying shutting down, which to my mind you couldn't get any clearer than that.
When we start the player if a person has a voice you already get the first option in the menu announced that being files, or if you set rockbox to start in the file browser you get your first folder spelled out or announced.
I think your above ideas would just be overkill.

Can you give me an example though when a beep would be useful? I have never had any problems as far as I know with missing splashes or ccancelling them accidentally.

I am a blind user just to let you know.

Loading...