Review Request 110273: Juk should be able to exclude folders.

[Michael Pyne]

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



collectionlist.cpp
<http://git.reviewboard.kde.org/r/110273/#comment24066>

    We can do both here since .canonicalPath() returns a new QString:
    
    // m_excludeFolders is cleared here
    foreach(const QString &folder, collection->excludedFolders()) {
        m_excludeFolders << folder.canonicalPath();
    }



directorylist.h
<http://git.reviewboard.kde.org/r/110273/#comment24067>

    I missed this earlier, but it should be excludedDirs to match the verb tense of addedDirs and removedDirs



directorylist.cpp
<http://git.reviewboard.kde.org/r/110273/#comment24068>

    I didn't notice this last time because I only reviewed online, but this line uses tabs for indentation instead of spaces.
    
    This isn't a big issue as I would manually fix before applying, but it's something to watch for in existing code bases.



directorylist.cpp
<http://git.reviewboard.kde.org/r/110273/#comment24069>

    This whole line can be removed I think.



playlist.cpp
<http://git.reviewboard.kde.org/r/110273/#comment24070>

    if( again :)



playlistcollection.h
<http://git.reviewboard.kde.org/r/110273/#comment24071>

    The method name didn't get changed.
    
    It shouldn't be "excludeFolders" as that is a present-tense verb, which means that some action would be performed by calling this method.
    
    Instead this is an informational method, which we normally use past-tense verbs for.
    
    E.g. "excludedFolders" (notice the extra 'd' in front of Folders)
    
    Also this should be a const method still, as noted in the first issue.



playlistcollection.cpp
<http://git.reviewboard.kde.org/r/110273/#comment24072>

    I understand your TODO now.
    
    However I would suggest that this feature shouldn't be used to prevent a file from *ever* showing up in a playlist, but only to prevent JuK from automatically finding it.
    
    In other words, the user should be able to manually add the file, even from an excluded folder.
    
    I'll have to review the code further to see if this works correctly already or not.


Overall pretty good, most (though not all) of the initially seen issues were fixed.

Still a couple of style issues but that's not a big deal as I can fix those.

I'm currently running this code as well so I can at least confirm it doesn't break JuK.

My only major point of concern is that it should still be possible to manually add files from an excluded folder. We just don't want to have JuK automatically search excluded folders. I'll need to review the codepaths to see if that's still an issue. If it is we can still go ahead and commit (after the issues noted in this pass are fixed) and then I can adjust to fix the new "bug" introduced before the next release.

- Michael Pyne


On May 11, 2013, 7:21 p.m., Tom Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110273/
> -----------------------------------------------------------
> 
> (Updated May 11, 2013, 7:21 p.m.)
> 
> 
> Review request for KDE Multimedia.
> 
> 
> Description
> -------
> 
> If we add some folders to Juk's watching list, Juk will add everything
> in the folder. But sometimes, we don't want Juk to add some folders.
> There isn't a way to exclude folders.
> 
> It is bugging me all the time. I can't stand it so I starting patching
> it with a little C++ and Qt knowledge.
> 
> Here is my patch. If my patch can merge into mainline, our users
> should be happier :). I don't have much C++ experience, there are some
> hacks in my code, but that's the best what I can do. So please help me
> to improve it.
> 
> 
> This addresses bug 319106.
>     http://bugs.kde.org/show_bug.cgi?id=319106
> 
> 
> Diffs
> -----
> 
>   collectionlist.h e8c15de 
>   collectionlist.cpp f4df66b 
>   directorylist.h f13756f 
>   directorylist.cpp b715a2c 
>   directorylistbase.ui 6146726 
>   playlist.h 1fc640b 
>   playlist.cpp 2153f9b 
>   playlistcollection.h d9fd9ff 
>   playlistcollection.cpp fbb33a6 
> 
> Diff: http://git.reviewboard.kde.org/r/110273/diff/
> 
> 
> Testing
> -------
> 
> Yes, it works for me.
> But I need some help to improve the patch. Nobody reviews the new version? 
> 
> 
> Thanks,
> 
> Tom Li
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-multimedia/attachments/20130511/299d6fe0/attachment.htm>
-------------- next part --------------
_______________________________________________
kde-multimedia mailing list
kde-multimedia at kde.org
https://mail.kde.org/mailman/listinfo/kde-multimedia