D27097: Port from QRegExp to QRegularExpression

[David Faure]

dfaure added a comment.


  Deprecate the attribute, keep the code. There is more KF5-based code out 
there than the one visible by lxr.kde.org. Some of it not even public. We make 
the same promise as Qt, preserving SC and BC in major versions.
  
  OK for "Invalid PCRE pattern syntax"

REPOSITORY
  R310 KTextWidgets

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

To: ahmadsamir, #frameworks, mlaurent, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27097: Port from QRegExp to QRegularExpression

[Ahmad Samir]

ahmadsamir added a comment.


  Looking at the incremental stuff some more, there are no public methods that 
mention incremental; the only way is to set the KFind::FindIncremental flag; 
the only method I could find is in the private API (find_p.h) 
startNewIncrementalSearch(). So technically it's all hidden behind the 
d-pointer, there's nothing to deprecate about incremental search, except remove 
the flag/enum FindIncremental... (does that mean we can remove the incremental 
code now, or wait till the KF6 branching?).
  
  FWIW, I checked lxr again and KFind::FindIncremental exists only in kmplayer 
and khtml, and in both cases it's in commented out code.

REPOSITORY
  R310 KTextWidgets

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

To: ahmadsamir, #frameworks, mlaurent, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27097: Port from QRegExp to QRegularExpression

[Ahmad Samir]

ahmadsamir marked an inline comment as done.
ahmadsamir added a comment.


  In D27097#615648 , @dfaure wrote:
  
  > OK for deprecating Incremental. A full revert (at KF6 time) of commit 
9ac82e27ad0322e444c16 
 
looks tricky though :-)
  >  I searched the git history, and I can't see where we ever used 
FindIncremental... [I searched kdelibs, since I'm pretty sure this was added 
for KHTML]
  
  
  It's got to be removed at KF6; since it's not actually used, it'll make the 
code simpler, I think (partial/incremental matching is usually a complex pain).
  
  > You say that nothing calls find(QRegExp), that's because the find dialog 
just sets the option RegularExpression, right?
  >  This makes sense actually. I don't see why we have this API. The user uses 
the dialog and checks a box...
  
  Yeah, I meant explicitly call Kfind::find() that takes a QRegExp.
  
  > It sounds like we should just deprecate find(QRegExp) and point to the 
RegularExpression option.
  >  (and make it private, as you said, i.e. move to private class)
  
  OK, will do.

INLINE COMMENTS

> dfaure wrote in kfinddialog.cpp:567
> I doubt end users want to open Qt API documentation...

One alternative is https://perldoc.perl.org/perlre.html :)

I could opt for:
"Invalid PCRE (Perl-compatible regular expressions) pattern syntax" OR
"Invalid PCRE pattern syntax"

then one online search later, the user finds what PCRE is.

REPOSITORY
  R310 KTextWidgets

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

To: ahmadsamir, #frameworks, mlaurent, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27097: Port from QRegExp to QRegularExpression

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  OK for deprecating Incremental. A full revert (at KF6 time) of commit 
9ac82e27ad0322e444c16 
 
looks tricky though :-)
  I searched the git history, and I can't see where we ever used 
FindIncremental... [I searched kdelibs, since I'm pretty sure this was added 
for KHTML]
  
  You say that nothing calls find(QRegExp), that's because the find dialog just 
sets the option RegularExpression, right?
  This makes sense actually. I don't see why we have this API. The user uses 
the dialog and checks a box...
  It sounds like we should just deprecate find(QRegExp) and point to the 
RegularExpression option.
  (and make it private, as you said, i.e. move to private class)

INLINE COMMENTS

> kfind.cpp:775
>  d->pattern = pattern;
> -setOptions(options());   // rebuild d->regExp if necessary
> +// rebuild d->regex and d->regeExp (the latter is deprecated) if 
> necessary
> +setOptions(options());

typo: additional 'e' in regeExp

> kfinddialog.cpp:567
> +if (!QRegularExpression(q->pattern()).isValid()) {
> +KMessageBox::error(q, i18n("Invalid regular expression (see the 
> QRegularExpression documentation)."));
>  return;

I doubt end users want to open Qt API documentation...

REPOSITORY
  R310 KTextWidgets

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

To: ahmadsamir, #frameworks, mlaurent, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27097: Port from QRegExp to QRegularExpression

[Ahmad Samir]

ahmadsamir updated this revision to Diff 74871.
ahmadsamir edited the test plan for this revision.
ahmadsamir added a comment.


  Add test plan section

REPOSITORY
  R310 KTextWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27097?vs=74850=74871

BRANCH
  l-qregularexpression (branched from master)

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

AFFECTED FILES
  autotests/kfindtest.cpp
  autotests/kfindtest.h
  autotests/krichtextedittest.cpp
  src/CMakeLists.txt
  src/findreplace/kfind.cpp
  src/findreplace/kfind.h
  src/findreplace/kfind_p.h
  src/findreplace/kfinddialog.cpp
  src/findreplace/kreplace.cpp
  src/findreplace/kreplace.h
  src/widgets/krichtextedit.cpp

To: ahmadsamir, #frameworks, mlaurent, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D27097: Port from QRegExp to QRegularExpression

[Ahmad Samir]

ahmadsamir added a comment.


  A couple of notes:
  
  - "Incremental" search isn't used by anything in KDE, AFAICS from 
lxr.kde.org, remove it (in a separate diff)?
  - Nothing in KDE uses the static KFind::find(... QRegExp..) directly; should 
we make the new one that takes a QRegularExpression private? the same question 
applies to KReplace::replce(... QRegExp).

REPOSITORY
  R310 KTextWidgets

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

To: ahmadsamir, #frameworks, mlaurent, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D27097: Port from QRegExp to QRegularExpression

[Ahmad Samir]

ahmadsamir created this revision.
ahmadsamir added reviewers: Frameworks, mlaurent, dfaure.
Herald added a project: Frameworks.
ahmadsamir requested review of this revision.

REVISION SUMMARY
  Port QRegExp::setMinimal() by making the regex pattern non-greedy, where
  possible.
  
  Deprecate the KFind::find() and KReplace::replace() methods that take a
  QRegExp.

REPOSITORY
  R310 KTextWidgets

BRANCH
  l-qregularexpression (branched from master)

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

AFFECTED FILES
  autotests/kfindtest.cpp
  autotests/kfindtest.h
  autotests/krichtextedittest.cpp
  src/CMakeLists.txt
  src/findreplace/kfind.cpp
  src/findreplace/kfind.h
  src/findreplace/kfind_p.h
  src/findreplace/kfinddialog.cpp
  src/findreplace/kreplace.cpp
  src/findreplace/kreplace.h
  src/widgets/krichtextedit.cpp

To: ahmadsamir, #frameworks, mlaurent, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns