Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix autocomplete prefix for custom identifierRegexes #4919

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AprilArcus
Copy link

@AprilArcus AprilArcus commented Sep 6, 2022

Hi, Ace team! My team has been using a fork of Ace, and I'm hoping to merge some of our changes upstream so that we can return to using your official releases.

The engineer who originally accomplished this work has long since departed my team, so forgive me for this somewhat rote transcription of his working notes.

This was sent upstream in #2352, but closed because fcebd0f seemed to address the same issue. That fix unfortunately does not work as we need, and fails the behavior described in our test named test leading @ not duplicated on autocomplete

The root cause: Autocomplete prefix can be wrong for completions with a custom identifierRegex because Autocompleter.base is computed relative to a prefix computed for the default identifierRegex.

Below, I reproduce the test my colleague referred to.

'test leading @ not duplicated on autocomplete': function (test) {
    editor.setValue('');
    editor.navigateFileStart();

    var text = 'view: users { derived_table: { sql: @{;; } }';
    exec('insertstring', 1, text);
    editor.moveCursorTo(0, 38);
    editor.getValue();

    attachedSymbolsToSession(text, editor.session, { const: 'value' });
    editor.execCommand('startAutocomplete');

    var completion = editor.completer.completions.filtered.filter(function(completion) { return completion.caption === '@{const}'; });
    test.ok(completion[0]);

    editor.completer.insertMatch(completion[0]);
    test.equal(editor.getValue(), 'view: users { derived_table: { sql: @{const};; } }');

    test.done();
},

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@AprilArcus AprilArcus force-pushed the fix-autocomplete-prefix branch from c266a7b to fc7659b Compare September 6, 2022 21:45
@andrewnester
Copy link
Contributor

@nightwing could you please take a look at the PR?

@andrewnester
Copy link
Contributor

@AprilArcus could you please address failing build? Seems it's just related to missing semicolon

Hi, Ace team! My team has been using a fork of Ace, and I'm hoping to
merge some of our changes upstream so that we can return to using your
official releases.

The engineer who originally accomplished this work has long since
departed my team, so forgive me for this somewhat rote transcription of
his working notes.

> This was sent upstream in ajaxorg#2352,
> but closed because ajaxorg@fcebd0f
> seemed to address the same issue.  That fix unfortunately does not
> work as we need, and fails the behavior described in our test named
> `test leading @ not duplicated on autocomplete`
>
> The root cause: Autocomplete prefix can be wrong for completions with
> a custom identifierRegex because `Autocompleter.base` is computed
> relative to a prefix computed for the default identifierRegex.

Below, I reproduce the test my colleague referred to.

```
'test leading @ not duplicated on autocomplete': function (test) {
    editor.setValue('');
    editor.navigateFileStart();

    var text = 'view: users { derived_table: { sql: @{;; } }';
    exec('insertstring', 1, text);
    editor.moveCursorTo(0, 38);
    editor.getValue();

    attachedSymbolsToSession(text, editor.session, { const: 'value' });
    editor.execCommand('startAutocomplete');

    var completion = editor.completer.completions.filtered.filter(function(completion) { return completion.caption === '@{const}'; });
    test.ok(completion[0]);

    editor.completer.insertMatch(completion[0]);
    test.equal(editor.getValue(), 'view: users { derived_table: { sql: @{const};; } }');

    test.done();
},
```
@AprilArcus AprilArcus force-pushed the fix-autocomplete-prefix branch from fc7659b to e4e7582 Compare September 19, 2022 22:11
@AprilArcus
Copy link
Author

done

@codecov
Copy link

codecov bot commented Sep 20, 2022

Codecov Report

Base: 85.75% // Head: 85.85% // Increases project coverage by +0.10% 🎉

Coverage data is based on head (e4e7582) compared to base (ff3dd69).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4919      +/-   ##
==========================================
+ Coverage   85.75%   85.85%   +0.10%     
==========================================
  Files         539      542       +3     
  Lines       40728    41138     +410     
  Branches     6445     6515      +70     
==========================================
+ Hits        34927    35320     +393     
- Misses       5801     5818      +17     
Flag Coverage Δ
unittests 85.85% <100.00%> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/autocomplete.js 65.70% <100.00%> (+0.64%) ⬆️
src/virtual_renderer.js 79.83% <0.00%> (-0.22%) ⬇️
src/layer/decorators.js 97.56% <0.00%> (ø)
src/scrollbar_test.js 98.76% <0.00%> (ø)
src/scrollbar_custom.js 95.15% <0.00%> (ø)
src/virtual_renderer_test.js 98.88% <0.00%> (+0.22%) ⬆️
src/layer/gutter.js 90.45% <0.00%> (+0.35%) ⬆️
src/lib/event.js 73.30% <0.00%> (+0.48%) ⬆️
src/range.js 85.41% <0.00%> (+0.69%) ⬆️
src/scrollbar.js 83.51% <0.00%> (+2.19%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@AprilArcus
Copy link
Author

AprilArcus commented Jul 28, 2023

Can anyone help me rebase this over #5084? I'm stumped.

@azmkercso @andrewnester

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants