search

ajaxorg / ace

Ace (Ajax.org Cloud9 Editor)
https://ace.c9.io
Other
26.76k stars 5.29k forks source link

Various modes assume `state` must be a string #2333

Closed kevinushey closed 2 years ago

kevinushey commented 9 years ago

Using ag to check for state == in the lib/ace/mode/ folder,

lib/ace/mode/asciidoc.js
49:        if (state == "listblock") {

lib/ace/mode/c_cpp.js
68:        if (state == "start") {
73:        } else if (state == "doc-start") {
74:            if (endState == "start") {

lib/ace/mode/coffee.js
83:            state === 'start' && indenter.test(line))

lib/ace/mode/csharp.js
34:        if (state == "start") {

lib/ace/mode/dot.js
33:        if (state == "start") {

lib/ace/mode/folding/asciidoc.js
116:            if (state == "dissallowDelimitedBlock") {

lib/ace/mode/gherkin.js
64:        if (state == "start") {

lib/ace/mode/golang.js
33:        if (state == "start") {

lib/ace/mode/haxe.js
34:        if (state == "start") {

lib/ace/mode/jack.js
56:        if (state == "start") {

lib/ace/mode/javascript.js
68:        if (state == "start" || state == "no_regex") {
73:        } else if (state == "doc-start") {
74:            if (endState == "start" || endState == "no_regex") {

lib/ace/mode/json.js
55:        if (state == "start") {

lib/ace/mode/jsx.js
34:        if (state == "start") {

lib/ace/mode/liquid.js
60:        if (state == "start") {

lib/ace/mode/livescript.js
20:        if (state === 'start' && indenter.test(line)) {

lib/ace/mode/lsl.js
70:        if (state === "start") {

lib/ace/mode/lua.js
104:        if (state == "start") {

lib/ace/mode/markdown.js
60:        if (state == "listblock") {

lib/ace/mode/mushcode.js
60:        if (state == "start") {

lib/ace/mode/ocaml.js
79:            state === 'start' && indenter.test(line))

lib/ace/mode/perl.js
68:        if (state == "start") {

lib/ace/mode/pgsql.js
48:        if (state == "start" || state == "keyword.statementEnd") {

lib/ace/mode/php.js
87:        if (state == "start") {
92:        } else if (state == "doc-start") {

lib/ace/mode/powershell.js
34:        if (state == "start") {

lib/ace/mode/praat.js
62:        if (state == "start") {

lib/ace/mode/python.js
60:        if (state == "start") {

lib/ace/mode/ruby.js
65:        if (state == "start") {

lib/ace/mode/scad.js
66:        if (state == "start") {
71:        } else if (state == "doc-start") {
72:            if (endState == "start") {

lib/ace/mode/sh.js
63:        if (state == "start") {

lib/ace/mode/tcl.js
62:        if (state == "start") {

lib/ace/mode/textile.js
48:        if (state == "intag")

lib/ace/mode/twig.js
60:        if (state == "start") {

lib/ace/mode/vala.js
71:        if (state == "start" || state == "no_regex") {
76:        } else if (state == "doc-start") {
77:            if (endState == "start" || endState == "no_regex") {

lib/ace/mode/yaml.js
54:        if (state == "start") {

lib/ace/mouse/dragdrop_handler.js
371:        if (useragent.isIE && this.state == "dragReady") {
377:        if (this.state === "dragWait") {

lib/ace/mode/xquery/jsoniq_lexer.js
4383:        state = (state === 'start' || !state) ? '["start"]' : state;

lib/ace/mode/xquery/xquery_lexer.js
4213:        state = (state === 'start' || !state) ? '["start"]' : state;

lib/ace/snippets.js
408:            if (typeof state === "object") {

lib/ace/tokenizer.js
227:            if (startState === "#tmp") {

If I understand correctly, because it's possible for the state to now be an array of states (rather than just a single state), these checks might need some amending. My understanding is that the function getNextLineIndent(state, line, tab), for example, would pass down 'all' of the current states, and so the logic therein may not work if there are multiple states.

(In RStudio, we use the Ace editor, and a number of our custom modes use logic of the form if (state == ...); we had been using a very old version of Ace for quite a while so certainly the onus is on us to update if the API has indeed changed).

FWIW, I am thinking that a function like e.g. getPrimaryState(), to get the top-level state (for example), might be useful (just to abstract over the fact that state could be a string or an array).

nightwing commented 9 years ago

All this cases should work as expected since https://github.com/ajaxorg/ace/blob/master/lib/ace/mode/text.js#L304 checks for the array. adding getPrimaryState or changing getState to return the primary state and adding another function to return the stack of states might be good, but could you show me an example of the code that actually breaks because of this? I am asking because modes that do not use push do not have to worry about state being an array, and i thought old code should not notice the difference.

kevinushey commented 9 years ago

In RStudio, we had code like this in our c_cpp.js file (as we provided a means for embedding R in it):

this.getLanguageMode = function(position) {
    return this.$session.getState(position.row).match(/^r-/) ? 'R' : 'C_CPP';
};

Ie, we queried the session directly to get the current state, assuming it would be a string. Because this is a custom mode, I understand that in this realm it's our responsibility to handle it, but an API similar to the $delegate above for getting the 'primary' or 'current' state would be helpful for our use case, I think.

mtorromeo commented 8 years ago

I'm getting the error rule.nextState.indexOf is not a function while using mode/markdown.js and it seems related to this issue.

nightwing commented 8 years ago

@mtorromeo could you please show the code for which this error is shown?

mtorromeo commented 8 years ago

@nightwing I cannot share the code but this is an excerpt of the inizialization:

this.editor = ace.edit(this.editorElement);
this.editor.setShowPrintMargin(false);
this.editor.setTheme("ace/theme/" + this.options.theme);
this.editor.session.setUseWrapMode(true);
this.editor.session.setMode("ace/mode/markdown");
this.editor.setValue(this.element.value);
nightwing commented 8 years ago

I don't think the issue is triggered by the way editor is initialized, (unless you have modified markdown mode). It must be caused by markdown snippet you display in the editor. Please try if the error is reproducible on http://ajaxorg.github.io/ace/kitchen-sink.html?doc=Markdown, it should be easy to remove all the parts that you cannot share and keep the error, by replacing all word characters by random word characters

mtorromeo commented 8 years ago

All ace files are unmodified. I don't think the problem lies in the content because I have a bunch of documents and all of them present this behavior.

The kitchen-sink works fine.

Maybe this screenshot of the developer tools helps: https://onedrive.live.com/redir?resid=57702ACE22443C3!1813&authkey=!AJwddgyxIw69b4U&v=3&ithint=photo%2cpng

It's stopped with a breakpoint right before the error triggers. The iteration gets to rules.no_regex[3] where rule is an array instead of an object and nextState is a function instead of a string.

nightwing commented 8 years ago

Unfortunately this doesn't help much, maybe there is something changing Array/object prototypes? Please try creating a small demo that reproduces the problem.

mtorromeo commented 8 years ago

That would be mootools then. This is an older codebase and I cannot easily migrate away from it. Older versions of ace did work fine though...

mtorromeo commented 8 years ago

I just downgraded to 1.1.9 and it's working just fine.

nightwing commented 8 years ago

Ah, of course, Motools always breaks something :( https://github.com/ajaxorg/ace/commit/7c6d99c265d60af716bc5b125839dd1bd5d0df1b should fix that issue, (which is completely unrelated to the original issue reported here)

mtorromeo commented 8 years ago

I should have mentioned it before but thanks for the fix! And sorry about the noise in the wrong bug report.

github-actions[bot] commented 2 years ago

This issue has not received any attention in 1 year. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.