Web lists-archives.com

Re: Review Request 128664: Nested tags for Baloo




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

On August 19th, 2016, 8:51 a.m. UTC, Vishesh Handa wrote:

This is quite an interesting change.

Could you please also elaborate on how this will change the tags kioslave? I'm afraid I don't quite follow.

This patch brings Digikam-style tags to Baloo.

Exact tag matches weren't possible with nested tags using the term generator, so tags are now indexed without using the term generator. An equal match query is run for the KIO slave. Phrase matches are less effective without the term generator, but things like query phrase matching for tags with spaces or slashes were broken or partially working anyway (no results, the second match was frequently ignored, etc, with 'baloosearch'.).

The rest of the patch is just minor adjustments to the kio slave to allow nested tags to be parsed from the url path and embedding nested tag search results in tag search queries.


On August 19th, 2016, 8:51 a.m. UTC, Vishesh Handa wrote:

src/kioslaves/tags/kio_tags.cpp (Diff revision 1)
103
            if (resultTag.startsWith(tag, Qt::CaseInsensitive) && resultTag.compare(tag, Qt::CaseInsensitive)) {

QString::compare returns 0 on match or < 0 and > 0 depending on which term is larger. I don't quite understand what is going on over here.

What it reads like is 'if it starts with but is equal to itself'

I think it's evaluating as integer false. I've replaced this with a separator check instead, and eliminated the QStringList creation as well.


On August 19th, 2016, 8:51 a.m. UTC, Vishesh Handa wrote:

src/kioslaves/tags/kio_tags.cpp (Diff revision 1)
384
TagsProtocol::ParseResult TagsProtocol::parseUrl(const QUrl& url, QString& tag, QString& fileUrl, bool)
395
TagsProtocol::ParseResult TagsProtocol::parseUrl(const QUrl& url, QString& tag, QString& fileUrl, bool)

This code is getting hard to follow. Do you think you could write a simple unit test for just this function?

That way it will be clear what inputs it can accept.

Also I think this function can be made const.

Making it a const results in a discards qualifiers warning. It's also really simple to not break, and I added some commenting.


On August 19th, 2016, 8:51 a.m. UTC, Vishesh Handa wrote:

src/lib/searchstore.cpp (Diff revision 1)
PostingIterator* SearchStore::constructQuery(Transaction* tr, const Term& term)
58
    m_prefixes.insert(QByteArray("tag"), QByteArray("TA"));
58
    m_prefixes.insert(QByteArray("tag"), QByteArray("TA"));

I think this line along with the next can now be removed.

It looks like these are still used in fetchPrefix().


- James


On August 18th, 2016, 11:56 p.m. UTC, James Smith wrote:

Review request for Baloo and Vishesh Handa.
By James Smith.

Updated Aug. 18, 2016, 11:56 p.m.

Bugs: 334615
Repository: baloo

Description

Index and query each tag as a single full term for generating recursed search results. Represent nested tags as recursed items in the Tags:// KIO Slave.

Testing

Compile, run

Diffs

  • src/file/basicindexingjob.cpp (88bb59a01e5592abb74b1ab345bfc6765d35db57)
  • src/kioslaves/tags/kio_tags.cpp (de2e6d71945632e23a85f831878b4c431360731c)
  • src/lib/searchstore.cpp (060a4fd795ab858eb84526f93f827d09ee85db7c)

View Diff