Rockbox

Tasklist

FS#10387 - Rework pluginlib actions

Attached to Project: Rockbox
Opened by Thomas Martitz (kugel.) - Saturday, 27 June 2009, 15:52 GMT
Last edited by Thomas Martitz (kugel.) - Thursday, 20 May 2010, 17:43 GMT
Task Type Patches
Category User Interface
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Version 3.3
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

I 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.
This task depends upon

Closed by  Thomas Martitz (kugel.)
Thursday, 20 May 2010, 17:43 GMT
Reason for closing:  Accepted
Additional comments about closing:  r26202
Comment by Paul Louden (Llorean) - Saturday, 27 June 2009, 17:52 GMT
Wouldn't SELECT and CANCEL make more sense than START and QUIT, and why shouldn't every button have a repeat?
Comment by Thomas Martitz (kugel.) - Saturday, 27 June 2009, 18:00 GMT
Why should it make more sense? It totally depends on the plugin what the key actually does. And most of the existing PLA plugins use it for starting or exiting the "content".

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.
Comment by Paul Louden (Llorean) - Saturday, 27 June 2009, 18:33 GMT
It makes more sense because "Start" and "Quit" are specific while "select" and "cancel" are general. We refer to a button in almost every manual as "select" and it will almost always be the "select" action. The button doesn't *start* things very often. In fact, in most plugins it's simply used to select menu entries.

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"
Comment by Thomas Martitz (kugel.) - Saturday, 27 June 2009, 18:53 GMT
Well, OK for cancel/select, I don't care much.

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.
Comment by Paul Louden (Llorean) - Saturday, 27 June 2009, 18:54 GMT
Yes, but we also allow repeat on the power off button, so focusing on the cancel button doesn't change that.
Comment by Thomas Martitz (kugel.) - Saturday, 27 June 2009, 19:17 GMT
That's only allowed for targets which are already short on buttons, which are not that much (ondio and ipods, any more?). And I don't want to encourage this.
The buttons that are offered are sufficient so far.
Comment by Paul Louden (Llorean) - Saturday, 27 June 2009, 19:22 GMT
Alright - how many targets have a hardware power off on a button that *has* to be used for "Cancel" because there aren't enough other buttons, exactly, and the hardware power off is in a short enough time (say, less than 5-7 seconds) to be dangerous?

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).
Comment by Thomas Martitz (kugel.) - Saturday, 27 June 2009, 19:37 GMT
I'm not going to discuss this further. This is unrelated to this patch.

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.
Comment by Paul Louden (Llorean) - Saturday, 27 June 2009, 19:40 GMT
How is this unrelated? It's a discussion of whether or not this patch should do something, and why.

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?
Comment by Teruaki Kawashima (teru) - Saturday, 11 July 2009, 15:35 GMT
I tested the patch and noticed issue with holding direction keys.
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.
Comment by Thomas Martitz (kugel.) - Saturday, 11 July 2009, 16:38 GMT
Sync and rename PLA_START* to PLA_SELECT* and PLA_QUIT to PLA_CANCEL

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.
Comment by Thomas Martitz (kugel.) - Sunday, 12 July 2009, 15:58 GMT
Fix crashes and problems with REPEAT.
Comment by Thomas Martitz (kugel.) - Sunday, 12 July 2009, 16:10 GMT
Remove the chopper hunk.
Comment by Thomas Martitz (kugel.) - Friday, 14 August 2009, 16:48 GMT
Few fixes to button mapping and to metronome (/me hopes mentronome is usable now).
Comment by Teruaki Kawashima (teru) - Sunday, 30 August 2009, 06:21 GMT
worked well for me, using gigabeat X.
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?
Comment by Thomas Martitz (kugel.) - Sunday, 30 August 2009, 11:06 GMT
Yea, depending on the target the text is bullshit. I'm thinking of having that text in the lib.
Comment by Teruaki Kawashima (teru) - Sunday, 30 August 2009, 13:53 GMT
the text is wrong for all target, imo.
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.
Comment by Thomas Martitz (kugel.) - Saturday, 10 October 2009, 16:47 GMT
Fixed the problems you mentioned.
Comment by Thomas Martitz (kugel.) - Saturday, 10 October 2009, 18:27 GMT
fix pitch detector.
Comment by Johannes Schwarz (Ubuntuxer) - Thursday, 22 October 2009, 17:42 GMT
I tested your patch on different targets in the simulator and it seems to work well.
The changes in the file bins.pl doesn't belong to this patch, right?
Comment by Thomas Martitz (kugel.) - Thursday, 22 October 2009, 18:26 GMT
uhm no, that was just for making the test builds (see test builds forum). Thanks for testing, which targets did you test?
Comment by Johannes Schwarz (Ubuntuxer) - Sunday, 25 October 2009, 13:43 GMT
I tested the targets e200, fuze, c200, ipod video, mrobe, giga_s and onda in the simulator. I haven't found any bug.
I make minor changes to the game bubbles and update the new game codebuster to the new layout of pluginlib_actions.
Comment by Thomas Martitz (kugel.) - Wednesday, 21 April 2010, 09:50 GMT
Sync, add alarmclock add PLA_SELECT_REL which only fires when the button is released. basically you either use PLA_SELECT or PLA_SELECT_REL (and PLA_SELECT_REPEAT). _REL makes it possible to map different actions than on PLA_SELECT_REPEAT, PLA_SELECT not.
Comment by Thomas Martitz (kugel.) - Sunday, 16 May 2010, 13:10 GMT
Sync, add mpio hd200 and fix tapping in metronome.
Comment by Thomas Martitz (kugel.) - Sunday, 16 May 2010, 13:19 GMT
Fix some compiler warnings.
Comment by Thomas Martitz (kugel.) - Monday, 17 May 2010, 06:15 GMT
Fix maze repeat actions.
Comment by Thomas Martitz (kugel.) - Thursday, 20 May 2010, 15:52 GMT
Commit candidate

Loading...