[CLOSED] REVIEW ONLY: Fix regexp replace in Replace All

[core-ai-bot]

Issue by njx
Wednesday Oct 30, 2013 at 19:47 GMT
Originally opened as adobe/brackets#5772

---

For #5770.

When doing a replace all, the replace function used by regexp replace needs to use the matches from each item in the
replace all, not the match that was found in the calling replace().

---

njx included the following code: https://github.com/adobe/brackets/pull/5772/commits

[core-ai-bot]

Comment by MarcelGerber
Wednesday Oct 30, 2013 at 20:09 GMT

---

When there are only 3 params left, we should call the function with only 3 params, too.
If you'd fix that, I'd prefer your PR.

[core-ai-bot]

Comment by gruehle
Wednesday Oct 30, 2013 at 20:23 GMT

---

Everything looks good to me. I did a bunch of testing with 'Replace All', and everything worked as expected.

[core-ai-bot]

Comment by peterflynn
Wednesday Oct 30, 2013 at 20:34 GMT

---

@njx Fix looks good to me too. It doesn't look like there's any way it could affect the existing non-regexp Replace All functionality.

OTOH I think it'd also be ok to just change the release notes to say $n substitution is only supported for regular Replace.

[core-ai-bot]

Comment by njx
Wednesday Oct 30, 2013 at 20:37 GMT

---

@SAPlayer - not sure what you mean. I removed the old replace function from the call to _showReplaceAllPanel().

[core-ai-bot]

Comment by peterflynn
Wednesday Oct 30, 2013 at 20:39 GMT

---

Btw -- not related to this bug, but... if the user wanted the literal string "$1" to be the replacement when doing a regexp Replace, is there any escaping that would allow that? It looks like you'd normally be able to use "$$" to escape "$," but we don't appear to support that.

[core-ai-bot]

Comment by MarcelGerber
Wednesday Oct 30, 2013 at 20:46 GMT

---

@njx I mean, when this function is called, we should only pass 3 params instead of 4.

[core-ai-bot]

Comment by njx
Wednesday Oct 30, 2013 at 21:05 GMT

---

@SAPlayer Can you comment on the specific line you're referring to? The only call to that function is 3 params: _showReplaceAllPanel(editor, query, text); on line 555.

[core-ai-bot]

Comment by njx
Wednesday Oct 30, 2013 at 21:05 GMT

---

@peterflynn Good point, should file that separately.

[core-ai-bot]

Comment by MarcelGerber
Wednesday Oct 30, 2013 at 21:20 GMT

---

@njx Oh, sorry, I've missed that. Everything's ok ;)

[core-ai-bot]

Comment by njx
Friday Nov 08, 2013 at 19:04 GMT

---

@peterflynn -@SAPlayer added code to handle $$; if you have a chance, feel free to try it and see if you find any issues.