Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Patches
  • Category Plugins
  • Assigned To No-one
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version Daily build (which?)
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by kugel. - 2007-12-16
Last edited by Unhelpful - 2008-12-27

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

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)

Closed by  Unhelpful
2008-12-27 12:42
Reason for closing:  Accepted
Additional comments about closing:   Warning: Undefined array key "typography" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 371 Warning: Undefined array key "camelcase" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 407

Parts of this, and equivalents for the rest, merged in r19599.

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

Just fixed a few typos in comments, and added one. No functional change.

Here’s the file.

No fuctional change. Made an enum for show_album title for easier-to-read-code, some changes to the comments.

Sync and implement smooth_resize. Problem: It doesn’t resize.

Here’s the patch.

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.

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.

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.

Sync, also fix a bug introduced r17781 (1 track missing in the tack list).

Sync, also fix a bug introduced r17781 (1 track missing in the tack list).

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.

this patch still doesnt work for me…. even after removing the cache folder

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).

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.

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.

ok, i said id have another look and i did… the only thing i’ve changed is so it looks for cover.100×100.bmp if the preffered size isnt found, 88×88 isnt exactly a “standard” size, the cache is made but it still says 0 albums…

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 ;-)

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.120×120.bmp and a cover.100×100.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.

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….

jdgordon, is your problem caused by this patch or does it occur in the unpatched plugin too?

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?

Forgot to mention:  FS#8347  is closed (Reason: “Fixed: about to ci a fix”) but the bug is still alive in r18359 :-(

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).

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).

This is fixed in this patch.

Sure about this? The above screenshot is from r18359 with pf_album_title_v10.diff applied.

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.

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.)

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.

kugel., by removing the reset_track_list() call from main() (case PICTUREFLOW_SELECT_ALBUM) you re-introduced the tracklist bug described in  FS#8347 .

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.

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 :/

Oh, I forgot to add: This version also contains the fix Akio Idehara posted here: http://www.rockbox.org/tracker/task/9622

Why did you add an unrelated bugfix to your patch? There’s already a patch for that…

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.

Fix  FS#9135  (finally). And remove the fix from  FS#9622 , it needs a better one.

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

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.

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

Loading...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing