----------------------------------------------------------- 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 > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
