Created attachment 166701 [details] Writer file showing problem with replace function Version: 7.0.0.3 (x64) Build ID: 8061b3e9204bef6b321a21033174034a5e2ea88e CPU threads: 4; OS: Windows 10.0 Build 19041; UI render: Skia/Raster; VCL: win Locale: en-GB (en_GB); UI: en-GB Calc: threaded Open the attached file. 1. With the cursor at the start of the document, open the Find & Replace dialog. 2. Tick "Regular Expressions". Put "$ (quotation dollar) in "Find" and ” (right double quotation mark: Alt + 0148) in "Replace". Press "Find Next". 3. The quotation mark at the end of paragraph 1 is selected. Now press "Replace". EXPECTED RESULT: The quotation mark is replaced by a curly right quotation mark. ACTUAL RESULT: The selection moves to the next instance without replacing anything. AFAICS, this bug occurs when ANY (looked-for) character is followed by a footnote or endnote anchor at the end of a paragraph.
Thank you for reporting the bug. I can confirm that the bug is present in Version: 7.0.3.1 (x64) Build ID: d7547858d014d4cf69878db179d326fc3483e082 CPU threads: 4; OS: Windows 10.0 Build 18363; UI render: Skia/Raster; VCL: win Locale: en-US (en_US); UI: en-US Calc: CL As I see it appears with any other symbol in Replace field, not only right double quotation mark. Also if in Step 3 click Replace All instead of Replace it works right.
Bibisected with linux64-7.0 to https://git.libreoffice.org/core/commit/0cd3b7926cafc01d06b589124215e9cb7c148f19 tdf#65038: use trailing string characters for look-ahead assertions Adding Cc: to Mike Kaganski
This is fairly easily resolved with - if (pRegexMatcher && endPos < searchStr.getLength()) + if (pRegexMatcher && endPos < searchStr.getLength() && !aSrchPara.searchString.endsWith("$")) and similar for startsWith("^"). However, making a unit test will be the hardest part. Comment 1 indicates that a replaceAll works, so copying Mike's test code won't work. I'm also puzzled why it WOULD work in the first place.
proposed fix at http://gerrit.libreoffice.org/c/core/+/120410
Justin Luth committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/a511bffd67a9cebfdc878766581ac08c79d7ff51 tdf#137737 i18n search: don't expand start/end with regex ^ or $ It will be available in 7.3.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Justin Luth committed a patch related to this issue. It has been pushed to "libreoffice-7-2": https://git.libreoffice.org/core/commit/42a62ab46af52fae0592150ffe5e9ce8b39314b5 tdf#137737 i18n search: don't expand start/end with regex ^ or $ It will be available in 7.2.1. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Verified, thanks Arch Linux 64-bit Version: 7.3.0.0.alpha0+ / LibreOffice Community Build ID: c6883e7a031dec5fe3a365c4fd6adccff09696e5 CPU threads: 8; OS: Linux 5.13; UI render: default; VCL: kf5 (cairo+xcb) Locale: fi-FI (fi_FI.UTF-8); UI: en-US Calc: threaded Built on 25 August 2021
Version: 7.3.0.0.alpha0+ / LibreOffice Community Build ID: 008f28c9e01114e731f785e9e238236a1ed069d2 CPU threads: 4; OS: Linux 5.4; UI render: default; VCL: gtk3 Locale: en-GB (en_GB.UTF-8); UI: en-US TinderBox: Linux-rpm_deb-x86_64@86-TDF, Branch:master, Time: 2021-08-22_15:17:36 Calc: threaded I have repeated the test described in my opening post and the result is still the same;the selection moves to the next instance without replacing anything. So not fixed.
(In reply to R. Green from comment #8) > Version: 7.3.0.0.alpha0+ / LibreOffice Community > Build ID: 008f28c9e01114e731f785e9e238236a1ed069d2 > CPU threads: 4; OS: Linux 5.4; UI render: default; VCL: gtk3 > Locale: en-GB (en_GB.UTF-8); UI: en-US > TinderBox: Linux-rpm_deb-x86_64@86-TDF, Branch:master, Time: > 2021-08-22_15:17:36 > Calc: threaded > > I have repeated the test described in my opening post and the result is > still the same;the selection moves to the next instance without replacing > anything. So not fixed. That is because you are testing with a build from 22 Aug while the fix was committed today.
Ah. I missed that. I'll retest when the updated AppImage is released.
(In reply to Commit Notification from comment #5) This introduces a regression. 1. Create a lorem ipsum text. 2. Select any random part inside any paragraph. 3. Using F&R, use regex "^." (any character in the beginning of the paragraph), ad replace with "A" (character A). 4. Use "Current selection only" 5. Replace All. The expected result is that the text is not found (unless we also selected the very first character of the paragraph, in which case the very first character of the paragraph should be replaced). The actual result now is that all the selected text is replaced with AAAAA...
(In reply to Mike Kaganski from comment #11) > (In reply to Commit Notification from comment #5) > > This introduces a regression. Reported in bug so we don't mix bug ids, for future reference, etc...
Xisco Fauli committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/e73c3942de474004b04c1d538c89354183d44ae9 tdf#144089: Revert "tdf#137737 i18n search: don't expand start/end with regex ^ or $" It will be available in 7.3.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
(In reply to Mike Kaganski from comment #11) > 3. Using F&R, use regex "^." (any character in the beginning of the > paragraph), ad replace with "A" (character A). I don't think ^ specifically means "beginning of paragraph" in every context. I would expect it to be the beginning of the search string. So if only a selection of text was chosen, it would refer to the beginning of the selection (and if the selection contained multiple lines[aka paragraphs], it would also match the beginning of these other lines). But your regression steps make it plain there is more involved than just this.
(In reply to Justin L from comment #14) > I don't think ^ specifically means "beginning of paragraph" in every > context. I would expect it to be the beginning of the search string. So if > only a selection of text was chosen, it would refer to the beginning of the > selection (and if the selection contained multiple lines[aka paragraphs], it > would also match the beginning of these other lines). No. It must match start of the paragraph to behave according to least surprise principle. The caret is a special kind of a look-behind assertion, and as such, it needs to consider the data *to the left* of current position. Consider a look-behind condition like (?<=\s). It must only succeed if the previous character was a "space". Then in a text like lorem ipsum when you select only the second word "ipsum", and use "current selection only", you would have the "i" *matching* the regex. If you would not consider the text outside of the selection as the context, you would not be able to match the starting character, or you would need to select the character before this. (Yes, there are ways to create more sophisticated queries, but they would likely be less specific - e.g., using ^ as alternative in (?<=^|\s) would also match starts of paragraphs in selections spanning over paragraph boundaries, which is likely not what user wanted.) Regex engines also have this idea, expressed in e.g. icu::RegexMatcher::region [1] allowing to search inside a specific region, but considering the data outside of it as the context (configurable though). [1] https://unicode-org.github.io/icu-docs/apidoc/released/icu4c/classicu_1_1RegexMatcher.html#a556e40af5e725121ad23944b24e0fb1b
Xisco Fauli committed a patch related to this issue. It has been pushed to "libreoffice-7-2": https://git.libreoffice.org/core/commit/f7cf8f988c7c2856df5cb79fcbef22258cbce6db tdf#144089: Revert "tdf#137737 i18n search: don't expand start/end with regex ^ or $" It will be available in 7.2.1. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
DoSearch in sw/source/core/crsr/findtxt.cxx uses lcl_CleanStr, which does some utter magic with special characters in the text. Namely, in case of RES_TXTATR_FTN (among other things), it replaces it with '\x7f' in the middle of the text, and drops in the end. The input looks like "He heard quiet steps behind him. That didn't bode well. Who could be following\x01 him this late at night and in this deadbeat part of town."\x01 and the output looks like "He heard quiet steps behind him. That didn't bode well. Who could be following\x7f him this late at night and in this deadbeat part of town." That latter string arrives to the TextSearch::searchForward's searchStr; and in it, the "$ matches (because the footnote anchor is dropped). Note how you will be unable to find regex 'following ' (without quotes; i.e. the word "following" followed by a space) in this text - because there is a footnote anchor between the word and the space. Personally I would simply not strip the anchor from the string; but that needs a way to treat those meta-characters specially - having some syntax for them; another option would be to remove them *everywhere*, not only at the end (so that the 'following ' example would work). But I digress. OTOH, ReplaceBackReferences (again in sw/source/core/crsr/findtxt.cxx) does not pre-process the string obtained from the paragraph using that lcl_CleanStr, and passes the original string to TextSearch::searchForward as is. Hence the difference between the results of find and replace. The replace previously relied on the "find" step limiting the endPos, and so the truncation happened inside TextSearch::searchForward using that position; and now the function sees the actual anchor following the position, and the regex does not match. Trying to find a way to "synchronize" this - likely the "natural" option is to make ReplaceBackReferences use lcl_CleanStr.
(In reply to Mike Kaganski from comment #17) It's even more complicated. After doing the ReplaceBackReferences, SwView::ExecSearch also does SwView::Replace, which again calls DoSearch (indirectly), but now passes nStart = nEnd = 139. In this case, lcl_CleanStr in it does not replace anything (because the anchors are outside of the start-end range). Trying to outsmart ourselves. Fun.
FTR: although complicated, the regex "(?=[\p{Control}]?$) works as expected (of course, this does *not* mean that this is not a bug, because it only works because at "search" time it finds the quote in the end of the search string, and at replace time, it finds this quote followed by the control character at the end of the search string).
My proposal now is: 1. Fix this by excluding special processing of footnote/endnote anchor. This will change the behavior: the footnote anchor will be considered a "character" (non-word, so e.g. regex g\b will find "following" in the first body paragraph). Note: "$ will *not* find the first body paragraph's last " anymore, while "[:control:]$ will (but of course, one would need to use captures or look-ahead assertions to not replace the footnote/endnote character itself, also removing the note itself). 2. In the long run, see how to process the rest of the special characters consistently. Namely, they are RES_TXTATR_FLYCNT (fly anchor), RES_TXTATR_FIELD (field), RES_TXTATR_REFMARK (reference), RES_TXTATR_TOXMARK (index entry), RES_TXTATR_META and RES_TXTATR_METAFIELD (no idea when these are used). The problem here is that each of them will create the same problem; and for those that conceptually can't be considered "control characters", we need to drop them from the string at *all* times, not only when they are at start/end of string, and also when they are outside of the nStart/rEnd boundaries. Please reply what you think specifically about #1 here. Note again, that this would fix it in a very different way compared to what OP wanted.
(In reply to Mike Kaganski from comment #20) > Please reply what you think specifically about #1 here. Note again, that > this would fix it in a very different way compared to what OP wanted. This is difficult because we have two very real situations here. The most common one would be OPs - where LO implementation specifics are not cared about, and the regex is only concerned about the "text". So I think this is the one that needs the most focus, even though regex is a "pro user" feature. The second would be someone who is actually searching for footnote markers etc. But that "developer class" level of user will be very rare. I personally expect that find/replace would ignore all of these internal things - and that is obviously how previous implementers also viewed it. (And they also acknowledged it is basically impossible to avoid mid-paragraph.)
(In reply to Justin L from comment #21) > This is difficult because we have two very real situations here. The most > common one would be OPs - where LO implementation specifics are not cared > about, and the regex is only concerned about the "text". So I think this is > the one that needs the most focus, even though regex is a "pro user" feature. > > The second would be someone who is actually searching for footnote markers > etc. But that "developer class" level of user will be very rare. Thank you! Let me mention tdf#70142 for some context here. People *in general* expect that what they *see* on screen is searchable (which includes fields, numbering, etc.); and in this regard, footnote anchor at lease should *participate* in search (even if its character is not (yet) searchable). OTOH, making dynamic text inside special objects searchable has some drawback: e.g., replacing would be impossible when find-and-replace searches inside fields (how would it replace inside or across boundaries of a field?). So conceptually, there might be two modes for find: "normal" search, and "search inside fields" (the latter one disabling replace). But anyway, recognizing special objects as parts of text (even if just as "control characters" in normal mode) would change workflow, but not disallow things (possibly just require somewhat more complex expressions - consider comment 19). I suppose that separating "normal power users" from "advanced power users" when talking about regexes is too much, and IMO just mentioning in the release notes (and related help page) that now the selected special objects (footnote anchors) match [\p{Control}] would be enough - this adds flexibility. OTOH, things like floating object anchors or reference or index entry are completely "metadata". We need to modify the code to remove those objects from the search string completely (which should be possible IMO, just needs more work; and doing that would enable tdf#70142, because that way, we would be able to replace the special characters *universally* including in the middle of the string).
https://gerrit.libreoffice.org/c/core/+/121134 is the proposed WIP change implementing #1 from comment 20. I do not intend to push it right now, before UX could discuss this - only for testing.
(In reply to Mike Kaganski from comment #23) > https://gerrit.libreoffice.org/c/core/+/121134 is the proposed WIP change > implementing #1 from comment 20. I do not intend to push it right now, > before UX could discuss this - only for testing. UX-team, Mike needs your opinion here
No objection within a week, I guess that's a silent agreement. Like Justin I also think there are not many users who complain about missing anchors in a regex search.
So I believe that I have exercised all due precautions (I nod to bug 142214 comment 47 ;D - and I did all the following even before that controversy). I had set needsUXEval and asked additional review at 2021-08-27; I had described the background and implications in comment 20 and comment 22; I waited for more opinions even after comment 25 for two weeks more. I thank Justin for providing the feedback. Providing a configuration option here is *not* an option for now, at least until there is a proper "remove all control characters everywhere" change, as explained in the last paragraph of comment 22. For now, the current behavior is broken. I am going to push the patch now.
Mike Kaganski committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/657ec7ff6863ebc79dcea228898b619d43543a6c tdf#137737: treat foot/endnote anchor as a "normal" (control) character It will be available in 7.3.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
I can confirm it is fixed in: Version: 7.3.0.1 / LibreOffice Community Build ID: 840fe2f57ae5ad80d62bfa6e25550cb10ddabd1d CPU threads: 4; OS: Linux 5.4; UI render: default; VCL: gtk3 Locale: en-AU (en_AU.UTF-8); UI: en-US Calc: threaded As in, the original search described will only search (and replace) the double quotes that don't have a footnote/endnote right behind them.
Xisco Fauli committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/00315ee52b09cb64de268f762809c2418716f49e tdf#137737: sw: Add UItest It will be available in 7.4.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.