This is the bug/patch tracker for Rockbox. Click here for more information.
Quick links: Bugs · Patches · Rockbox frontpage
FS#10387 - Rework pluginlib actions
Attached to Project:
Rockbox
Opened by Thomas Martitz (kugel.) - Saturday, 27 June 2009, 17:52 GMT+2
Last edited by Thomas Martitz (kugel.) - Thursday, 20 May 2010, 19:43 GMT+2
Opened by Thomas Martitz (kugel.) - Saturday, 27 June 2009, 17:52 GMT+2
Last edited by Thomas Martitz (kugel.) - Thursday, 20 May 2010, 19:43 GMT+2
|
DetailsI reworked PLA to only offer two contexts, one of which is for remotes.
The aim is to remove the currently pretty useless solution. It has so many contexts that tend to clash on targets in various situations. That means, this patch greatly simplifies the system, giving only 1/2 contexts with a minimal set of buttons. With this patch, the actions are: PLA_UP*, _DOWN*, _LEFT*, _RIGHT*, _START* and _QUIT. Additionally, scrollwheel targets have _SCROLL_FWD* and _SCROLL_BACK* * plus the according _REPEAT action. Affected plugins: Bubbles, demestify, dice, fire, jackpot, maze, mazezam, metronome, robotfindskitten, rocklife and clock I looked into the manuals for the targets to decide whether my mapping makes sense, but I guess it still needs testing. Another thing, we can consider adding a _MENU* action, but it was not really needed, and I'm not sure every target has another free button for that. After this is in, I'd like to convert more plugins to PLA (mostly demos), as it greatly simplifies and adds a good deal of consistency. 
pla_rework.diff
(54.3 KiB)
apps/plugins/clock/clock.c | 36 -- apps/plugins/fire.c | 26 + apps/plugins/rocklife.c | 4 apps/plugins/dice.c | 2 apps/plugins/lib/pluginlib_actions.c | 525 ++++++++--------------------------- apps/plugins/lib/pluginlib_actions.h | 23 - apps/plugins/mazezam.c | 27 - apps/plugins/metronome.c | 34 +- apps/plugins/demystify.c | 30 +- apps/plugins/robotfindskitten.c | 33 -- apps/plugins/maze.c | 37 -- apps/plugins/jackpot.c | 4 apps/plugins/bubbles.c | 47 --- 13 files changed, 235 insertions(+), 593 deletions(-) |
This task depends upon
Closed by Thomas Martitz (kugel.)
Thursday, 20 May 2010, 19:43 GMT+2
Reason for closing: Accepted
Additional comments about closing: r26202
Thursday, 20 May 2010, 19:43 GMT+2
Reason for closing: Accepted
Additional comments about closing: r26202
Quit maps to the same button as Power off for many devices, so I rather not have repeat for that one. It will just conflict, so I rather not encourage repeat on that one.
But that's also a problem I don't address and which exists already. I actually added repeat to most others.
As for repeating on the "Quit//Cancel" repeating, we allow (and commonly use) it in the core, so it's rather unreasonable to restrict it in plugins.
As well, unless the button is *only* going to be used to quit plugins, "cancel" is a better term for it, because you can cancel out of the plugin, or you can cancel selection. They're both just more general terms than "Start" and "Quit"
However
"As for repeating on the "Quit//Cancel" repeating, we allow (and commonly use) it in the core, so it's rather unreasonable to restrict it in plugins." is not true for most targets.
Cancel in the core is almost never mapped to power off (instead, it's commonly mapped to BUTTON_LEFT), while this PLA_QUIT is mapped to the power off button for most targets (because BUTTON_LEFT is already in use for PLA_LEFT). So, in the core there's no problem, but for PLA there is.
And once more, this isn't an issue my patch covers at all.
The buttons that are offered are sufficient so far.
This is the _only_ case where repeating cancel is bad - when cancel needs to be assigned the same button as a hardware power off.
Even on the e200 the hardware power off is quite slow. Isn't it something like 15 seconds? We don't always allow software power down in the plugin (for example, otherwise "Down" would shut down iPods in plugins).
PS: You can actually power off from any plugin which has a menu. Any other plugins already exits on a short press. In both cases, holding the power off button leads to a software shutdown. What you're trying to achieve just complicates things and possibly leads to unexpected behavior.
Your reasoning for why it shouldn't is false, which was my point. Saying "I refuse to actually demonstrate the cases in which my reasoning is true" doesn't really help your case any.
So please, on what specific players does allowing long-use of cancel cause a problem?
for example, in robotfindskitten, if I hold left button, robot used to keep moving left.
but with the patch, robot moves only twice. (one is by PLA_LEFT and the other is by PLA_LEFT_REPEAT)
I do not see the reason to have PLA_QUIT_REPEAT. _QUIT seems tend to quit the plugin or enter it's menu. thus PLA_QUIT_REPEAT is not likely to be used imo...
and as for gigabeat, holding power button ~3 sec would turn off the player.
I think it's useful to have PLA_MENU at least for the targets which has menu button.
teru: Adding more buttons is always on cost of the generic-ness, the ondio can't have a MENU, for example.
Scrollwheels are special, IMO. And I didn't make a generic button for the scrollwheel buttons, but special buttons only for those.
but i have issue with metronome.
i got following error when compiling.
> CC apps/plugins/metronome.c
> /home/teru/rockbox/apps/plugins/metronome.c:640: error: syntax error before ‘}’ token
> make: *** [/home/teru/rockbox/build/apps/plugins/metronome.o] Error 1
and messages "start: press select" and "start: hold select" in plugin doesn't make much sense for me.
shouldn't they be "start: hold select" and "pause: press power" or something like?
I tried to build simulaters for some keypad types and found issues in pluginlib_actions.c.
at line 127: CONFIG_LEYPAD -> CONFIG_KEYPAD
at line 129: ( PLA_DOWN -> { PLA_DOWN
at line 243: BUTTON_PREV -> ?
BUTTON_PREV doesn't exist in H10.
at line 261: BUTTON_MINUS|BUTTON_MINUS -> maybe BUTTON_MINUS|BUTTON_REPEAT ?
direction is not defined for IAUDIO_M3_PAD.
The changes in the file bins.pl doesn't belong to this patch, right?
I make minor changes to the game bubbles and update the new game codebuster to the new layout of pluginlib_actions.