Rockbox

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

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#6854 - Add scrolling to the credits

Attached to Project: Rockbox
Opened by Sylvain Fourmanoit (syfou) - Monday, 19 March 2007, 09:50 GMT+2
Last edited by Peter D'Hoye (petur) - Thursday, 30 August 2007, 21:57 GMT+2
Task Type Patches
Category Plugins
Status Closed
Assigned To No-one
Player Type All players
Severity Low
Priority Normal
Reported Version
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Private No

Details

With more than 280 contributors to Rockbox so far, the credits plugin takes about two minutes to list everyone -- it seems to me it could benefit from some added interactivity; this patch adds simple scrolling capabilities to the credits plugin, so that the user can browse the list of contributors back and forth. Once applied:

- The original text sliding effect is kept intact.
- As with most interactive plugins, there is only one button to quit the plugin (this is model-dependant) -- pressing any button is not good enough anymore.
- The model-dependent up and down buttons can be used to scroll the list, one half of the screen at a time, so that every contributor gets listed at least once.
- At the end of the list, the credits plugin exits if no manual scrolling occurred, and loop otherwise.

There are many other improvements that could be incorporated to make it feel better (automated scrolling in the direction of last manual scroll for instance), but this is a simple, working solution that adds the scrolling feature with little changes to current tree.
(application/octet-stream)    rockbox-scrolling-credits.patch (8.3 KiB) 
 apps/plugins/credits.c |  190 +++++++++++++++++++++++++++++++++++++------------
 1 file changed, 147 insertions(+), 43 deletions(-)

This task depends upon

Closed by  Peter D'Hoye (petur)
Thursday, 30 August 2007, 21:57 GMT+2
Reason for closing:  Accepted
Additional comments about closing:  maybe the navigation isn't 100% fluid but it will do for now...
Comment by Peter D'Hoye (petur) - Friday, 13 April 2007, 22:46 GMT+2
Nice tinkering, but here's why this patch doesn't make it into the code tree: Please read the CONTRIBUTING document regarding coding rules, specifically regarding the use of tabs. Some of the lines of the patch are only replacing spaces with tabs (which we do not want). Yes I could clean it up for you, but this is the way I learned it too ;)
Comment by Sylvain Fourmanoit (syfou) - Wednesday, 18 April 2007, 18:45 GMT+2
This should correct the spaces/tabs issue. CONTRIBUTING also states "Keep lines below 80 columns length"... The current file in subversion (rev. 13201) does not respect this in a few places I modified, hence I didn't change the original formatting when it occurred.
(application/octet-stream)    rockbox-scrolling-credits-2.patch (9.1 KiB) 
 credits.c |  186 ++++++++++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 145 insertions(+), 41 deletions(-)

Comment by Peter D'Hoye (petur) - Sunday, 20 May 2007, 00:12 GMT+2
looks ok.
could you either generate your patch from the root of the rockbox tree or state where to execute it (option one is preferred)

one more thing, the scrolling only starts working if the screen is filled... how's that for a task ;)
Comment by Sylvain Fourmanoit (syfou) - Sunday, 20 May 2007, 00:35 GMT+2
Here you go (patch generated from root).

As for the part about scrolling only working once the screen is filled, it is just because I did want to have minimal impact on current code... Maybe I am just overly cautious, but I didn't fully understand the justification of a few timed operations in the plugin, and I do not have access to other real devices besides the Sansa e200 target.
   rockbox-scrolling-credits-3.patch (9.2 KiB) 
 apps/plugins/credits.c |  186 ++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 145 insertions(+), 41 deletions(-)

Comment by Peter D'Hoye (petur) - Sunday, 20 May 2007, 02:55 GMT+2
more work for you... it crashed the sim here (reproducable)
just move up/sown a few times and then proceed down to the bottom. Once at the end you'll get a segmentation fault (probably a div by 0). I guess the scrollin code conflicts with the moving down, causing some index to go wrong?

too bad, I was going to commit after a final check.
Comment by Peter D'Hoye (petur) - Wednesday, 01 August 2007, 15:46 GMT+2
Will you fix this or face the patch being rejected?
Comment by Sylvain Fourmanoit (syfou) - Wednesday, 01 August 2007, 17:22 GMT+2
Thanks for the reminder -- I will fix this: just allow me a few days (let's say until after this week-end). If it bugs you to see this task open after so long, feel free to close it and I will reopen or resubmit as appropriate.
Comment by Peter D'Hoye (petur) - Wednesday, 01 August 2007, 17:25 GMT+2
no problem.... I'm just poking around on sleeping patches
If you intend to work on it it's fine
Comment by Sylvain Fourmanoit (syfou) - Monday, 06 August 2007, 23:23 GMT+2
Here is a fourth patch addressing this issue; It is not an incremental change on the third one, but rather a different approach: it implements a new plugin (called credits2 for now -- I guess that credits_browser or something similar would be better in the end but it is just a proof of concept) that just displays the credits on a synclist.

I did this because I found the credit browsing I implemented before hard to use and behaving in a counter-intuitive fashion: I think this is far better for the end-user. I tested in in the uisimulator for a while, and it seems stable this time.

Right now, it duplicates the content of 'credits.raw' in the final binary -- I guess this could be changed later if this proved to be a good idea -- there is nothing preventing to merge this into the original credits plugin either.
   rockbox-scrolling-credits-4.patch (4.5 KiB) 
 apps/plugins/CATEGORIES |    1 
 apps/plugins/Makefile   |    6 ++
 apps/plugins/SOURCES    |    1 
 apps/plugins/credits2.c |   99 ++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 107 insertions(+)

Comment by Peter D'Hoye (petur) - Saturday, 11 August 2007, 01:06 GMT+2
hmmmm I don't know about a second credits plugin, one seems enough for me
Comment by Peter D'Hoye (petur) - Tuesday, 21 August 2007, 20:06 GMT+2
Note to Bagder:
re-opened because OP will supply new patch. The "I just don't think this code should go in anymore..." was referring to the old patch.
Comment by Sylvain Fourmanoit (syfou) - Wednesday, 22 August 2007, 03:48 GMT+2
This merges the latest patch into the original credits plugin (no more second plugin): once a key is pressed, it provides the credits on a synclist instead of exiting right away, so that the user gets a chance to interactively browse through the names. I think this is both simple (code-wise) and natural (user-wise). I tested it for a while on both a charcell and non-charcell ui simulator, without getting any segfaults.

I am not sure I got the USB states handling right though: on the simulator, I got a few hangs (rockbox not waking up once plugged, then unplugged when credits is running) I couldn't reproduce on my sansa e280. The bug, if any, eludes me... I seems to get it both with this patch applied or not.
   rockbox-scrolling-credits-5.patch (4.8 KiB) 
 apps/plugins/credits.c |   84 ++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 72 insertions(+), 12 deletions(-)

Comment by Peter D'Hoye (petur) - Wednesday, 22 August 2007, 21:03 GMT+2
hmmm I'm certainly not in favor of this version, it is not intuitive, not a nice way of working at all...
Also doesn't handle USB well, it gets stuck in the listview.

What was wrong with the first way of working anyway, why couldn't you just fix the bug and be done with it?
Comment by Sylvain Fourmanoit (syfou) - Wednesday, 22 August 2007, 21:40 GMT+2
> hmmm I'm certainly not in favor of this version, it is not intuitive, not a nice way of working at all...
> [...] What was wrong with the first way of working anyway?

Not intuitive? It's just a regular list. I showed initial first version to two people, how were clearly struggling with it (they both got lost with the controls a couple of times): that's the reason I reused rockbox pre-written, uniform interface instead of the custom effect.

Is there anything I can do to make it better (or to convince you it's better ;-D )?

>Also doesn't handle USB well, it gets stuck in the listview.

I know: I didn't understand how signaling is supposed to work... I read rockbox code a little more yesterday, and I think I got it now. I could fix this.

> Why couldn't you just fix the bug and be done with it?

I will post this one-liner next Monday (I travel for a few days) but I sure hope you'll reconsider. Thanks for your time once again.
Comment by Sylvain Fourmanoit (syfou) - Monday, 27 August 2007, 21:57 GMT+2
Here is an updated version of rockbox-scrolling-credits-3.patch. It fixes:

- the indexing problem that caused a segmentation fault in previous version (appears stable on simulator).
- another slight display glitch due to incorrect refreshing on the title line on loop.

   rockbox-scrolling-credits-6.patch (9.3 KiB) 
 apps/plugins/credits.c |  188 ++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 147 insertions(+), 41 deletions(-)

Loading...