Tolerate Escaped Right Braces And Empty Groups #388

joefitzgerald · 2015-04-14T12:48:08Z

Fixes Hide snippet (or $0) in suggestions #375

atom/snippets#123 allows snippets to contain escaped right braces (\\}). This might be useful - for example - to display interface{} within a snippet's body (as is common for go suggestions). This PR allows them to be displayed correctly in the suggestion list. Additionally, it resolves #375, by removing empty snippet groups from the text displayed in the suggestion list.

E.g.:

hello$0 > hello
hello${0} > hello
hello${0:} > hello
${0:hello} > hello
${0:hello{\\}} > hello{}

When reviewing the code, you may find the lack of regex ugly, or overly complex. However, without the presence of lookbehind in javascript regexes, an alternative approach is required to differentiate an escaped right brace from an unescaped right brace.

Additionally, a small epilogue:

This also does away with snippet markers; they seem unnecessary. Implicitly, the moment a person who is typing reaches the first snippet, their replacement prefix will no longer match - the snippetMarker (|) will not match the text in almost all scenarios.

benogle · 2015-04-14T16:44:41Z

lib/suggestion-list-element.coffee

-    # 1. Pull the snippets out, replacing with placeholder
-    # 2. Highlight relevant characters
-    # 3. Place snippet HTML back at the placeholders
+  getHighlightedHTML: (text, snippet, replacementPrefix) =>


Why the fat arrow?

Fixed in 4fda987.

benogle · 2015-04-14T17:06:40Z

This also does away with snippet markers; they seem unnecessary. Implicitly, the moment a person who is typing reaches the first snippet, their replacement prefix will no longer match - the snippetMarker (|) will not match the text in almost all scenarios.

The markers are there so the snippets dont get highlighted in the char highlight process. There are tests for this, but you've edited them so they no longer test what they're supposed to. See https://github.com/atom-community/autocomplete-plus/issues/301 and atom-community@37f2151

benogle · 2015-04-14T17:09:58Z

spec/autocomplete-manager-integration-spec.coffee

@@ -336,7 +335,7 @@ describe 'Autocomplete Manager', ->
              charMatch = editorView.querySelector('.autocomplete-plus li span.word .character-match')
              expect(word.textContent).toBe 'ab(c)c'
              expect(charMatch.textContent).toBe 'c'
-              expect(charMatch.parentNode).toHaveClass 'word'
+              expect(charMatch.parentNode.parentNode).toHaveClass 'word'


This makes the test invalid. The entire point of this test is this line. In this new code, the first c, the snippet, will be highlighted, when the second c is the one that should be highlighted.

@benogle I think the tests you have are invalid. It should be valid to have a character match for something within the snippet body. It's just not valid to match any of the HTML wrapping a snippet (i.e.  or ) or the snippet definition (i.e. ${0: or }).

With my new logic, I'm able to stably determine the snippet definition text and exclude it from character matches, prior to insertion of snippet HTML. Consequently, the tests need to change. I think the intent of #301 is to ensure that:

Snippet HTML does not make its way into the suggestion list

Snippet HTML is not highlighted

Snippet definition text does not make its way into the suggestion list

Snippet definition text does not cause incorrect highlighting of character matches

This is now achieved with my latest changes, that I am about to push.

If you are going to change the spec, check that the parent node is the snippet span class, not this parent.parent indirect check.

benogle · 2015-04-14T17:15:15Z

I'm fine with the non-regex parser, but i think we need to come up with a way to separate the html generation of the snippets with that of the char highlights. My thinking:

Run the snippet parser. This will give you indices into where they are, and you can gen the HTML for them.
Run the char match highlighting code without the snippets in there. My solution was the markers, which maybe arent ideal.
Compose the char highlighted text with the snippet highlighted text.
Return the composed text

benogle · 2015-04-14T17:16:46Z

lib/suggestion-list-element.coffee

+        text += char
+        continue
+
+    return text


just text

Fixed in 4fda987.

joefitzgerald · 2015-04-16T00:15:37Z

@benogle 👀

benogle · 2015-04-16T00:42:01Z

Looking...

benogle · 2015-04-16T00:44:43Z

lib/suggestion-list-element.coffee

+    return text unless text?.length and text.indexOf('$') isnt -1 # No snippets
+    text.replace(@emptySnippetGroupRegex, '') # Remove all occurrences of $0 or ${0} or ${0:}
+
+  removeAllSnippets: (text) ->


Looks like nothing calls this method?

Yes, need to remove that. Ignore that function.

benogle · 2015-04-16T01:19:50Z

Overall, it uses a lot of arrays, and this function will be run a lot. Like several times for each list display. I feel like the skipChar arrays can be removed. The approach in my head is similar to yours, just with different replacement var contents:

Run findSnippets to find the start, end, snippetTextStart, snippetTextEnd
Assemble the text as it would be displayed to the user (call it replacementText)
Run that text through findCharacterMatches to get a list of indices
Use the same algorithm you have now to walk replacementText and assemble the HTML based on the indices you have, using math on snippetTextStart

e.g.

snippet = 'text(${1:ab}, ${2:cd})'

# Run findSnippets(snippet)
snippets = [{
  start: 5
  end: 11
  textStart: 9
  textEnd: 10
  text: 'ab'
}, {
  start: 14
  end: 20
  textStart: 18
  textEnd: 19
  text: 'ab'
}]

# Use snippets to assemble this
replacementText = 'text(ab, cd)'

# Mung snippets to have indices into `replacementText`
snippets = [{
  start: 5
  end: 11
  textStart: 9
  textEnd: 10
  replacementStart: 5
  replacementEnd: 6
  text: 'ab'
}, {
  start: 14
  end: 20
  textStart: 18
  textEnd: 19
  replacementStart: 9
  replacementEnd: 10
  text: 'cd'
}]

# Run findCharacterMatches(replacementText)
characterMatches = [0, 2, 5]

displayHTML = ''
for replacementChar, index in replacementText
  # update displayHTML using your indices.

return displayHTML

joefitzgerald · 2015-04-16T01:22:44Z

Got any idea why the symbol-provider-specs are failing?

benogle · 2015-04-16T01:23:15Z

It's a flaky test. It almost always fails on the pr build.

benogle · 2015-04-16T01:28:43Z

Also, you could do away with the indexOf checks with 2 objects: one for snippet starts/ends, and one for char matches. So maybe findCharacterMatches returns an object {0: true, 2: true, 5: true}, and instead of adding replacementStart/End to the snippets array, you create another object: {5: 'start', 6: 'end', ...}

joefitzgerald · 2015-04-16T01:37:01Z

I'm not sure switching the snippet info over to 'start' / 'end' saves anything; is it just to avoid indexOf checks? The storage of the information will be larger.

benogle · 2015-04-16T01:37:41Z

It saves allll the skipChar arrays.

benogle · 2015-04-16T01:40:01Z

Wait, you're referring to the last comment. Yes, it's saving the indexOf checks, and saving constant iteration on the snippets array (which admittedly is small).

Also, I'm not optimizing for absolute storage as this stuff is temp. I want to optimize for minimal garbage and cycles.

benogle · 2015-04-16T01:44:01Z

I'm not sure switching the snippet info over to 'start' / 'end' saves anything; is it just to avoid indexOf checks? The storage of the information will be larger.

And to be clear, I meant creating an object with the 'start' and 'end' strings in place of the Mung snippets step.

- Fixes #375

- ... then perform one pass over the text to add HTML / remove snippet definitions

joefitzgerald · 2015-04-16T14:46:15Z

@benogle 👀 on the latest changes please.

benogle · 2015-04-16T18:28:26Z

Ok looking / tinkering

joefitzgerald · 2015-04-16T23:53:15Z

Those tests seem to fail pretty reliably for stable reasons. Could it be they need modifying?

benogle · 2015-04-17T00:06:47Z

Could it be they need modifying?

Yes, they should be fixed to reliably pass.

I've left feedback in the form of a PR: https://github.com/atom-community/autocomplete-plus/pull/396

joefitzgerald added the enhancement label Apr 14, 2015

joefitzgerald assigned benogle Apr 14, 2015

joefitzgerald added the in progress label Apr 14, 2015

benogle reviewed Apr 14, 2015
View reviewed changes

benogle assigned joefitzgerald and unassigned benogle Apr 14, 2015

joefitzgerald force-pushed the jf-enhance-snippet-display branch 3 times, most recently from 0a8fe6b to e0385de Compare April 16, 2015 00:06

benogle reviewed Apr 16, 2015
View reviewed changes

joefitzgerald added 4 commits April 17, 2015 00:36

Tolerate Escaped Right Braces And Empty Groups

7e9f4e4

- Fixes #375

Remove Unused Regexes

3da7d95

🎨 Cleanup

6912ab7

Find Snippets, Store Key Indices

a3f130d

joefitzgerald added 3 commits April 17, 2015 00:36

Find Indices For Char Matches And Snippets

b9859f5

- ... then perform one pass over the text to add HTML / remove snippet definitions

🎨 Remove Unused Function

c2df821

💪 Try To Reduce Garbage

8904e0b

joefitzgerald force-pushed the jf-enhance-snippet-display branch from ee736ab to 8904e0b Compare April 16, 2015 14:40

Fix Specs To Test The Correct Thing

d19d60e

Fix Escape Char (It's Not '\\', it's '\')

a75b2a8

benogle mentioned this pull request Apr 17, 2015

My take on Tolerate Escaped Right Braces And Empty Groups #396

Merged

benogle closed this in #396 Apr 17, 2015

joefitzgerald removed the in progress label Apr 17, 2015

benogle deleted the jf-enhance-snippet-display branch April 17, 2015 19:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tolerate Escaped Right Braces And Empty Groups #388

Tolerate Escaped Right Braces And Empty Groups #388

joefitzgerald commented Apr 14, 2015

benogle Apr 14, 2015

joefitzgerald Apr 14, 2015

benogle commented Apr 14, 2015

benogle Apr 14, 2015

joefitzgerald Apr 15, 2015

benogle Apr 16, 2015

benogle commented Apr 14, 2015

benogle Apr 14, 2015

joefitzgerald Apr 14, 2015

joefitzgerald commented Apr 16, 2015

benogle commented Apr 16, 2015

benogle Apr 16, 2015

joefitzgerald Apr 16, 2015

benogle commented Apr 16, 2015

joefitzgerald commented Apr 16, 2015

benogle commented Apr 16, 2015

benogle commented Apr 16, 2015

joefitzgerald commented Apr 16, 2015

benogle commented Apr 16, 2015

benogle commented Apr 16, 2015

benogle commented Apr 16, 2015

joefitzgerald commented Apr 16, 2015

benogle commented Apr 16, 2015

joefitzgerald commented Apr 16, 2015

benogle commented Apr 17, 2015

Tolerate Escaped Right Braces And Empty Groups #388

Tolerate Escaped Right Braces And Empty Groups #388

Conversation

joefitzgerald commented Apr 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benogle commented Apr 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benogle commented Apr 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joefitzgerald commented Apr 16, 2015

benogle commented Apr 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benogle commented Apr 16, 2015

joefitzgerald commented Apr 16, 2015

benogle commented Apr 16, 2015

benogle commented Apr 16, 2015

joefitzgerald commented Apr 16, 2015

benogle commented Apr 16, 2015

benogle commented Apr 16, 2015

benogle commented Apr 16, 2015

joefitzgerald commented Apr 16, 2015

benogle commented Apr 16, 2015

joefitzgerald commented Apr 16, 2015

benogle commented Apr 17, 2015