Review Request 116640: Fix for "Bug 331798 - Wrong media shown as current media in playlist after reordering"

[Shantanu Tushar]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/116640/#review52353
-----------------------------------------------------------


Seems to work as expected, but has few issues listed below.


libs/mediacenter/playlistmodel.cpp
<https://git.reviewboard.kde.org/r/116640/#comment37065>

    firstIndex, secondIndex are poor parameter names as they don't tell you what they mean.
    Better names would be something like originalIndex, newIndex (if I understood your logic right)



libs/mediacenter/playlistmodel.cpp
<https://git.reviewboard.kde.org/r/116640/#comment37066>

    Rather than using this -1 as a flag, just call setCurrentIndex wherever required inside the if/else



libs/mediacenter/playlistmodel.cpp
<https://git.reviewboard.kde.org/r/116640/#comment37063>

    Change this to else if {



libs/mediacenter/playlistmodel.cpp
<https://git.reviewboard.kde.org/r/116640/#comment37064>

    again, else if {


- Shantanu Tushar


On March 7, 2014, 1:50 p.m., Ashish Madeti wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/116640/
> -----------------------------------------------------------
> 
> (Updated March 7, 2014, 1:50 p.m.)
> 
> 
> Review request for Plasma, Shantanu Tushar and Sinny Kumari.
> 
> 
> Bugs: 331798
>     http://bugs.kde.org/show_bug.cgi?id=331798
> 
> 
> Repository: plasma-mediacenter
> 
> 
> Description
> -------
> 
> Patch for Bug 331798 : Playlist shows the wrong song as "now playing" (in red color) in some cases of reordering the playlist.
> 
> 
> Diffs
> -----
> 
>   libs/mediacenter/playlistmodel.cpp 6477047 
>   mediaelements/playlist/Playlist.qml d578720 
>   mediaelements/playlist/PlaylistDelegate.qml a5f6ebb 
>   mediaelements/playlist/PlaylistDelegateLogic.js e69de29 
> 
> Diff: https://git.reviewboard.kde.org/r/116640/diff/
> 
> 
> Testing
> -------
> 
> 1.Moved a song with index less than the currently playing media to some place below the currently playing media.
> 2.Moved a song with index greater than the currently playing media to some place above the currently playing media.
> 3.Moved the currently playing media to various places in the playlist.
> 
> In all the cases correct song is now showed as Now Playing i.e in "red" color.
> 
> 
> Thanks,
> 
> Ashish Madeti
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20140307/e6c67d8f/attachment.html>