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

[Ralf Engels]

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


Sorry for the late response.

Still a nice patch, except for the slight issues:


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

    Please use the artist variable declared above.
    Also you need to check if artist is set.
    
    Please note that Meta::ArtistPtr is a shared pointer, so as long as it stays in scope the artist will not be deleted.
    
    If you write something like this however:
    if( track->artist() && !track->artist()->prettyName().isEmpty())
    
    then the artist might be deleted by another process directly at the && :)
    
    Also please use isEmpty() instead of isNull()



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

    It is impossible to localize that sensible in all languages.
    There might be languages (Arabic for example) where the order of the parameters need to be exchanged.
    That is the reason why Qt has numbered parameters.
    
    Also there might be languages (Chinese) where the spaces need to be removed.
    
    So please put it in one line with " (%1 by %2)"


Also you copied the code four times.
Please make a function out of it.
You might call it longTrackName( Meta::TrackPtr track ) or something like this.

- Ralf Engels


On Sept. 10, 2011, 10:26 p.m., Ryan McCoskrie wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102236/
> -----------------------------------------------------------
> 
> (Updated Sept. 10, 2011, 10:26 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.cpp 349464c 
> 
> Diff: http://git.reviewboard.kde.org/r/102236/diff/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/20120116/e57315f7/attachment-0001.html>