LightTable / LightTable

The Light Table IDE ⛺
www.lighttable.com
MIT License
11.72k stars 915 forks source link

Search "/" and replace #1938

Open msadakov opened 9 years ago

msadakov commented 9 years ago

The search can not handle at the beginning and at the end with a "/", and then replaced.

Error:

Invalid behavior: :lt.objs.find/replace!
ReferenceError: query is not defined
    at eval (/home/max/LightTable/builds/lighttable-0.8.0-linux/resources/app/core/node_modules/codemirror_addons/search.js:126:71)
    at runInOp (/home/max/LightTable/builds/lighttable-0.8.0-linux/resources/app/core/node_modules/codemirror/lib/codemirror.js:2138:18)
    at CodeMirror.operation (/home/max/LightTable/builds/lighttable-0.8.0-linux/resources/app/core/node_modules/codemirror/lib/codemirror.js:4425:35)
    at Object.replace (/home/max/LightTable/builds/lighttable-0.8.0-linux/resources/app/core/node_modules/codemirror_addons/search.js:122:10)
    at Function.lt.objs.find.__BEH__replace_BANG_ (file:///home/max/LightTable/builds/lighttable-0.8.0-linux/resources/app/core/node_modules/lighttable/bootstrap.js:32498:23)
    at c (file:///home/max/LightTable/builds/lighttable-0.8.0-linux/resources/app/core/node_modules/lighttable/bootstrap.js:6196:14)
    at a (file:///home/max/LightTable/builds/lighttable-0.8.0-linux/resources/app/core/node_modules/lighttable/bootstrap.js:6236:18)
    at c (file:///home/max/LightTable/builds/lighttable-0.8.0-linux/resources/app/core/node_modules/lighttable/bootstrap.js:17774:76)
    at a (file:///home/max/LightTable/builds/lighttable-0.8.0-linux/resources/app/core/node_modules/lighttable/bootstrap.js:17808:18)
    at a (file:///home/max/LightTable/builds/lighttable-0.8.0-linux/resources/app/core/node_modules/lighttable/bootstrap.js:17819:34)

Without the "/" all right

kenny-evitt commented 9 years ago

@devmru Would you comment with the exact search and replace text you entered? It's not entirely clear from you original comment.

msadakov commented 9 years ago

Procedure: Ctrl+F -> input "find" set value "/var/www/" -> input "replace" set value "/home/max/www/" -> button "all" click -> Error

kenny-evitt commented 9 years ago

@devmru thanks

rundis commented 9 years ago

Seems to be an issue with the Codemirror search/find plugin. Can reproduce the problem here: https://codemirror.net/demo/search.html

rundis commented 9 years ago

Since it's a regex behind the scenes you'd have to write something like: Find: /\/var\/www\// Replace: /\/home\/max\/www\//

msadakov commented 9 years ago

I did some tests. Problem "/" early search

msadakov commented 9 years ago

Ctrl+F -> input "find" set value "/var/www/" -> input "replace" set value "/home/max/www/" -> button "all" click -> Error Ctrl+F -> input "find" set value "var/www/" -> input "replace" set value "/home/max/www/" -> button "all" click -> No error

kenny-evitt commented 9 years ago

@devmru @rundis This seems to be the problematic code:

  function isRegex(s) {
    if(s instanceof RegExp) return true;
    return s.match(/^\/(.+)\/([a-z]*)$/);
  }

It'd be nice to continue supporting regexes so we need some way to distinguish between regular strings and regexes. Unless we treat everything like a regex. Maybe we could just treat any input as /input/g? Or maybe we could tweak the regex-regex in the isRegex function to not match /var/www/, if that's even possible (or desirable).

As-is, /var/www/ seems like the perfect input to break something that's trying to support both plain text and regex expressions.

danmbox commented 9 years ago

no checkbox to select regex / non-regex search, nice...

kenny-evitt commented 9 years ago

@danmbox Pull requests welcome.