Rockbox

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

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#8335 - album title config & small improvoments for PictureFlow

Attached to Project: Rockbox
Opened by Thomas Martitz (kugel.) - Sunday, 16 December 2007, 16:25 GMT+1
Last edited by Andrew Mahone (Unhelpful) - Saturday, 27 December 2008, 13:42 GMT+1
Task Type Patches
Category Plugins
Status Closed
Assigned To No-one
Player Type All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Private No

Details

This patch adds a setting to the pictureflow plugin.

The setting named "Show album titles" lets the user dicide where to show the album titles

*hide album titles
*show at the bottom
*show at the top

(show at the bottom is default)

For people knowing there albums very well hide is the best option. For all others "show at the top" is the best option (imo), since it it doesn't destroy the beautiful reflections of the cover with overlapping album titles.

The track list (after selecting a cover) is moved downwards by 30 pixels, when the title is set to be shown at the top, so that the album title doesn't overlap with the track list.

Small improvements are:
*correct the zoom settings menu to be called Zoom (it was "Number of slides")
*move the track list downwards by 30 pixels when show fps is enabled to not overlap the track list
(IMO it looks better in any case when the track list is moved downwards a bit, but I didn't want to change to much at once)
   pf_album_title_v1.diff (4 KiB) 
 apps/plugins/pictureflow.c |   41 +++++++++++++++++++++++++++++++++--------
 1 file changed, 33 insertions(+), 8 deletions(-)

This task depends upon

Closed by  Andrew Mahone (Unhelpful)
Saturday, 27 December 2008, 13:42 GMT+1
Reason for closing:  Accepted
Additional comments about closing:  Parts of this, and equivalents for the rest, merged in r19599.
Comment by Thomas Martitz (kugel.) - Monday, 17 December 2007, 02:30 GMT+1
Ok, next version. This should be ready to commit. I've tested it deeply on my e200. I've added an exception for smaller screens (height < 100) like c200's, though I think pictureflow should be more optimized for small screens in general.


Overall improvement concering the tracklist:
*full screen tracklist if both album titles and fps are hidden
*move the tracklist if album titles and/or fps are shown, including reducing the number of visible items
   pf_album_title_v2.diff (5.5 KiB) 
 apps/plugins/pictureflow.c |   62 ++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 53 insertions(+), 9 deletions(-)

Comment by Thomas Martitz (kugel.) - Monday, 17 December 2007, 02:35 GMT+1
Just fixed a few typos in comments, and added one. No functional change.
Comment by Thomas Martitz (kugel.) - Monday, 17 December 2007, 02:35 GMT+1
Here's the file.
   pf_album_title_v2b.diff (5.6 KiB) 
 apps/plugins/pictureflow.c |   62 ++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 53 insertions(+), 9 deletions(-)

Comment by Thomas Martitz (kugel.) - Tuesday, 18 December 2007, 23:56 GMT+1
No fuctional change. Made an enum for show_album title for easier-to-read-code, some changes to the comments.
   pf_album_title_v3.diff (5.7 KiB) 
 apps/plugins/pictureflow.c |   65 ++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 56 insertions(+), 9 deletions(-)

Comment by Thomas Martitz (kugel.) - Sunday, 11 May 2008, 05:50 GMT+1
Sync and implement smooth_resize. Problem: It doesn't resize.
Comment by Thomas Martitz (kugel.) - Sunday, 11 May 2008, 05:51 GMT+1
Here's the patch.
   pf_album_title_v4.diff (6.1 KiB) 
 apps/plugins/pictureflow.c |   69 +++++++++++++++++++++++++++++++++++++++------
 1 file changed, 60 insertions(+), 9 deletions(-)

Comment by Thomas Martitz (kugel.) - Saturday, 17 May 2008, 05:16 GMT+1
Ok, I got resize to work. Doing this, I also introduced a setting to let the user decide if covers should be resized (default is yes). Like before, on color targets, smooth resize is used (on other simple resize).

Secondly, I changed a bit how PREFERRED_IMG_HEIGHT/_WIDTH are calculated. It's now dependent on the screen size instead of a fixed size (e.g. a Gigabeat F/X defaults to 150 height and width). Also, this values will be used for resizing.
NOTE: Resizing will not keep the aspect ratio. If this behavior isn't wanted, please comment on that.

Thirdly, I fixed (or at least worked around) the wrong order of tracks in the tracks list ( FS#8425 ). It's maybe a bit dirty, but I noticed that the tagcache search for the track titles returned the results in the wrong order. My fix is to "invert" the track index when getting the names from the index.

This patch also moves show_fps into the config struct.

This patch also fixes  FS#8347  by forcing the track list to reset upon selecting the album.

The only tracker entry which doesn't get fixed is  FS#8927  :) ( FS#8326  doesn't seem to happen anymore)


If there are no objections anymore I'm done with this patch, it should be ready for commit. I hope I didn't change too much at once. If I did I could consider splitting it up a bit.
   pf_album_title_v5.diff (10.2 KiB) 
 apps/plugins/pictureflow.c |  133 +++++++++++++++++++++++++++++++++------------
 1 file changed, 98 insertions(+), 35 deletions(-)

Comment by Thomas Martitz (kugel.) - Saturday, 17 May 2008, 18:06 GMT+1
Hold on, after some talk with Slasheri, the track search isn't reliable at all (in terms of ordering). So, my fix is as worse as the original implemntation.

Slashery said the inverted order I get comes most likely from the filesystem which returns the files in some order. I have named my tracks to have the track number leading, so they are if they are ordered by name they are also ordered by track number.
Comment by Thomas Martitz (kugel.) - Saturday, 17 May 2008, 18:42 GMT+1
So, this simply removes the attempt to order the tracklist correctly.

More needs to be done to give it an order. Until then the tracklist is more or less random.

I'm not even sure if the tracklist was intended to be ordered.
   pf_album_title_v6.diff (9.8 KiB) 
 apps/plugins/pictureflow.c |  127 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 93 insertions(+), 34 deletions(-)

Comment by Thomas Martitz (kugel.) - Tuesday, 24 June 2008, 16:54 GMT+1
Sync, also fix a bug introduced r17781 (1 track missing in the tack list).
   pf_album_title_v7.diff (9.8 KiB) 
 apps/plugins/pictureflow.c |  128 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 92 insertions(+), 36 deletions(-)

Comment by Thomas Martitz (kugel.) - Tuesday, 24 June 2008, 16:58 GMT+1
Sync, also fix a bug introduced r17781 (1 track missing in the tack list).
   pf_album_title_v7.diff (9.8 KiB) 
 apps/plugins/pictureflow.c |  128 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 92 insertions(+), 36 deletions(-)

Comment by Thomas Martitz (kugel.) - Tuesday, 24 June 2008, 17:21 GMT+1
Ok, I've noticed the covers were a bit too small on a screen like the one of the h300.

I changed PREFERRED_IMG_WIDTH and _HEIGHT to LCD_HEIGHT / 2 now. This should be fine with the committed smooth scaling. Also, it only uses this when resizing is enabled.
   pf_album_title_v8.diff (9.8 KiB) 
 apps/plugins/pictureflow.c |  128 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 92 insertions(+), 36 deletions(-)

Comment by Jonathan Gordon (jdgordon) - Sunday, 29 June 2008, 04:26 GMT+1
this patch still doesnt work for me.... even after removing the cache folder
Comment by Thomas Martitz (kugel.) - Sunday, 20 July 2008, 19:36 GMT+1
It'd be very nice if people could give some feedback on this patch. jdgordon seems to have problems using this patch, while it works just fine for me (e200 sim+target, h300 sim).
Comment by Marc Guay (Marc_Guay) - Tuesday, 22 July 2008, 01:10 GMT+1
e200

- If the track list is taller than the screen the last song title gets cut in half.
- You can zoom way too far in so that the album titles and FPS are mixed with the art. User's choice, I suppose but maybe worth mentionning.

Aside from that it seems fine to me. Required a delete of the rocks/pictureflow folder.
Comment by Thomas Martitz (kugel.) - Tuesday, 22 July 2008, 15:03 GMT+1
Marc, thanks a lot for your feedback. It's nice to hear that it generally works for you, since I just can't reproduce jdgordons problems.

Regarding your issues:
- I can't confirm that. I've tried some albums with 15+ songs, so that I need to scroll in the track list. The last song was fine in all cases.
- Yea, probably a users choice. I don't see a need to restrict the zoom function in anyway.
- Correct, the pictureflow folder needs to be deleted, because pictureflow uses a rather uncommon way of saving & loading config. Literally all changes to the config struct make existing config files break pictureflow.
Comment by Jonathan Gordon (jdgordon) - Tuesday, 29 July 2008, 10:25 GMT+1
ok, i said id have another look and i did...
the only thing i've changed is so it looks for cover.100x100.bmp if the preffered size isnt found, 88x88 isnt exactly a "standard" size, the cache is made but it still says 0 albums...
   pf_album_title_v9.diff (10.8 KiB) 
 apps/plugins/pictureflow.c |  142 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 103 insertions(+), 39 deletions(-)

Comment by Thomas Schott (scotty) - Sunday, 03 August 2008, 16:00 GMT+1
jdgordon, the problem is that create_albumart_cache() calls create_bmp() and both functions change and use config.avg_album_width in different ways if resize is enabled. v10 should fix this. now the plugin runs fine on my iPodColor :-)
kugel., thanks for this patch, i like the album titles on top ;-)
   pf_album_title_v10.diff (11.6 KiB) 
 apps/plugins/pictureflow.c |  152 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 110 insertions(+), 42 deletions(-)

Comment by Thomas Martitz (kugel.) - Sunday, 03 August 2008, 18:05 GMT+1
Nice. Seems to work well!

JdGordon, I've tried to reproduce your problem. I've got something:
You said it only found a few covers, the debug you showed me said it only found 10 out of much more albums. http://pastebin.ca/1091783

My tests with having a cover.120x120.bmp and a cover.100x100.bmp in each of my 7 test music folders were successful. It found every cover.
I also ran a test with having a cover.bmp in a few of the folders, which also have been successful.

I did my tests with e200 and h300 sim.
Comment by Jonathan Gordon (jdgordon) - Monday, 04 August 2008, 03:14 GMT+1
that was the point of the pastebin... showing a few or showing 10 is the same when I have hundreds on the disk it should have found....
Comment by Thomas Schott (scotty) - Wednesday, 27 August 2008, 22:58 GMT+1
jdgordon, is your problem caused by this patch or does it occur in the unpatched plugin too?
Comment by Thomas Schott (scotty) - Thursday, 28 August 2008, 17:55 GMT+1
I think we should split this patch to separate the fixes introduced in v5 ( FS#8347 ) and v7 (1 track missing in the tack list) from the improvements. The fixes are ready to commit IMHO.

In pf_album_title_v10.fixes.diff I replaced the reset_track_list() call by a simple "start_index_track_list = 0;", which does the job too.

Should we open a new bug report just for the fixes (or reopen  FS#8347 ) to get them committed ASAP?
   pf_album_title_v10.fixes.diff (0.8 KiB) 
 apps/plugins/pictureflow.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

   pf_album_title_v10.improvements.diff (11 KiB) 
 apps/plugins/pictureflow.c |  147 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 106 insertions(+), 41 deletions(-)

Comment by Thomas Schott (scotty) - Thursday, 28 August 2008, 18:04 GMT+1
Forgot to mention:  FS#8347  is closed (Reason: "Fixed: about to ci a fix") but the bug is still alive in r18359 :-(
Comment by Thomas Martitz (kugel.) - Thursday, 28 August 2008, 18:09 GMT+1
Well, I'd like any version to be committed since I have no problems with the resizing.

But I don't have a problem with committing a version without resizing (and without the PREFFERED_IMG stuff rewritten). I just would want to use the config struct from v10, so that a later commit of resizing would break the config again (which in turns means the pf folder needs to be deleted).
Comment by Thomas Schott (scotty) - Thursday, 28 August 2008, 18:39 GMT+1
As Marc_Guay mentioned there are still some open issues with the tracklist on some screensize/fontsize combinations, e.g. on iPodColor with standard font and a tracklist that contains 13 tracks (see attached screenshot).
   tracklist_example.bmp (75.7 KiB)
Comment by Thomas Martitz (kugel.) - Thursday, 28 August 2008, 18:53 GMT+1
This is fixed in this patch.
Comment by Thomas Schott (scotty) - Thursday, 28 August 2008, 19:22 GMT+1
Sure about this? The above screenshot is from r18359 with pf_album_title_v10.diff applied.
Comment by Thomas Martitz (kugel.) - Thursday, 28 August 2008, 19:34 GMT+1
Well, it should be fixed since v2, look at the notes there. Either something reindroduced it, or I...don't know.

I just know, I made the tracklist move up when the album title is at the bottom.
Comment by Thomas Schott (scotty) - Sunday, 31 August 2008, 15:02 GMT+1
New Version:
- fixed (and slightly changed) calculation of tracklist position
- moved the calculation to reset_track_list(), so it's done only once and not in every show_track_list() loop
- revert my replacing of reset_track_list() in PICTUREFLOW_SELECT_ALBUM handling (cause recalculation may also be needed when returning from the settings menu)

Tested on iPodColor and all screen sizes (in sim) with different fontsize/settings combinations.

(Fixes from v5 and v7 are included.)

   pf_album_title_v11.diff (12.3 KiB) 
 apps/plugins/pictureflow.c |  173 +++++++++++++++++++++++++++++++--------------
 1 file changed, 121 insertions(+), 52 deletions(-)

Comment by Thomas Martitz (kugel.) - Saturday, 27 September 2008, 02:47 GMT+1
Sync'd to svn

changed defaults of "Center margin" to -40 (about the first value where's no space between center slide and side slides) and "show album name" to be shown at the top.

slightly changed the album name display to look better (centered in 2 lines)

reset_track_list() only when show_fps or album name setting changed (those are the only one affecting it, no need to redraw on every selection).

Removed the changes to PREFERRED_IMG_WIDTH and _HEIGHT and the album art searching mechanism accordingly, so that it should exactly behave like SVN. Instead, we use RESIZE_WIDTH and _HEIGHT macros now.

Fixed an issue with the centering of the track list.
   pf_album_title_v12.diff (11.2 KiB) 
 apps/plugins/pictureflow.c |  165 ++++++++++++++++++++++++++++++---------------
 1 file changed, 111 insertions(+), 54 deletions(-)

Comment by Thomas Schott (scotty) - Tuesday, 07 October 2008, 17:33 GMT+1
kugel., by removing the reset_track_list() call from main() (case PICTUREFLOW_SELECT_ALBUM) you re-introduced the tracklist bug described in  FS#8347 .
Comment by Thomas Martitz (kugel.) - Tuesday, 07 October 2008, 22:49 GMT+1
Uhm...yea. But resetting the track_list isn't the proper fix. We need to adjust the scroll position according to the saved selected item and scroll automatically.
Comment by Thomas Martitz (kugel.) - Friday, 12 December 2008, 19:23 GMT+1
Update: This should be the final version. At least unless there's a decision on what happens about the resize functions in the pluginlib (this version uses smooth_scaling, not the core function).

BTW: The issue above was a trivial fixed. I can't believe I spotted it so late :/
   pf_album_title_v13_final.diff (11.8 KiB) 
 apps/plugins/pictureflow.c |  173 +++++++++++++++++++++++++++++----------------
 1 file changed, 115 insertions(+), 58 deletions(-)

Comment by Thomas Martitz (kugel.) - Friday, 12 December 2008, 19:24 GMT+1
Oh, I forgot to add: This version also contains the fix Akio Idehara posted here: http://www.rockbox.org/tracker/task/9622
Comment by Jonas Häggqvist (rasher) - Friday, 12 December 2008, 19:37 GMT+1
Why did you add an unrelated bugfix to your patch? There's already a patch for that...
Comment by Thomas Martitz (kugel.) - Friday, 12 December 2008, 19:46 GMT+1
This patch already fixed several issues, fixing one more doesn't hurt.

Plus: I needed to apply it for testing and apparently forgot to remove it before preparing the diff.
Comment by Thomas Martitz (kugel.) - Friday, 12 December 2008, 20:26 GMT+1
Fix  FS#9135  (finally). And remove the fix from  FS#9622 , it needs a better one.
   pf_album_title_v14_final.diff (12.4 KiB) 
 apps/plugins/pictureflow.c |  179 +++++++++++++++++++++++++++++----------------
 1 file changed, 119 insertions(+), 60 deletions(-)

Comment by Thomas Martitz (kugel.) - Saturday, 13 December 2008, 16:49 GMT+1
Sync.

to sum up what improvements are left after the bugfix commit yesterday:
a) album title config (show at top, bottom or not at all)
b) resize of album art to lcd_height/2 (usefull if covers have different sizes or are too small) at cache building. Zoom is still available available after cache building
c) change center margin to a nicer looking default value (-40), so that there's no space between the center cover and the ones next to it instead of a huge gap, also allow more values for center margin
d) some other cleanup's and fixes to bugs which are revealed by album title config
   pf_album_title_v15_final.diff (10.1 KiB) 
 apps/plugins/pictureflow.c |  153 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 110 insertions(+), 43 deletions(-)

Comment by Taylore (trailblaze) - Monday, 22 December 2008, 22:50 GMT+1
lol.all it takes is a few days.. Hmm, is the word "final" final, because i kinda got a hunk error with rockbox 19558?? Hunk #6 at 646 if this helps you or not.. Thank you for making this patch, and it seems usuefull, i just hope i can use it soon.
Comment by Thomas Martitz (kugel.) - Friday, 26 December 2008, 01:33 GMT+1
just in case it wasn't sync'd: here's a new version

it also removes the rather useless (since it's not configurable) search for cover.XxY.bmp
   pf_album_title_v15_final_sync.diff (10.3 KiB) 
 apps/plugins/pictureflow.c |  162 ++++++++++++++++++++++++++++++---------------
 1 file changed, 108 insertions(+), 54 deletions(-)

Loading...