Rockbox

This is the bug/patch tracker for Rockbox. Click here for more information.

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#10322 - Abortable splash

Attached to Project: Rockbox
Opened by Thomas Martitz (kugel.) - Friday, 12 June 2009, 15:06 GMT+2
Last edited by Thomas Martitz (kugel.) - Friday, 12 June 2009, 15:15 GMT+2
Task Type Patches
Category User Interface
Status Assigned
Assigned To No-one
Player Type All players
Severity Low
Priority Normal
Reported Version Version 3.2
Due in Version Undecided
Due Date Undecided
Percent Complete 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)
   abortable-splash.diff (7 KiB) 
 apps/playlist.c   |    2 -
 apps/gui/splash.c |   63 ++++++++++++++++++++++++++++++++++++++++++++----------
 apps/gui/splash.h |   20 +++++++++++------
 apps/filetree.c   |   12 +++++-----
 apps/root_menu.c  |    2 -
 5 files changed, 73 insertions(+), 26 deletions(-)

This task depends upon

Comment by Jonathan Gordon (jdgordon) - Saturday, 13 June 2009, 02:08 GMT+2
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, 17:02 GMT+2
- "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, 04:07 GMT+2
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, 07:37 GMT+2
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, 14:15 GMT+2
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, 19:47 GMT+2
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, 19:48 GMT+2
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, 19:53 GMT+2
then its the users fault if they cancel it before reading it.
Comment by Thomas Martitz (kugel.) - Sunday, 14 June 2009, 19:53 GMT+2
I definitely disagree. You can cancel it accidentally quite easily.
Comment by Scott Berry (scottbb) - Sunday, 14 June 2009, 21:34 GMT+2
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, 21:55 GMT+2
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, 07:17 GMT+2
This is the how it should be done
   dismiss_splash.diff (0.9 KiB) 
 apps/gui/splash.c |   15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

Comment by Frank Gevaerts (fg) - Wednesday, 17 June 2009, 00:12 GMT+2
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.) - Wednesday, 17 June 2009, 00:23 GMT+2
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, 10:23 GMT+2
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, 10:27 GMT+2
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.
   dismiss_splash_090617.patch (0.6 KiB) 
 apps/gui/splash.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Comment by Frank Gevaerts (fg) - Wednesday, 17 June 2009, 10:48 GMT+2
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, 10:56 GMT+2
Huh, those also add to the button queue? That's not what I'd expect... ;-)
Comment by Steve Bavin (pondlife) - Wednesday, 17 June 2009, 11:33 GMT+2
OK, this one ignores SYS_ events - it should probably also ignore scrollwheel events, but I don't have a scrollwheel target.
   dismiss_splash_090617a.patch (2.7 KiB) 
 apps/gui/splash.c        |   21 ++++++++++++++++++++-
 firmware/export/kernel.h |    2 +-
 firmware/kernel.c        |   17 +++++++++--------
 3 files changed, 30 insertions(+), 10 deletions(-)

Comment by Thomas Martitz (kugel.) - Wednesday, 17 June 2009, 20:10 GMT+2
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, 20:14 GMT+2
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, 20:30 GMT+2
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, 20:48 GMT+2
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, 08:44 GMT+2
Either way, get_action would remove the button press, no?
Comment by Thomas Martitz (kugel.) - Thursday, 18 June 2009, 12:10 GMT+2
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, 12:24 GMT+2
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, 18:57 GMT+2
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, 15:42 GMT+2
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...