D13670: Add subseq operator to match sub-sequences

[David Faure]

dfaure closed this revision.

REPOSITORY
  R309 KService

REVISION DETAIL
  https://phabricator.kde.org/D13670

To: michaeleden, dfaure, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13670: Add subseq operator to match sub-sequences

[Michael Eden]

michaeleden added a dependent revision: D13988: Use subseq matching for service 
runner.

REPOSITORY
  R309 KService

BRANCH
  feature/match-subseq

REVISION DETAIL
  https://phabricator.kde.org/D13670

To: michaeleden, dfaure, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13670: Add subseq operator to match sub-sequences

[Michael Eden]

michaeleden added a comment.


  @dfaure you can land it its no problem. I'm also not in a rush to get it into 
5.48 or anything (Bug 262837 is from 2011).
  I have a few diffs in review, I think I'll apply for a dev account since I'd 
like to keep doing this stuff.

REPOSITORY
  R309 KService

BRANCH
  feature/match-subseq

REVISION DETAIL
  https://phabricator.kde.org/D13670

To: michaeleden, dfaure, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13670: Add subseq operator to match sub-sequences

[David Faure]

dfaure added a comment.


  Ah, so you don't have a developer account, which means you can't push this 
commit either, and I should do it for you?
  
  Well, you could also apply for a developer account ;)
  
https://community.kde.org/Infrastructure/Get_a_Developer_Account#Apply_for_a_KDE_Developer_account
 (I hope it's still accurate).
  
  I'm tagging 5.48 today though, is there a rush to get this in, or can it wait 
for 5.49?

REPOSITORY
  R309 KService

BRANCH
  feature/match-subseq

REVISION DETAIL
  https://phabricator.kde.org/D13670

To: michaeleden, dfaure, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13670: Add subseq operator to match sub-sequences

[Michael Eden]

michaeleden added a comment.


  @dfaure I don't believe I have permission to edit the wiki, can I make a 
revision against it somehow?

REPOSITORY
  R309 KService

BRANCH
  feature/match-subseq

REVISION DETAIL
  https://phabricator.kde.org/D13670

To: michaeleden, dfaure, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13670: Add subseq operator to match sub-sequences

[David Faure]

dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.


  Perfect, thanks!

REPOSITORY
  R309 KService

BRANCH
  feature/match-subseq

REVISION DETAIL
  https://phabricator.kde.org/D13670

To: michaeleden, dfaure, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13670: Add subseq operator to match sub-sequences

[Michael Eden]

michaeleden updated this revision to Diff 37301.
michaeleden added a comment.


  Respond to @dfaure comments by adding tests and separating subseq func

REPOSITORY
  R309 KService

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13670?vs=36493&id=37301

BRANCH
  feature/match-subseq

REVISION DETAIL
  https://phabricator.kde.org/D13670

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kservicetest.cpp
  autotests/kservicetest.h
  src/services/ktraderparse.cpp
  src/services/ktraderparse_p.h
  src/services/ktraderparsetree.cpp
  src/services/ktraderparsetree_p.h
  src/services/lex.l
  src/services/yacc.y

To: michaeleden, dfaure, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13670: Add subseq operator to match sub-sequences

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> ktraderparsetree.cpp:443
> +
> +  QString::Iterator i = c2.str.begin(), j = c1.str.begin();
> +  for (; i != c2.str.end() && j < c1.str.end(); ++i) {

Nothing is being modified, so this should use constBegin(), constEnd(), and 
const_iterator.

> ktraderparsetree.cpp:444
> +  QString::Iterator i = c2.str.begin(), j = c1.str.begin();
> +  for (; i != c2.str.end() && j < c1.str.end(); ++i) {
> +if ((chk_case && *i == *j) || (!chk_case && i->toLower() == 
> j->toLower())) {

This code (the actual algorithm) should be extracted into a function, and a 
unittest should be written for it (to make sure all corner cases are correctly 
handled).

Good opportunity to document the kind of matching being done, too, as you had 
to explain to me in this review request.

And then once this commit is in, it should be documented on 
https://techbase.kde.org/Development/Tutorials/Services/Traders, please 
remember to do so.

REPOSITORY
  R309 KService

REVISION DETAIL
  https://phabricator.kde.org/D13670

To: michaeleden, dfaure, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13670: Add subseq operator to match sub-sequences

[Michael Eden]

michaeleden added a comment.


  @dfaure if a user was trying to find `LibreOffice 6.0 Writer`
  
  with `subin` they would have type `libre` to narrow it down to LibreOffice 
programs, then `libreoffice 6.0 writer` to get to that application.
  with `subseq` they can type `libre` to get to LibreOffice programs but then 
just have `librewrite` match with `LibreOffice 6.0 Writer`
  
  This is related to Bug 262837 

REPOSITORY
  R309 KService

REVISION DETAIL
  https://phabricator.kde.org/D13670

To: michaeleden, dfaure, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13670: Add subseq operator to match sub-sequences

[David Faure]

dfaure added a comment.


  I'm confused, what's the difference with subin?

REPOSITORY
  R309 KService

REVISION DETAIL
  https://phabricator.kde.org/D13670

To: michaeleden, dfaure, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13670: Add subseq operator to match sub-sequences

[Michael Eden]

michaeleden added a reviewer: Frameworks.

REPOSITORY
  R309 KService

REVISION DETAIL
  https://phabricator.kde.org/D13670

To: michaeleden, dfaure, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13670: Add subseq operator to match sub-sequences

[Michael Eden]

michaeleden created this revision.
michaeleden added a reviewer: dfaure.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: kde-frameworks-devel.
michaeleden requested review of this revision.

REVISION SUMMARY
  Added `subseq` and `~subseq` operators which match subsequences of strings.
  This is useful to filter out matches quicker:
  
$ ktraderclient5 --constraint "'libremath' ~subseq Name" --servicetype 
'Application' --short
servicetype is : Application
constraint is : 'libremath' ~subseq Name
got 1 offers.

/nix/store/zq1kgdxk426qgf06nzpd7xqkpk4z5hh2-system-path/share/applications/math.desktop

TEST PLAN
  I'll need help knowing where tests for this will go.

REPOSITORY
  R309 KService

BRANCH
  feature/match-subseq

REVISION DETAIL
  https://phabricator.kde.org/D13670

AFFECTED FILES
  src/services/ktraderparse.cpp
  src/services/ktraderparse_p.h
  src/services/ktraderparsetree.cpp
  src/services/ktraderparsetree_p.h
  src/services/lex.l
  src/services/yacc.y

To: michaeleden, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns