D23457: Port regex search to QRegularExpression

[Ahmad Samir]

ahmadsamir added a comment.


  Disagreement doesn't always mean distrust.

REPOSITORY
  R39 KTextEditor

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

To: ahmadsamir, #ktexteditor, dhaumann, cullmann
Cc: kde-frameworks-devel, kwrite-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D23457: Port regex search to QRegularExpression

[Ahmad Samir]

ahmadsamir updated this revision to Diff 71617.
ahmadsamir added a comment.


  Address comments

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23457?vs=71178=71617

BRANCH
  l-qregularexpression (branched from master)

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

AFFECTED FILES
  autotests/src/regexpsearch_test.cpp
  autotests/src/searchbar_test.cpp
  src/CMakeLists.txt
  src/document/katedocument.cpp
  src/search/kateplaintextsearch.cpp
  src/search/kateregexp.cpp
  src/search/kateregexp.h
  src/search/kateregexpsearch.cpp
  src/search/kateregexpsearch.h
  src/search/katesearchbar.cpp
  src/search/katesearchbar.h
  src/utils/katesedcmd.cpp
  src/utils/katesedcmd.h

To: ahmadsamir, #ktexteditor, dhaumann, cullmann
Cc: kde-frameworks-devel, kwrite-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D23457: Port regex search to QRegularExpression

[Dominik Haumann]

dhaumann added a comment.


  In D23457#574846 , @ahmadsamir 
wrote:
  
  > Personally, I am not convinced replacing \s with [ \t], and deviating from 
PCRE default behaviour is a good idea in this case.
  
  
  That is not to be discussed, and Christoph tried to explain this already 
before.
  
  In short: If these issues don't get fixed to be compatible with previous 
behavior, this patch will not get merged. I'd be very much in favor of having 
the compatibility, since QRegularExpression is the way forward, but adding 
regressions is a big no-go and not up for discussion.
  
  We are maintaining Kate for close to 20 years now. We'd like to ask you to 
give us some trust in our decision.

REPOSITORY
  R39 KTextEditor

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

To: ahmadsamir, #ktexteditor, dhaumann, cullmann
Cc: kde-frameworks-devel, kwrite-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D23457: Port regex search to QRegularExpression

[Ahmad Samir]

ahmadsamir added a comment.


  Personally, I am not convinced replacing \s with [ \t], and deviating from 
PCRE default behaviour is a good idea in this case.

REPOSITORY
  R39 KTextEditor

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

To: ahmadsamir, #ktexteditor, dhaumann, cullmann
Cc: kde-frameworks-devel, kwrite-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D23457: Port regex search to QRegularExpression

[Dominik Haumann]

dhaumann added a comment.


  Was anything of the previously commented issues addressed?

REPOSITORY
  R39 KTextEditor

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

To: ahmadsamir, #ktexteditor, dhaumann, cullmann
Cc: kde-frameworks-devel, kwrite-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D23457: Port regex search to QRegularExpression

[Ahmad Samir]

ahmadsamir updated this revision to Diff 71178.
ahmadsamir added a comment.


  clang-fromat the code in this diff

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23457?vs=71059=71178

BRANCH
  l-qregularexpression (branched from master)

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

AFFECTED FILES
  autotests/src/regexpsearch_test.cpp
  autotests/src/regexpsearch_test.h
  autotests/src/searchbar_test.cpp
  src/CMakeLists.txt
  src/document/katedocument.cpp
  src/search/kateplaintextsearch.cpp
  src/search/kateregexp.cpp
  src/search/kateregexp.h
  src/search/kateregexpsearch.cpp
  src/search/kateregexpsearch.h
  src/search/katesearchbar.cpp
  src/search/katesearchbar.h
  src/utils/katesedcmd.cpp
  src/utils/katesedcmd.h

To: ahmadsamir, #ktexteditor, dhaumann, cullmann
Cc: kde-frameworks-devel, kwrite-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D23457: Port regex search to QRegularExpression

[Ahmad Samir]

ahmadsamir updated this revision to Diff 71059.
ahmadsamir added a comment.


  Rebase

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23457?vs=64618=71059

BRANCH
  l-qregularexpression (branched from master)

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

AFFECTED FILES
  autotests/src/regexpsearch_test.cpp
  autotests/src/regexpsearch_test.h
  autotests/src/searchbar_test.cpp
  src/CMakeLists.txt
  src/document/katedocument.cpp
  src/search/kateplaintextsearch.cpp
  src/search/kateregexp.cpp
  src/search/kateregexp.h
  src/search/kateregexpsearch.cpp
  src/search/kateregexpsearch.h
  src/search/katesearchbar.cpp
  src/search/katesearchbar.h
  src/utils/katesedcmd.cpp
  src/utils/katesedcmd.h

To: ahmadsamir, #ktexteditor, dhaumann, cullmann
Cc: kde-frameworks-devel, kwrite-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D23457: Port regex search to QRegularExpression

[Ahmad Samir]

ahmadsamir added a comment.


  In D23457#568067 , @dhaumann wrote:
  
  > Any news here?
  
  
  For starters, this needs to be rebased; I'll try and get to it soon.

REPOSITORY
  R39 KTextEditor

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

To: ahmadsamir, #ktexteditor, dhaumann, cullmann
Cc: kde-frameworks-devel, kwrite-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D23457: Port regex search to QRegularExpression

[Dominik Haumann]

dhaumann added a comment.


  Any news here?

REPOSITORY
  R39 KTextEditor

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

To: ahmadsamir, #ktexteditor, dhaumann, cullmann
Cc: kde-frameworks-devel, kwrite-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D23457: Port regex search to QRegularExpression

[Ahmad Samir]

ahmadsamir added a comment.


  In D23457#519773 , @cullmann wrote:
  
  > In D23457#519217 , @ahmadsamir 
wrote:
  >
  > > Maybe they'll also see it as ktexteditor/kate using a regex engine that 
matches what the abundance of online pcre docs say, and how other editors that 
use pcre behave?
  > >
  > > IIUC, '\s' was workedaround so as not to match a newline so that the 
search pattern wouldn't be considered multiline (isMultiLine() function), which 
makes findAll and replaceAll slower as it took longer, v.s. just matching 
against each line separately.
  >
  >
  > Actually, it is not faster.
  >  If you take a look at the code, for single line regex, it iterates over 
the individual lines.
  >  For multi line regexes, it will first concatinate all lines into one 
buffer.
  >  For large files that is "very" slow.
  >  And if you e.g. search + hit then "next match", this will be done again 
and again.
  
  
  I was mainly talking about find/replaceAll operations; qregularexpression is 
quite fast, I dabbled with using a global match and doing a findAll in one go, 
that was fast, but the code got complicated quite fast too. As I found out, 
ktexteditor wants the matches fed back to it one by one, since it has to do a 
lot of other stuff: highlighting, replacing text, undo history, buffer stuff, 
moving ranges... etc.
  [..]
  
  >> The thing is, what kateregexp did was replace '\s' with '[ \t]', which 
users who want this behaviour can easily use.
  > 
  > That is true, perhaps we should add this as extra into the menu as 
proposal, like \s/...
  
  There's only so many menu entries that can be added, new users will have to 
read the docs at some point, regex is a complicated minefield.
  
  >> Technically it's a whole new class, QRegularExpression, some different 
behaviours are sort of expected...
  > 
  > ;=) That is really no good reasoning why one changes a behavior.
  >  It is clear that if you port something over to a new class, behavior might 
change, but that doesn't make it a good thing per default.
  
  True. But I also meant, that would be a good time to introduce new 
behaviours, as long as they are sane and adhere more to pcre standard 
behaviour. pcre documentation is impressive and with probably many guides 
floating around the internet, deviating from what the documentation says is 
potentially more annoying/frustrating to users.
  
  [..]
  
  > As you seems to have now played a bit with this part of the code, are you 
interested in test out the still not merged https://phabricator.kde.org/D19367 
change?
  
  I'll see what I can do.

REPOSITORY
  R39 KTextEditor

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

To: ahmadsamir, #ktexteditor, dhaumann, cullmann
Cc: kde-frameworks-devel, kwrite-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D23457: Port regex search to QRegularExpression

[Christoph Cullmann]

cullmann added a comment.


  In D23457#519217 , @ahmadsamir 
wrote:
  
  > Maybe they'll also see it as ktexteditor/kate using a regex engine that 
matches what the abundance of online pcre docs say, and how other editors that 
use pcre behave?
  >
  > IIUC, '\s' was workedaround so as not to match a newline so that the search 
pattern wouldn't be considered multiline (isMultiLine() function), which makes 
findAll and replaceAll slower as it took longer, v.s. just matching against 
each line separately.
  
  
  Actually, it is not faster.
  If you take a look at the code, for single line regex, it iterates over the 
individual lines.
  For multi line regexes, it will first concatinate all lines into one buffer.
  For large files that is "very" slow.
  And if you e.g. search + hit then "next match", this will be done again and 
again.
  
  But given it only happens more often for stuff containing \s, I assume that 
should be not that problematic, thought not sure if the behavior change is that 
good.
  
  > The thing is, what kateregexp did was replace '\s' with '[ \t]', which 
users who want this behaviour can easily use.
  
  That is true, perhaps we should add this as extra into the menu as proposal, 
like \s/...
  
  > Technically it's a whole new class, QRegularExpression, some different 
behaviours are sort of expected...
  
  ;=) That is really no good reasoning why one changes a behavior.
  It is clear that if you port something over to a new class, behavior might 
change, but that doesn't make it a good thing per default.
  
  On the other side, I see you did a lot of testing, that is highly appreciated.
  
  I will think a bit more about this patch.
  
  As you seems to have now played a bit with this part of the code, are you 
interested in test out the still not merged https://phabricator.kde.org/D19367 
change?

REPOSITORY
  R39 KTextEditor

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

To: ahmadsamir, #ktexteditor, dhaumann, cullmann
Cc: kde-frameworks-devel, kwrite-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D23457: Port regex search to QRegularExpression

[Ahmad Samir]

ahmadsamir added a comment.


  Maybe they'll also see it as ktexteditor/kate using a regex engine that 
matches what the abundance of online pcre docs say, and how other editors that 
use pcre behave?
  
  IIUC, '\s' was workedaround so as not to match a newline so that the search 
pattern wouldn't be considered multiline (isMultiLine() function), which makes 
findAll and replaceAll slower as it took longer, v.s. just matching against 
each line separately.
  
  The thing is, what kateregexp did was replace '\s' with '[ \t]', which users 
who want this behaviour can easily use.
  
  Technically it's a whole new class, QRegularExpression, some different 
behaviours are sort of expected...

REPOSITORY
  R39 KTextEditor

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

To: ahmadsamir, #ktexteditor, dhaumann, cullmann
Cc: kde-frameworks-devel, kwrite-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D23457: Port regex search to QRegularExpression

[Christoph Cullmann]

cullmann added a comment.


  In D23457#519206 , @ahmadsamir 
wrote:
  
  > In D23457#519205 , @cullmann 
wrote:
  >
  > > Hi, without any further look at the code changes, I don't think an 
behavior change like "\s can match a newline" is a good idea.
  > >  Or do I misunderstand that?
  >
  >
  > It's exactly what it says, \s can match a new line char in pcre.
  >
  > Why do you think this is a bad idea?
  
  
  Because before it didn't do that in KTextEditor, or?
  That means all people that got used to the current behavior will see this as 
an regression, if it is not optional.

REPOSITORY
  R39 KTextEditor

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

To: ahmadsamir, #ktexteditor, dhaumann, cullmann
Cc: kde-frameworks-devel, kwrite-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D23457: Port regex search to QRegularExpression

[Ahmad Samir]

ahmadsamir added a comment.


  In D23457#519205 , @cullmann wrote:
  
  > Hi, without any further look at the code changes, I don't think an behavior 
change like "\s can match a newline" is a good idea.
  >  Or do I misunderstand that?
  
  
  It's exactly what it says, \s can match a new line char in pcre.
  
  Why do you think this is a bad idea?

REPOSITORY
  R39 KTextEditor

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

To: ahmadsamir, #ktexteditor, dhaumann, cullmann
Cc: kde-frameworks-devel, kwrite-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D23457: Port regex search to QRegularExpression

[Christoph Cullmann]

cullmann added a comment.


  Hi, without any further look at the code changes, I don't think an behavior 
change like "\s can match a newline" is a good idea.
  Or do I misunderstand that?

REPOSITORY
  R39 KTextEditor

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

To: ahmadsamir, #ktexteditor, dhaumann, cullmann
Cc: kde-frameworks-devel, kwrite-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D23457: Port regex search to QRegularExpression

[Ahmad Samir]

ahmadsamir created this revision.
ahmadsamir added reviewers: KTextEditor, dhaumann, cullmann.
Herald added projects: Kate, Frameworks.
ahmadsamir requested review of this revision.

REVISION SUMMARY
  - Do away with the kateregexp class; move isMultiLine() to kateregexpsearch
  - \s can match a newline
  - Dot '.' will match any character except a newline by default; it can be set 
to match a newline if QRegularExpression::DotMatchesEverythingOption is set, 
right now it can be set implicitly as a match directive, '(?s)', in the search 
pattern
  - Explicitly enable QRegularExpression::MultilineOption, more details about 
that are in KateRegExpSearch::search()
  - Update the relevant unit tests (searchbar_test, regexpsearch_test)

TEST PLAN
  All unit tests pass except for vimode_emulatedcommandbar
  
  Search away, test it for as long as possible before committing

REPOSITORY
  R39 KTextEditor

BRANCH
  ahmad/qregularexpression (branched from master)

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

AFFECTED FILES
  autotests/src/regexpsearch_test.cpp
  autotests/src/regexpsearch_test.h
  src/CMakeLists.txt
  src/document/katedocument.cpp
  src/search/kateplaintextsearch.cpp
  src/search/kateregexp.cpp
  src/search/kateregexp.h
  src/search/kateregexpsearch.cpp
  src/search/kateregexpsearch.h
  src/search/katesearchbar.cpp
  src/search/katesearchbar.h
  src/utils/katesedcmd.cpp
  src/utils/katesedcmd.h

To: ahmadsamir, #ktexteditor, dhaumann, cullmann
Cc: kde-frameworks-devel, kwrite-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann