Opened 12 years ago

Closed 12 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:
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 12 years ago.
ReplaceAll2.patch (2.3 KB ) - added by x-ist 12 years ago.
Added a final ScrollToSelection()
ReplaceAll3.patch (3.7 KB ) - added by x-ist 12 years ago.
Now hopefully correctly generated patch.

Download all attachments as: .zip

Change History (12)

comment:1 by x-ist, 12 years ago

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 ;)

by x-ist, 12 years ago

Attachment: ReplaceAll.patch added

comment:2 by x-ist, 12 years ago

patch: 01

comment:3 by x-ist, 12 years ago

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 by humdinger, 12 years ago

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.

by x-ist, 12 years ago

Attachment: ReplaceAll2.patch added

Added a final ScrollToSelection()

in reply to:  4 ; comment:5 by x-ist, 12 years ago

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 12 years ago by x-ist (previous) (diff)

in reply to:  5 comment:6 by humdinger, 12 years ago

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.

by x-ist, 12 years ago

Attachment: ReplaceAll3.patch added

Now hopefully correctly generated patch.

comment:7 by humdinger, 12 years ago

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 by x-ist, 12 years ago

Sorry, will do.

comment:9 by humdinger, 12 years ago

Resolution: fixed
Status: newclosed

Closing as fixed with hrev44406. Thanks!

Note: See TracTickets for help on using tickets.