Opened 8 years ago

Closed 7 years ago

#8141 closed bug (fixed)

Recursive replacement is recursively replacing recursively...

Reported by: humdinger Owned by: korli
Priority: normal Milestone: R1
Component: Applications/StyledEdit Version: R1/Development
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

This is hrev43238.

  • Open StyledEdit, and paste the text:
    "As we enjoy great advantages from the inventions of others, we should be
    glad of an opportunity to serve others by any invention of ours; and this we
    should do freely and generously."
    
  • ALT+A and ALT+C to copy all
  • ALT+R to replace:
    "As we enjoy great advantages from the inventions of others, we should be
    glad of an opportunity to serve others by any invention of ours; and this we
    should do freely and generously."
    

with

As Ben Franklin put it:
"As we enjoy great advantages from the inventions of others, we should be
glad of an opportunity to serve others by any invention of ours; and this we
should do freely and generously."
  • Click "Replace all" and watch StyledEdit sink into a recursive loop.

Attachments (3)

ReplaceAll.patch (2.2 KB) - added by x-ist 7 years ago.
ReplaceAll2.patch (2.3 KB) - added by x-ist 7 years ago.
Added a final ScrollToSelection()
ReplaceAll3.patch (3.7 KB) - added by x-ist 7 years ago.
Now hopefully correctly generated patch.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 7 years ago by x-ist

Reproduced it by using text "A B" and replacing "B" by "A B".

Actually, the problem here is not recursive replacement, but a repetitive one, since the search is performed using the wrap=true parameter, so that after reaching the end of the text it simply starts over at the beginning and finds the previous replacements again.

Tomorrow there will be patch, I hope ;)

Changed 7 years ago by x-ist

Attachment: ReplaceAll.patch added

comment:2 Changed 7 years ago by x-ist

Has a Patch: set

comment:3 Changed 7 years ago by x-ist

The patch avoids wrapping-around when replacing, it just runs from the very beginning to the end once. Furthermore I thought it'd be better to not scroll during entire document when performing replace all.

comment:4 Changed 7 years ago by humdinger

Your patch fixes the problem, thanks!
I'm not sure though why the caret is placed after the last occurence that's been replaced, but the view isn't scrolled to it. I'd say, either you scroll to the caret so the user sees where he's going to type, or don't scroll and leave the caret where it was before the replace action. I think I'd prefer the latter...

Oh, and next time, please use "git format-patch" in order to be easier credited.

Changed 7 years ago by x-ist

Attachment: ReplaceAll2.patch added

Added a final ScrollToSelection()

comment:5 in reply to:  4 ; Changed 7 years ago by x-ist

Replying to humdinger:

I'm not sure though why the caret is placed after the last occurence that's been replaced, but the view isn't scrolled to it. I'd say, either you scroll to the caret so the user sees where he's going to type, or don't scroll and leave the caret where it was before the replace action. I think I'd prefer the latter...

Good point! Added a final ScrollToSelection() after all replacements are done. Intermediate scrolling is still ommited though. I think it makes more sense to scroll to the last replacement because the original caret position is not necessarily preserved, for instance when the replacement is an empty string. Then your final caret position is hard to predict.

Oh, and next time, please use "git format-patch" in order to be easier credited.

I did

git format-patch origin
git diff src/apps/stylededit/ > ReplaceAll2.patch

but it didn't affect the generated patch.

Last edited 7 years ago by x-ist (previous) (diff)

comment:6 in reply to:  5 Changed 7 years ago by humdinger

Replying to x-ist:

Added a final ScrollToSelection() after all replacements are done. Intermediate scrolling is still ommited though. I think it makes more sense to scroll to the last replacement because the original caret position is not necessarily preserved, for instance when the replacement is an empty string. Then your final caret position is hard to predict.

Hmm... isn't as trivial as I first thought then. I guess one would have to save the original caret position and then account for the number of characters that were removed/added by the replacement action. I may file another enhancement ticket for that...

Oh, and next time, please use "git format-patch" in order to be easier credited.

I did

git format-patch origin
git diff src/apps/stylededit/ > ReplaceAll2.patch

but it didn't affect the generated patch.

You'll have to locally commit your changes first. Then "git format-patch origin" will generate the patch. No need to "git diff" . As usual it's most convenient to work in your own branch, never on master.

Changed 7 years ago by x-ist

Attachment: ReplaceAll3.patch added

Now hopefully correctly generated patch.

comment:7 Changed 7 years ago by humdinger

I don't want to make you churn out patch after patch, so this will do :)
For the future, you should configure git to provide your name and email address. See the FAQ.

comment:8 Changed 7 years ago by x-ist

Sorry, will do.

comment:9 Changed 7 years ago by humdinger

Resolution: fixed
Status: newclosed

Closing as fixed with hrev44406. Thanks!

Note: See TracTickets for help on using tickets.