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

add subsequence provider #886

Merged
merged 17 commits into from Sep 29, 2017

Conversation

Projects
None yet
5 participants
@leroix
Contributor

leroix commented Sep 13, 2017

Description of the Change

I wanted to get this up for feedback sooner rather than later.

This adds the subsequence provider which uses the soon-to-be-merged
findWordsWithSubsequence method on TextBuffer atom/superstring#22. I've also moved all the logic about the provider config into its own class.

I've just copied to tests over from the symbol provider and added a few where it made sense.

Alternate Designs

SymbolProvider and FuzzyProvider store token lists that have to be created when new buffers load and kept in sync with buffer changes. This can be somewhat slow and happens on the main thread--often causing long frames.

Benefits

Using the findWordsWithSubsequence on TextBuffer does the work of finding matches in a separate thread never blocking a render (yay performance!) and greatly simplifies the logic in autocomplete-plus.

Possible Drawbacks

Currently, we don't have support for the enableExtendedUnicodeSupport, but, hopefully, we can get support added shortly.

TODO:

  • fix crashing issue in findWordsWithSubsequence
  • evaluate performance (improved?)
  • get atom/superstring#22 merged and have text-buffer use the new release
  • clamp the search range in large buffers

leroix added some commits Sep 13, 2017

add subsequence provider
This adds the subsequence provider which uses the soon-to-be-merged
`findWordsWithSubsequence` method on `TextBuffer`. I've also moved all
the logic about the provider config into its own class. This is still
WIP. I've just copied to tests over from the symbol provider, and I'm
working on making them pass.
@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Sep 13, 2017

Contributor

I think it could be okay to land a non-default provider on master that doesn't pass all the tests from the symbols provider (the tests could be commented out at first). But I like the approach of using the old tests. Once they all pass, it may make sense to clean them up if any cleanup is needed, but getting 100% passing against the old suite is a good idea for ensuring compatibility.

Contributor

nathansobo commented Sep 13, 2017

I think it could be okay to land a non-default provider on master that doesn't pass all the tests from the symbols provider (the tests could be commented out at first). But I like the approach of using the old tests. Once they all pass, it may make sense to clean them up if any cleanup is needed, but getting 100% passing against the old suite is a good idea for ensuring compatibility.

leroix added some commits Sep 14, 2017

get subsequence provider working
 - fix a few remaining bugs
 - port over a few tests from symbol provider (more to come)

@leroix leroix changed the title from add subsequence provider (WIP) to add subsequence provider Sep 19, 2017

leroix added some commits Sep 19, 2017

remove words under cursors from suggestions
I also fixed the tests. I thought Atom was using jasmine 2 for some
reason. Not caffeinated enough probably. All the tests are passing
except for "when the autocomplete.symbols changes between scopes". I'm
not sure what this one is trying to test exactly. Need to look into it
more.
@maxbrunsfeld

This is looking good. I like your strategy of porting all of the tests from the symbol provider. I left one suggestion for optimizing getSuggestions.

Show outdated Hide outdated lib/subsequence-provider.js Outdated
Show outdated Hide outdated spec/subsequence-provider-spec.js Outdated
Show outdated Hide outdated spec/subsequence-provider-spec.js Outdated
Show outdated Hide outdated spec/subsequence-provider-spec.js Outdated
@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Sep 27, 2017

Contributor

One other thing; for all the new files that you introduce, I would recommend dropping the dependency on babel (via the use babel string). I think that the only code change required is using require instead of import and module.exports instead of the export keyword.

Contributor

maxbrunsfeld commented Sep 27, 2017

One other thing; for all the new files that you introduce, I would recommend dropping the dependency on babel (via the use babel string). I think that the only code change required is using require instead of import and module.exports instead of the export keyword.

finish a few remaining items
 - remove babel from new files
 - limit search range in large buffers in subsequence provider
 - add new tests
@leroix

This comment has been minimized.

Show comment
Hide comment
@leroix

leroix Sep 29, 2017

Contributor

It seems like CI is downloading an older commit of Atom that doesn't have findWordsWithSubsequence 😕

Contributor

leroix commented Sep 29, 2017

It seems like CI is downloading an older commit of Atom that doesn't have findWordsWithSubsequence 😕

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Sep 29, 2017

Contributor

Yeah, CI runs against the latest stable and beta releases, so we'll have to put a conditional in the test suite that skips these new tests if .findWordsWithSubsequence doesn't exist.

Contributor

maxbrunsfeld commented Sep 29, 2017

Yeah, CI runs against the latest stable and beta releases, so we'll have to put a conditional in the test suite that skips these new tests if .findWordsWithSubsequence doesn't exist.

@leroix

This comment has been minimized.

Show comment
Hide comment
@leroix

leroix Sep 29, 2017

Contributor

@nathansobo @maxbrunsfeld I'm ready to merge this assuming CI passes. Thoughts?

Subsequence Provider tests are wrapped in a conditional on whether .findWordsWithSubsequence exists.

Contributor

leroix commented Sep 29, 2017

@nathansobo @maxbrunsfeld I'm ready to merge this assuming CI passes. Thoughts?

Subsequence Provider tests are wrapped in a conditional on whether .findWordsWithSubsequence exists.

leroix added some commits Sep 29, 2017

@leroix leroix merged commit 8641402 into atom:master Sep 29, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@leroix leroix deleted the leroix:subsequence-provider branch Sep 29, 2017

@maxbrunsfeld maxbrunsfeld referenced this pull request Sep 29, 2017

Merged

Flesh out the regex search APIs #35

8 of 8 tasks complete
@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Sep 29, 2017

Contributor

Nice! What's on the critical path of enabling this provider by default?

Contributor

nathansobo commented Sep 29, 2017

Nice! What's on the critical path of enabling this provider by default?

@leroix

This comment has been minimized.

Show comment
Hide comment
@leroix

leroix Sep 29, 2017

Contributor

I think it's pretty much ready to go with the exception of a bit of dog-fooding to root out the unintuitive matches that might crop up with the new matching algo and to make sure it feels fast 🐎 .

All the functionality from Symbol Provider is supported, but unicode can't be turned off. That seems to be determined by a system level setting in superstring. Perhaps, we should remove that setting from autocomplete-plus? I wouldn't say that's blocking making subsequence the default default provider though.

Contributor

leroix commented Sep 29, 2017

I think it's pretty much ready to go with the exception of a bit of dog-fooding to root out the unintuitive matches that might crop up with the new matching algo and to make sure it feels fast 🐎 .

All the functionality from Symbol Provider is supported, but unicode can't be turned off. That seems to be determined by a system level setting in superstring. Perhaps, we should remove that setting from autocomplete-plus? I wouldn't say that's blocking making subsequence the default default provider though.

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Sep 29, 2017

Contributor

I think removing the setting for unicode support will be fine; seems like unicode support no longer has any cost.

Contributor

maxbrunsfeld commented Sep 29, 2017

I think removing the setting for unicode support will be fine; seems like unicode support no longer has any cost.

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Sep 29, 2017

Member

Perhaps, we should remove that setting from autocomplete-plus?

👍

Member

Ben3eeE commented Sep 29, 2017

Perhaps, we should remove that setting from autocomplete-plus?

👍

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Sep 29, 2017

Contributor

🆒 I propose we dog-food for one release cycle and enable by default before the following beta release if everything is copacetic.

Contributor

nathansobo commented Sep 29, 2017

🆒 I propose we dog-food for one release cycle and enable by default before the following beta release if everything is copacetic.

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