From kde-devel Sun Sep 04 09:51:04 2016 From: Vishesh Handa Date: Sun, 04 Sep 2016 09:51:04 +0000 To: kde-devel Subject: Re: Review Request 128664: Nested tags for Baloo Message-Id: <20160904095104.21849.76968 () mimi ! kde ! org> X-MARC-Message: https://marc.info/?l=kde-devel&m=147298267507348 --===============5964759505697917154== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128664/#review98860 ----------------------------------------------------------- Please test out all possible workflows. Ideally it would be great if we could have automated tests for all the workflows with tags. src/file/basicindexingjob.cpp (line 95) Please add a test for this - We have a basicindexingjobtest file where the tests were "temporarily" removed. They clearly need to be added again with the behaviour this class expects. src/file/basicindexingjob.cpp (line 107) Alright. So now TA instead of being a prefix to store each word, it now stores the entire tag. This breaks existing behaviour. That's typically frowned upon. Example - All tags with more than 1 word. Perhaps it would just be easier if we would intrduce a new prefix. src/kioslaves/tags/kio_tags.cpp (line 91) So if we only have tag in the form - "Fire/Water" at that point unless one also has the tag "Fire", it will just never show up. - Vishesh Handa On Aug. 20, 2016, 10:58 p.m., James Smith wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/128664/ > ----------------------------------------------------------- > > (Updated Aug. 20, 2016, 10:58 p.m.) > > > Review request for Baloo and Vishesh Handa. > > > Bugs: 334615 > http://bugs.kde.org/show_bug.cgi?id=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. > > > Diffs > ----- > > src/file/basicindexingjob.cpp 88bb59a01e5592abb74b1ab345bfc6765d35db57 > src/kioslaves/tags/kio_tags.cpp de2e6d71945632e23a85f831878b4c431360731c > src/lib/searchstore.cpp 060a4fd795ab858eb84526f93f827d09ee85db7c > > Diff: https://git.reviewboard.kde.org/r/128664/diff/ > > > Testing > ------- > > Compile, run > > > Thanks, > > James Smith > > --===============5964759505697917154== MIME-Version: 1.0 Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: 7bit
This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128664/ 

Please test out all possible workflows. Ideally it would be great if we could have automated tests for all the workflows with tags.

src/file/basicindexingjob.cpp (Diff revision 2)
95 
bool BasicIndexingJob::indexXAttr(const QString& url, Document& doc)
 95 
bool BasicIndexingJob::indexXAttr(const QString& url, Document& doc)
 
Please add a test for this -


We have a basicindexingjobtest file where the tests were "temporarily" removed. They clearly need to be added again with the behaviour this class expects.

src/file/basicindexingjob.cpp (Diff revision 2)
106 
            doc.addXattrBoolTerm(QByteArray("TAG-") + tag.toUtf8());
 107 
                doc.addXattrTerm(QByteArray("TA") + tag.toUtf8());
 
Alright. So now TA instead of being a prefix to store each word, it now stores the entire tag.


This breaks existing behaviour. That's typically frowned upon. Example - All tags with more than 1 word.


Perhaps it would just be easier if we would intrduce a new prefix.

src/kioslaves/tags/kio_tags.cpp (Diff revision 2)
87 
        TagListJob* job = new TagListJob();
 91 
        Q_FOREACH (const QString& resultTag, job->tags()) {
 
So if we only have tag in the form -


"Fire/Water"


at that point unless one also has the tag "Fire", it will just never show up.

- Vishesh Handa


On August 20th, 2016, 10:58 p.m. UTC, James Smith wrote:

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

Updated Aug. 20, 2016, 10:58 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
--===============5964759505697917154==--