"Add To Playlist" patch

[dunno]

Shebb, thank you for your time and effort, could you post your updated work to the tracker, as I'm trying to get someone else to continue.

Goodluck with college. guess you dont need well wishing for partying.

[Assimalyst]

Shebb, thank you for your time and effort, could you post your updated work to the tracker, as I'm trying to get someone else to continue.

Goodluck with college. guess you dont need well wishing for partying.

Second that. Thanks Shebb and all the best!

[dunno]

an update, safetydan has passed the ball to hardeep,

[Assimalyst]

Cool! Thanks dunno; let's hope it doesn't take too long to get a CVS commit. Keep your inside information coming! ; )

[safetydan]

I am looking at this patch, but would like to be looking at the latest version. Shebb mentioned he had a newer version that fixed most of the issues that LinusN raised. Was that patch ever posted anywhere?

[Assimalyst]

They are posted as attachments throughout this thread. It consists of three files catalog.patch, playlist_catalog.c & playlist_catalog.h. They can be found attached in the following posts:

http://forums.rockbox.org/index.php?topic=2359.msg16592#msg16592
http://forums.rockbox.org/index.php?topic=2359.msg16593#msg16593
http://forums.rockbox.org/index.php?topic=2359.msg16598#msg16598

The outdated sourceforge tracker is here if you need it:

http://sourceforge.net/tracker/index.php?func=detail&atid=439120&group_id=44306&aid=1255322

and linus's comments:

- Several strings aren't localized (the error messages)
- When you add your first playlist to the catalog, you get an unnecessary error message "No catalog found"
- I want to move the "Playlist catalog" entry to the top main menu (the catalog browser)
- Perhaps it would be cleaner to use the gui_synclist API directly instead of the menu API
- When you play a playlist from the catalog browser, it doesn't take you to the WPS

Would be sooo cool if you could get this into CVS

[safetydan]

I have all those. I was hoping that the patch Shebb mentioned here

http://forums.rockbox.org/index.php?topic=2359.msg18006#msg18006

was uploaded somewhere. Would save me some work.

[Assimalyst]

mmmm, sorry, don't know where that one is, but i'll have a hunt . . .

[safetydan]

Okay, attached is step 1 which is to get Shebb's stuff compiling against current CVS. No changes have been made so far. I may not be able to do anything else tonight so I figured I'd at least attach this.


[attachment deleted by admin, too old]

[hardeep]

I had a look at the patch and there are a number of serious issues on top of what Linus already mentioned:

1. The code is statically allocating an array whose size is 25K (100 playlist names * MAX_PATH).  This is way too large because:
a. This is feature is not going to be used by that many people.
b. Most of the people that do use it will almost certainly not be working with 100 playlists.

2. The current version only works on the H100 and H300.  This is mainly because it's not using the gui_* functions as Linus mentioned.

3. There's some code that's duplicated from the playlist code.  I agree with the approach of not using the playlist functionality for this feature but we should try to re-use the code as much as possible.  On the Archos build, this patch increases the firmware size by 5K.

There's still a lot of work to be done to make this code CVS-ready.

-Hardeep

[safetydan]

I agree that there is a lot of work to do. I'm not even sure I like the catalog file idea in the first place. It would be simpler to have a playlist directory where lists for the "catalog" are stored. No need to statically allocate the size of the list then. There seems to be a lot of effort made to avoid just using the directory browser.

Still it's definitely worth looking at.

[LinusN]

I agree that it could be enough with a Playlists directory instead of a catalog. I don't think we should use the browser for the catalog. Too much hassle.

[idioteque]

I just wondered if anyone was still working on making this cvs ready?

and before anyone says it, no, i have no idea how to do that so I can't pick it up myself! 

[dunno]

Last I heard, there were two developers that were "looking" into it, Safetydan and Hardeep, up to now there is no news on any progress in irc on this, I have mentioned it occassionally without badgering the dev's, so guess its busy dying...

[Assimalyst]

Shame, i really liked this feature. Wish i was able to code.