Rockbox

Tasklist

FS#10712 - Pictureflow manual patch

Attached to Project: Rockbox
Opened by Michael Chicoine (mc2739) - Thursday, 22 October 2009, 15:10 GMT
Last edited by Marianne Arnold (pixelma) - Sunday, 15 November 2009, 22:40 GMT
Task Type Patches
Category Manual
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

This patch cleans up the pictureflow manual button map. It also adds iAudio M3 remote control mappings and allows the manual to build again.
This task depends upon

Closed by  Marianne Arnold (pixelma)
Sunday, 15 November 2009, 22:40 GMT
Reason for closing:  Accepted
Additional comments about closing:  Committed as r23642. Thanks for the collaboration. :)
Comment by Marianne Arnold (pixelma) - Friday, 23 October 2009, 07:47 GMT
Unfortunately I also made a patch for this but I first want to comment on your patch:

1) Thanks for adhering to my suggested style but there is one thing in a wrong order:

+ \opt{HAVEREMOTEKEYMAP}{
+ \opt{GIGABEAT_RC_PAD}{\ButtonRCVolDown{} / \ButtonRCVolUp}
+ \opt{IAUDIO_RC_PAD}{\ButtonRCDown{} / \ButtonRCUp}
+ }&

The & there at the end should be inside the {}, so changed order of the last two. The & is the column "seperator" and we have 2 column button tables for targets without HAVEREMOTEKEYMAP and 3 column button tables for targets with it. That's why the separator would also need to be optional. (Did you try compiling a manual of a non-remote target?)

Another thing that I noticed when working on this: using IAUDIO_RC_PAD there isn't quite right because this would be true for the X5 and M5 too once we add that option to their platform files. While the three use the same remote with the same mapping in core Rockbox (lists, WPS etc.) they don't in plugins (currently at least) - the M3 uses its own keypad definition which just contains remote control buttons because most plugins will be useless without display - remote - and so use the remote to control them. The X5 and M5 will only have the possibility to quit the plugin or so similar to what you can do on the H100 or H300, if at all. I chose to use IAUDIO_M3_PAD defines there even though they are in the remote controls column.

The \nopt{scrollwheel,IRIVER_H10_PAD,IAUDIO_M3_PAD} in the beginning look wrong - the differntiation is there for complete lines in the table so the \\ needs to be inside the {} too. Scrollwheel (and scrollpad targets in case of the H10) use the same buttons for scrolling left/right in "art view" and up/down in the track list - and the others use different buttons: left and right in art view" and up, down in the tracklist. The former just needs one line in the table, the other two, I also think that the M3 belongs to the latter too but it's hard to check. (I hope that's more understandable if you look at my diff.)

Finally my diff - as I said here: http://www.rockbox.org/irc/log-20091022#19:28:19 (and following) I'm not sure what to do with this problem (maybe it's not as bad as I thought, someone else looking at the diff could tell) and it still needs to be checked if the info itself is correct and maybe the last line could be simplified too, at least it still needs the M3 addded (but it won't break)
Comment by Marianne Arnold (pixelma) - Friday, 23 October 2009, 07:48 GMT
Forgot to attach the file...
Comment by Michael Chicoine (mc2739) - Saturday, 24 October 2009, 22:08 GMT
Thanks for looking over my patch.

I'm not sure why I put the & outside the {}. I'm sure I did it for a reason, but I don't remember what that reason was.

The first and second \nopt{scrollwheel,IRIVER_H10_PAD,IAUDIO_M3_PAD} are because on the M3 the only buttons defined in the pictureflow plugin are the remote buttons. I did this to remove those keys from the first column. Is there another/better way to do this?

The \nopt{ONDIO_PAD,IRIVER_H10_PAD,RECORDER_PAD,IAUDIO_M3_PAD} {\ButtonSelect} is needed to allow the M# manual to build (there is no ButtonSelect on the M3).

I did not test on non remote targets (and definitely should have). Which targets should I test to get a good idea of how much damage my patch may cause?
Comment by Marianne Arnold (pixelma) - Sunday, 01 November 2009, 18:58 GMT
Ok, I reworked my patch and checked if the info is correct. Unfortunately it's not as simple in all places as I thought it would be in my first "draft". While the plugin is using core actions there are enough exceptions and overridden things so that the corresponding \ActionSomething is not always appropriate. Still I tried to unify as much as possible, get a cleaner look and added some comments. I'm posting this here for a final review and plan to commit it in a few days if there are no objections. I also found a bug in the Iaudio remote keymap tex file which would lead to wrong info in this button table and would correct it while at it.

While working on this, I noticed that there is some outdated info and missing things in the rest of the chapter and maybe some native speaker can help out a bit here:
- the note in the beginning is wrong (that you can't play music). This is possible now and you can start a playlist from within pictureflow - instead there should be something saying that
- on swcodec (or probably "high mem" targets) you will stay in pictureflow, not automatically enter the WPS
- on low mem targets (where music has to stop for pictureflow because the plugin grabs the audio buffer), it should be mentioned that playback will start and you will be dropped into the WPS as both doesn't work (works this way on my Ondio)
- I believe there is a lowmem "feature" that can be used
- the menu changed a bit and at least the "Go to WPS" item is missing
Comment by Michael Chicoine (mc2739) - Sunday, 08 November 2009, 23:24 GMT
pixelma:

Your patch looks good to me.

Regarding the note at the beginning:
1. It looks OK for the targets that have playback capabilities. You may want to add something like this: "The PictureFlow plugin will continue to run while your tracks are played."
2. As far as I can tell, low mem targets do not have playback capabilities from within pictureflow. I do not have a low mem target to test on, but the simulators for these targets and the source for pictureflow seem to confirm that. If that is in fact the case, then the note at the beginning for those targets is correct.
3. The "Go to WPS" menu item (all targets) and "Playback Control" menu item (all but lowmem targets) are indeed missing.

I have updated your patch to include the additional line from item 1 above and the missing menu items.

Loading...