Code review comment for lp:~pwhelan/mixxx/shoutcast

Revision history for this message
Phillip Whelan (pwhelan) wrote :

> Review: Needs Information
> This looks like it includes your recent shoutcast-metadata-update.patch
and
> shoutcast-scons-autodeps.patch patches, am I correct?

More for the most part, yes you are correct. I did extensive testing on the
metadata update code and fixed it to actually work. I was doing the
crossfader conditional totally wrong. I also changed the code to use
properly instantiated ControlObjectThread objects which are instantiated
once, instead of for each pass of the code which detects which is the
active track.

>
> On lines 295-297 of the merge proposal, you convert QStrings to
> latin1-encoded char* strings. Does libshout support utf8? If so, use
> toUtf8() instead of toLatin1(). If not, leave it the way it is.

I'll check into that, but I seriously doubt that libshout supports UTF-8,
and if so, Shoutcast might not support metadata in UTF-8.

> Just a heads up - In my sqlite branch, I have a real "Player" class which
> should probably deprecate the PlayerInfo class. It'll probably be my job
to
> modify your code to use that since your code will be in trunk before
mine.
> :)

That'd be great, on both accounts.

> Anyways, looks good otherwise. For sanity, I also think this should go in
> the 1.7 branch A) because that's where the latest shoutcast code is
> otherwise (it might save us some merging headaches later) and B) all of
> your changes only apply when Mixxx is built with shoutcast=1.

I also thought the same.

> Thanks!
Your welcome!

« Back to merge proposal