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

Thu May 2 21:08:35 BST 2013

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

collectionlist.h
<http://git.reviewboard.kde.org/r/110273/#comment23798>

    m_excludeFiles is a poor name for what this really is (a list of folders to exclude files from).

    This carried over from the name chosen in playlistcollection.h

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

    You can remove this by pre-processing m_excludeFiles when CollectionList is constructed.

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

    Prefer "if(" to "if (" in JuK source.

    Also, .startsWith() should work here instead of .contains()

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

    if(

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

    You should prefer const QString & as opposed to QString here, to avoid making temporary copies of m_excludeFiles.

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

    You should be able to remove this by preprocessing m_excludeFiles to be canonical-form at all times (see CollectionList constructor for applicable comment)

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

    You should be able to use .startsWith() for the test instead of .contains().

    contains will search the whole string for "path" but startsWith can abort the search as soon as it encounters a character that doesn't match on both sides.

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

    m_excludeFiles is used very frequently in a loop while loading the playlists and collection list.

    You'll want to do any possible heavy-processing here, as soon as it's loaded, so that you don't have to do it in the loops.

    What that means is you want to make sure every path in m_excludeFiles is converted to canonical form at this point.

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

    This should fix the TODO you have listed in playlistcollection.cpp about the existing excludeDirs not showing up. Is the TODO still valid?

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

    Instead of updating m_result.excludeDirs in parallel with the model, why not just re-read the updated model's QStringList into m_result.excludeDirs after the loop is completed, just as you do in the corresponding "add directories" method at line 154?

directorylistbase.ui
<http://git.reviewboard.kde.org/r/110273/#comment23781>

    "Exclude" is misspelled in the name here.

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

    It's better to have this be a "const QString &" directory parameter to avoid being forced to make a copy of the list returned by m_collection->excludeFiles()...

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

    But that does mean you'll need to declare a different QString here to hold the canonical path (though, the QString can be const at least since it's not further modified).

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

    Be careful of trailing whitespace. Most editors can be configured to display it (and kate/kwrite can also be configured to strip it out when saving the file).

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

    This method name should be more descriptive. It returns a list of folders, not files. Something like "QStringList foldersExcludedFromSearch()" or eve "QStringList excludedFolders()" would be better.

    *ALSO*, this method should be declared const (see the method just above for an example).

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

    Trailing whitespace.

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

    You may need to remember to feed the excludeDirs back into the list view when constructing the DirectoryList dialog to fix this TODO.

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

    Trailing whitespace.

I haven't had a lot of time to do in-depth review (in fact I haven't even applied the patch) but I have to go again (should have left 6 minutes ago) so I wanted to at least leave some initial feedback on the implementation.

I want to say that I like the idea and that the implementation is much better than I would expect from a first-time contributor to a new codebase, with obvious attention paid to things like how to remove selected items in a list view, canonical filenames, etc. So don't take the number of comments as an indicator of the quality I perceive.

- Michael Pyne

On May 2, 2013, 2:41 p.m., Tom Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110273/
> -----------------------------------------------------------
> 
> (Updated May 2, 2013, 2:41 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.
> 
> 
> Thanks,
> 
> Tom Li
> 
>

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