Review Request: Fix for bug 263693: The Delete Tracks dialog is misleading/ambiguous

[Matěj Laitl]

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


It is good that you factored the common code to its own method, there are however some small string issues, please resolve them:


src/core-impl/collections/support/CollectionLocationDelegateImpl.cpp
<http://git.reviewboard.kde.org/r/102236/#comment9019>

    Hmm, I think you should use name() here instead of prettyName() as prettyName can give "Unknown artist" etc. (please check its implementation)



src/core-impl/collections/support/CollectionLocationDelegateImpl.cpp
<http://git.reviewboard.kde.org/r/102236/#comment9020>

    Same here, name() could be better. (check implementations)



src/core-impl/collections/support/CollectionLocationDelegateImpl.cpp
<http://git.reviewboard.kde.org/r/102236/#comment9015>

    You can (and should) use artistName and trackName here.



src/core-impl/collections/support/CollectionLocationDelegateImpl.cpp
<http://git.reviewboard.kde.org/r/102236/#comment9016>

    This will give:
    path/to/file.mp3(Track Name)
    
    (missing space)



src/core-impl/collections/support/CollectionLocationDelegateImpl.cpp
<http://git.reviewboard.kde.org/r/102236/#comment9017>

    This will give:
    path/to/file.mp3(Artist by )



src/core-impl/collections/support/CollectionLocationDelegateImpl.cpp
<http://git.reviewboard.kde.org/r/102236/#comment9018>

    For easier translatability (think of right-to-left scripts), I suggest you use one format string for the whole string and no strinc concatenation, for example  this (in every if clause:)
    
    str = i18nc("%1 is track url, %2 track title, %3 track artist", "%1 (%2 by %3)", url, trackName, artistName);


- Matěj Laitl


On March 7, 2012, 10:30 p.m., Ryan McCoskrie wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102236/
> -----------------------------------------------------------
> 
> (Updated March 7, 2012, 10:30 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> -------
> 
> Fix for bug 263693. When the user is asked to confirm deleting a file from his/her music collection the prompt will use the songs meta-data in place of the path name if possible.
> 
> 
> This addresses bug 263693.
>     https://bugs.kde.org/show_bug.cgi?id=263693
> 
> 
> Diffs
> -----
> 
>   src/core-impl/collections/support/CollectionLocationDelegateImpl.h 12b975f 
>   src/core-impl/collections/support/CollectionLocationDelegateImpl.cpp fb7c18f 
> 
> Diff: http://git.reviewboard.kde.org/r/102236/diff/
> 
> 
> Testing
> -------
> 
> Ran application and asked to delete several files from collection. Patch worked as expected.
> Deleted meta date from one track and asked to delete that also. Found that Meta::TrackPtr::prettyName()
> will return the file name of the track instead of an empty QString and that Meta::ArtistPtr::prettyName()
> returns 'Unknown Artist' in place of an empty QString. This will render the data checking needless under
> all known circumstances.
> 
> 
> Screenshots
> -----------
> 
> Uses meta-data instead of raw file path
>   http://git.reviewboard.kde.org/r/102236/s/220/
> 
> 
> Thanks,
> 
> Ryan McCoskrie
> 
>

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