----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/100128/#review314 -----------------------------------------------------------
Ship it! I think he meant const ref, so const QString&. But I'll do it before committing--patch looks good! - Leo On 2010-11-02 03:51:29, Thomas Karpiniec wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/100128/ > ----------------------------------------------------------- > > (Updated 2010-11-02 03:51:29) > > > Review request for Amarok. > > > Summary > ------- > > With this patch a search string of space-separated words will return rows > which match every word in the list. The old behaviour is to match rows > containing the entire search string in any one of the fields searched. > > This permits combination of terms Firefox-style: things like "beatles abbey" > to find all tracks from Abbey Road, or "riv anda" to get "Andalucia" from > Riverdance. > > My only misgiving is that as implemented it will tokenise the search term on > every single attempted row match. As rowMatch() is called both directly in > several functions to find matches in SearchProxy.cpp and after a timeout to > do the row filtering in SortFilterProxy.cpp, pulling the tokenisation up one > level will require either that the tokenisation code is repeated, or that the > search-related functions are all modified to use QStringLists instead of a > QString. If that is a better way I'll do that, but as it stands rowMatch() is > not a particularly small function so the small overhead seems reasonable. > > > Diffs > ----- > > src/playlist/proxymodels/ProxyBase.cpp b6ff53a > > Diff: http://git.reviewboard.kde.org/r/100128/diff > > > Testing > ------- > > Tested on a playlist of ~8000 tracks. Performance seems good, matching is > accurate and it works just fine with single terms or inputs with extra spaces. > > > Thanks, > > Thomas > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
