#214 feature/implement replace js function #351

0styx0 · 2023-08-06T02:15:08Z

Summary

Feature implemented: Users can replace using JS functions. The provided input will run as the return statement to string.replace and can use the same variables mentioned in MDN

Example:

Replace: dino(saur)
- Input type: Regular Expression
With: "great big " + match + " biga" + p1.toUpperCase()
- Output type: Function
Result: great big dinosaur bigaSAUR

Screenshot

UI changes:

Added a warning about executing arbitrary JS in the options page
Added an "Output Type" select in the sidebar and in the options page featuring options "text" and "function"

This PR addresses issue #214

…from the options page

…arbitrary JS execution

…tyx0/foxreplace into feature/implement_replace_js_function

…e STRINGS

Woundorf

Hi, thanks for the pull request and sorry for not responding before.

First of all, congratulation because it is a great work and you clearly know JavaScript better than me. And also thanks for translating the new strings to Catalan. I suppose you have used automatic translation but it is still fairly good.

I have added some comments with things that should be improved and other things.

Woundorf · 2023-08-18T16:00:42Z

options/gridcomponents.js

-                  InputTypeEditor.getOption(params, 1, browser.i18n.getMessage("inputType.wholeWords")) +
-                  InputTypeEditor.getOption(params, 2, browser.i18n.getMessage("inputType.regExp")) +
+                  this.options.reduce((accum, curOption, i) =>
+                    DropdownEditor.getOption(params, i, browser.i18n.getMessage(curOption)) + accum


The dropdown elements appear in reverse order from specified in code. The concatenation should be accum + DropdownEditor.getOption(...) instead.

Woundorf · 2023-08-18T16:09:23Z

options/options.html

@@ -218,6 +218,7 @@ <h4 class="i18n">options.otherSettings</h4>
                </div>
              </div>
              <div id="substitutionsContent" class="tab-pane" role="tabpanel">
+                <p class="i18n">options.output.function.warning</p>


Maybe a graphical warning sign would be more outstanding. For example:
 options.output.function.warning

Woundorf · 2023-08-18T16:12:31Z

scripts/core.js

+      this.caseSensitive = !!caseSensitive;
+      this.outputType = +outputType;
+      this.inputType = +inputType;
+
      if (this.inputType < this.INPUT_TEXT || this.inputType > this.INPUT_REG_EXP) this.inputType = this.INPUT_TEXT;  // avoid invalid values


Maybe you could add another line like this for the output type, to ensure it is a valid value.

Woundorf · 2023-08-18T16:27:18Z

scripts/core.js


        case this.INPUT_WHOLE_WORDS:
          // necessary according to https://stackoverflow.com/q/1520800
          this.regExp.lastIndex = 0;
          return string.replace(this.regExp, (word, index, string) => {
+            // todo by someone who understands: clarify what the following block does and why it does it


I don't remember the details because I wrote it many years ago, but it has to do with correct functioning of replace whole words with non-ASCII characters, including respecting the special strings $$, $&, etc. It was added in df6fe58.

Woundorf · 2023-08-18T16:32:06Z

scripts/core.js

+
+    }
+
+    replaceWithFn(userReplaceInput, curStrToReplace) {


Please add some documentation to this method, like in the others. Maybe some of the comments below could be moved to this method documentation.

Woundorf · 2023-08-18T16:36:21Z

scripts/core.js

-    let inputType = this.prototype.INPUT_TYPE_STRINGS.indexOf(json.inputType);
-    return new Substitution(json.input, json.output, json.caseSensitive, inputType);
+    const inputType = this.prototype.INPUT_TYPE_STRINGS.indexOf(json.inputType);
+    const outputType = this.prototype.OUTPUT_TYPE_STRINGS.indexOf(json.outputType);


Here you should check if the JSON really has an outputType, otherwise it fails. Look how the mode is filled in SubstitutionGroup.fromJSON for reference.

Woundorf · 2023-08-18T16:40:30Z

scripts/core.js

You have to update the version in substitutionListToJSON. Use version 2.6, which will be the version of FoxReplace when this is released. Also, in checkVersion set 2.6 as the current version and add 2.1 to the list of old versions.

Woundorf · 2023-08-18T16:43:50Z

scripts/replace.js

@@ -47,6 +47,7 @@ function replaceWindow(aWindow, aSubstitutionList, aPrefs) {
      case group.HTML_NONE: replaceText(doc, consecutiveGroups, aPrefs); break;
      case group.HTML_OUTPUT: replaceTextWithHtml(doc, consecutiveGroups, aPrefs); break;
      case group.HTML_INPUT_OUTPUT: replaceHtml(doc, consecutiveGroups, aPrefs); break;
+      default: console.error('Fox Replace: Invalid output type');


I don't think this is necessary, but in any case it is FoxReplace, without space, and it would be the HTML mode that is wrong, not the output type. But since there are no other checks like this in this file, I would remove the line entirely.

Woundorf · 2023-08-18T16:45:30Z

sidebar/sidebar.html

@@ -24,7 +24,7 @@
    <link rel="stylesheet" href="../lib/bootstrap.css">
    <script src="../lib/xregexp-all.js"></script>
    <script src="../scripts/core.js"></script>
-    <script src="sidebar.js"></script>
+    <script defer src="./sidebar.js"></script>


What is the purpose of this defer?

To ensure sidebar.js only loads once sidebar.html fully loads. If sidebar.js runs before sidebar.html fully renders, dom selectors will fail

Woundorf · 2023-08-18T16:47:07Z

sidebar/sidebar.html

+          <option value="0" selected="" class="i18n">outputType.text</option>
+          <option value="1" class="i18n">outputType.function</option>
+        </select>
+      </div>


You could add the warning also here, below the output type, preferably shown only if "function" is selected.

icon to preferences page)

…outputType undefined

0styx0 · 2023-08-21T18:30:48Z

@Woundorf thanks for your feedback! addressed all comments

Woundorf

Only one issue with the DropdownEditor. Everything else is fine.

Woundorf · 2023-08-21T19:00:54Z

options/gridcomponents.js

@@ -139,7 +146,7 @@ class DropdownEditor {
  init(params) {
    this.gui = $('<select>' +
                  this.options.reduce((accum, curOption, i) =>
-                    DropdownEditor.getOption(params, i, browser.i18n.getMessage(curOption)) + accum
+                    accum + DropdownEditor.getOption(params, i, browser.i18n.getMessage(curOption)) + accum


You have added the accum to the beginning but not removed it from the end. Now some items are repeated multiple times.

ugh good catch. hit c-z too many times. fixed

Styx Meiseles added 10 commits August 5, 2023 15:33

issue-214 feat: can replace regex with JS function

468c576

issue-214 feat: can replace whole words with function

b30e122

issue-214 feat: can replace text with function

6564ad2

issue-214 fix: fix bug I created that disabled replace functionality …

1aa89b6

…from the options page

issue-214 feat: add warning about replace with function allowing for …

3fb4449

…arbitrary JS execution

issue-214 feat: add warning about replace with function allowing for …

53e7b32

…arbitrary JS execution

Merge branch 'feature/implement_replace_js_function' of github.com:0s…

616519a

…tyx0/foxreplace into feature/implement_replace_js_function

issue-214 fix: make output type text display by default

2ab48d2

issue-214 fix: all value: number MUST correspond to the indices of th…

ca25e61

…e STRINGS

issue-214 fix: default to replace by text

48f95bb

Woundorf requested changes Aug 18, 2023

View reviewed changes

Styx Meiseles added 9 commits August 21, 2023 12:50

issue-214 fix: un-reverse dropdown items in list

69c146b

issue-214 feat: display sidebar warning if outputType=function (and add

a768ce9

icon to preferences page)

issue-214 feat: ensure outputType exists within enum

76c1aec

issue-214 docs: update doc for whole-world replace escaping

beb3c86

issue-214 docs: add doc-comment for replaceWithFn

e25106f

issue-214 fix: ensure fromJSON works correctly if deserializing with …

74f6212

…outputType undefined

issue-214 chore: update json version

47181ce

issue-214 chore: remove warning if no input type specified

b82dd7d

issue-214 docs: add doc-comment for DropdownEditor class

06c40a3

Woundorf requested changes Aug 21, 2023

View reviewed changes

issue-214 fix: remove accum from the end

1193d72

Woundorf merged commit 0a5ed28 into Woundorf:master Aug 23, 2023

Woundorf mentioned this pull request Aug 23, 2023

enable regex output to be string or function #214

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#214 feature/implement replace js function #351

#214 feature/implement replace js function #351

0styx0 commented Aug 6, 2023

Woundorf left a comment

Woundorf Aug 18, 2023

Woundorf Aug 18, 2023

Woundorf Aug 18, 2023

Woundorf Aug 18, 2023

Woundorf Aug 18, 2023

Woundorf Aug 18, 2023

Woundorf Aug 18, 2023

Woundorf Aug 18, 2023

Woundorf Aug 18, 2023

0styx0 Aug 21, 2023

Woundorf Aug 18, 2023

0styx0 commented Aug 21, 2023

Woundorf left a comment

Woundorf Aug 21, 2023

0styx0 Aug 21, 2023

#214 feature/implement replace js function #351

#214 feature/implement replace js function #351

Conversation

0styx0 commented Aug 6, 2023

Summary

Example:

UI changes:

Woundorf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0styx0 commented Aug 21, 2023

Woundorf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment