Davey Shafik [2004-06-10 23:27 UTC] Would be nice if you could find out the format iTunes uses (just download and install on Mac or PC and look at the .xml files it produces) and parse/create this also :)
Otherwise, not really sure how much use being able to create playlists using PHP is... parsing/rendering on the other hand is a great thing and should be moved to be the highlight of this package IMO.
- Davey
David Costa [2004-06-10 23:38 UTC] Hello Davey,
first and foremost thanks for your comments. I checked the ITunes format (I am using a Mac myself) but I do fear that the format, which is yes XML based but proprietary, doesn't allow a replication without running into legal troubles with Apple ;)
On the other hand M3U and Smil are widely accepted format. The test M3U works on Itunes and with Audion (a Mac OSX playes which works fine with M3U playlists).
There are similar modules on the CPAN but limited to the parsing, which as you said is fairly trivial.
I tried to add a number of rendering methods that might add some validity to the package.
Thanks again!!
Regards
David Costa
Daniel Convissor [2004-06-10 23:46 UTC] I'm looking at http://dotgeek.org/mp3_playlist/source.php?src=0.
How about cleaning up the code to meet PEAR coding standards? For example:
* The horizontal and vertical spacing of your docblocks.
* General horizontal and vertical spacing of the code in general.
Specific examples...
1) the extra space below control structure defs and method
names. Just to point to a few:
a) "if (!@$scanned) {"
b) "public function makeSmil..."
c) "public function makeSqlite..."
2) foreach should have the opening bracket on the same line
3) In "if ($this->debug == true) {" the bracket has more than
one space (or is a tab) and then the print_r() below it is
not nested correctly.
Much of this could be due to the mixed use of tabs and spaces. Please use spaces.
For an example of clean, standard docblocks, please see
http://pear.php.net/manual/en/standards.sample.php
David Costa [2004-06-10 23:55 UTC] Hello Daniel,
thanks for your comments:
"I'm looking at http://dotgeek.org/mp3_playlist/source.php?src=0.
How about cleaning up the code to meet PEAR coding standards?"
>> by all means you are right and I will fix these issues as soon as possible
"
For example:
* The horizontal and vertical spacing of your docblocks.
* General horizontal and vertical spacing of the code in general.
Specific examples......"
I will do it tomorrow, thanks again for spotting this formatting issue.
"Much of this could be due to the mixed use of tabs and spaces. Please use spaces."
This is not my case I do use set expandtab on my config which means that each time I press tab only spaces are included.
"For an example of clean, standard docblocks, please see
http://pear.php.net/manual/en/standards.sample.php"
Thanks. Beside the formatting problem (which will be fixed) let me know if you have any other comment or idea.
Cheers
David Costa
Bertrand Mansion [2004-06-11 08:49 UTC] A driver model for both input and output would lead to a better designed interface. Otherwise, you are going to have to add new methods to the main class or subclass it (bOrk).
"varchart" is not an sql datatype. BTW, sqlite does not need datatypes, that's why you didn't get an error.
About iTunes, see http://www.maczsoftware.com/phptunest/
I didn't look too closely but your usage of ob_ function looks scary. There might be some more elegant ways.
And why make it a PHP5 only class, I don't see any PHP5 only features ?
David Costa [2004-06-11 11:13 UTC] Hello Bertrand!
Thanks for taking some of your valuable time to review my proposal, see my comments below:
"A driver model for both input and output would lead to a better designed interface. Otherwise, you are going to have to add new methods to the main class or subclass it (bOrk)."
>> The structure is pretty similar. We do have an input in the constructor and an output.
"varchart" is not an sql datatype. BTW, sqlite does not need datatypes, that's why you didn't get an error.
>> Fixed to Char now, source updated. Sqlite does need at least to differentiate between 2 datatypes: Text and Numeric "SQLite is typeless for the purpose of deciding what data is allowed to be stored in a column. But some notion of type comes into play when sorting and comparing data. For these purposes, a column or an expression can be one of two types: numeric and text.."
As far as I know it considers text any type that containts BLOB,
CHAR, CLOB, TEXT.
"About iTunes, see http://www.maczsoftware.com/phptunest/"
>> Been there done that ;) that is a different approach thou. I start from the mp3 files and this approach is certainly cross platform (I tested the scrip on my Mac OS X which is my primarly development environment) and more reliable.
In other words I do not transform a proprietary playlist into another format but from the mp3 files I do create playlists in several formats including the open standards M3U and SMIL which are supported on Mac too. In any case I do value your suggetion.
"I didn't look too closely but your usage of ob_ function looks scary. There might be some more elegant ways."
>> you are right, I will try to think on something else for that.
And why make it a PHP5 only class, I don't see any PHP5 only features ?
>> Sqlite for instance. True Sqlite works on PHP4 too but with PHP 5.0.0. very close I started (since beta 1) to code every class in PHP 5.
Once again thanks for your time and attention,
Regards
David Costa
Stefan Neufeind [2004-06-12 08:05 UTC] The class looks fine to me - except a few things:
- Imho there should be access-rights given in the docblocks for your variables - even if it were @access public, but you might maybe reevalute this as well.
- maybe make...() should be to...()? Though there is no strict rule for it, but isn't this commonly used?
- imho you should also implement ways to not just generate a playlist by parsing a directory but also allow to manually create a playlist, add items to it using functions, read an existing playlist from a variety of formats etc.
And just for info: For the unclean MP3_ID-workaround I just filed a bug. :-)
David Costa [2004-06-22 16:15 UTC] Updated the code as per discussion on pear-dev the package is now
fully E_STRICT compliant, using the PEAR_Exceptions class in lieu of the pear errors.
The package is also no longer dependant on MP3_ID for the tags retrieval.
Feel free to have a look ;)
|
