Review Request 109752: JJ 316128: Handle Data CDs in Amarok

[Matěj Laitl]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/109752/#review29956
-----------------------------------------------------------


Looks rather good, please resolve minor issues below. Also please add ChangeLog entry under CHANGES (on top) mentioning the bug number.

WRT: to testing: what have you tested with, udisks1 or udisks2? Which KDE version? What happens with mixed audio CD with both CDDA tracks and data track?


src/core-impl/collections/umscollection/UmsCollection.cpp
<http://git.reviewboard.kde.org/r/109752/#comment22328>

    I think this check should be moved up above the while loop, as the StoregeAccess device should be directly the OpticalDisc device.



src/core-impl/collections/umscollection/UmsCollection.cpp
<http://git.reviewboard.kde.org/r/109752/#comment22325>

    nitpick: Amarok conding style says "OpticalDisc *opt" rather than "OpticalDisc * opt".
    
    Also "opt" sounds like "option" to me, "disc" would be a better variable name.



src/core-impl/collections/umscollection/UmsCollection.cpp
<http://git.reviewboard.kde.org/r/109752/#comment22326>

    "if (" -> "if("



src/core-impl/collections/umscollection/UmsCollection.cpp
<http://git.reviewboard.kde.org/r/109752/#comment22327>

    This debugging statement is only useful when developing please remove it. (along with the braces)


- Matěj Laitl


On March 26, 2013, 10:57 p.m., Anmol Ahuja wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109752/
> -----------------------------------------------------------
> 
> (Updated March 26, 2013, 10:57 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> -------
> 
> Added a check for data CDs in UmsCollection.cpp so they can be handled by UmsCollection in Amarok
> 
> 
> Diffs
> -----
> 
>   src/core-impl/collections/umscollection/UmsCollection.cpp 2b5c53b 
> 
> Diff: http://git.reviewboard.kde.org/r/109752/diff/
> 
> 
> Testing
> -------
> 
> Tested a data CD to see if it plays
> 
> 
> Thanks,
> 
> Anmol Ahuja
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20130327/ae095258/attachment.html>