upgrade to api 1.0 #34
Conversation
- use recommended selectors for keymaps - use atom.commands to dispatch commands - use compositedisposable and atom.commands to add and remove workspace commands - use addBottomPanel to add the output-view - use getActiveTextEditor instead of workspaceView.getActiveView()?.getEditor()
- $, $$, $$$, and View come from these modules now
- This restores the informational icons in the editor, however, mouse overs still don't work
- the property still exists, but is no longer a valid event target - fixes tooltips on gutter icons
… not the editor view
- atom seems to have eliminated the idea of a "project root", or obfuscated it...need to research this - some of the suggested deprecation upgrade paths don't work: onWillSave on buffers; TextEditorElement pixelPositionForBufferPosition is not obviously public
- had an extra "," in the code, which was causing the event attachment it to silently fail
…sitionForBufferPosition - as described here: atom/atom#5266
… on the project and use that as the project root - this way we don't have to keep traversing paths, and the project root is set up just once
- no longer need to include the autocomplete module as a dependency, and excluding it eliminates a bunch of deprecation warnings. - Note, autocomplete still appears to call "buildSuggestions" on the provider, even though it isn't documented on the provider API page: https://github.com/atom-community/autocomplete-plus/wiki/Provider-API Since it isn't flagged as deprecated, I presume its safe to leave it, rather than port to the requestHandler layout
|
Hey @chaika2013, bump! Is there something that should be done to this PR for it to be acceptable? Please comment / merge. |
| @@ -36,7 +35,13 @@ PREFIX_IMPORT_FUNCTIONS = new RegExp("[\\(\\s,]+(" + REGEXP_FUNCTION_NAME_MAY | |||
| # regexps for completion in functions | |||
| PREFIX_FUNCTION_NAME = new RegExp("(" + REGEXP_FUNCTION_NAME_JUST + "|" + REGEXP_ALIAS_NAME_CAP + "\\." + REGEXP_FUNCTION_NAME_MAYBE + ")$") | |||
|
|
|||
| class CompleteProvider extends Provider | |||
| class Suggestion | |||
| constructor: (@provider, options) -> | |||
There was a problem hiding this comment.
You do not need to track @Provider. Consider removing it, it's not used by autocomplete-plus.
|
@chaika2013 I take that back, this PR requires work; I am making suggestions on what should be done. |
|
@joefitzgerald , Thanks for the feedback. I will make the changes that you mentioned. |
- eliminate @Provider from Suggestion class, isn't needed - register a stub provider with autocomplete using the service api; in that object's requestHandler, forward the request to an editor-specific CompleteProvider that we create
|
@joefitzgerald, I have updated this to the new autocomplete API. |
|
Note: the provider api wiki page has been updated to reflect the new package.json / config based registration technique. This will resolve your deprecation warnings about atom.services. |
|
Also, if you need to track the current editor and cache data associated with it, you can look at FuzzyProvider for an example of how to do that. |
|
I think that using in-editor variable is not an ideal solution. Using a WeakMap to track editors and their respective providers would be better IMO. You may take a look at what I did if you are unsure of how to use it (disregard anything about emitter and ghc-mod and you've got yourself a minimal example)
|
| @@ -14,17 +26,30 @@ isHaskellSource = (fname) -> | |||
| return true | |||
| return false | |||
|
|
|||
| getElementsByClass = (elem,klass) -> | |||
| # this is pretty obtuse, but jquery doesn't seem to support inspecting the shadowRoot directly | |||
| # can remove the fallback once the option to disable the shadow dom is gone | |||
There was a problem hiding this comment.
You might want to drop jQuery altogether and use rootElement instead of shadowRoot, which will get you root element for shadow dom or otherwise. You might also consider using querySelectorAll instead of getElementsByClassName, which has semantics almost exactly like jQuery.find
…register with autocomplete
|
@jmquigs Providers are registered for scopes. Scopes have no notion of filename. So you really need to move to a cache (which is file aware) within a single provider. That is, you would track all open editors, in a single provider. You would cache the results of your long-running activities (by file). You would attach editor events such that when a file is saved, you update the cache for that file (if changed). |
|
By the way, I am planning on completely removing the old API on Tuesday, February 10th, 2015. I will obviously closely watch this issue in the interim, as you're clearly making lots of progress and I don't want to cause issues if you're 99% of the way to getting this solved. |
|
@joefitzgerald, well, I hear that ide-haskell autocompletion does not work anyway at the moment (#35 and also this), so it's not like you'll break anything by removing old ac-plus API. |
|
@joefitzgerald, as for your previous comment, there is slight miscommunication here, I reckon. There is a provider instance one passes to autocomplete-plus via service API. And then there's suggestion builder, which is called |
|
@lierdakil is right, the class called "CompleteProvider" here is an internal per-editor object that is not registered with autocomplete. It used to be registered with autocomplete. I probably should have renamed it when I stopped registering it; "SuggestionBuilder" is a good name. The actual provider is registered in the new "provideAutocomplete" function which I just added earlier today, and that is a singleton. That being said, I probably can get away with a single CompleteProvider/SuggestionBuilder instead of multiple, and just track the list open files separately as @lierdakil mentioned. Just want to be careful that I don't break stuff further here; there isn't much in the way of specs here (yet)... Also Re: feb 10, yeah the deployed ide-haskell plugin is pretty much broken already (even beyond autocomplete issues). There are actually some bugs that I want to fix with it still that this PR doesn't address. |
|
@jmquigs please bear in mind that further bug-fixing may be postponed for months. I managed to convince @chaika2013 to merge and deploy this PR when ready (assuming it will be ready soon), but he's extremely busy right now, so I seriously doubt he'll find time to deploy further work for a while. TL;DR we need to solve package-breaking bugs in this PR, and pray it would last. |
|
So you guys have reached out to the owner of this repo and there's no ETA on when he'll be back to maintain? If so, might want to pursue transferring this to an organization (or forking it to one) like atom-community, so you can add committeers and not get stuck like this. Or am I reading this wrong and the owner is willing to merge when this is done? |
|
@joefitzgerald For your question, see my previous comment. TL;DR -- he is willing to merge this as soon as possible, but not anything beyond this (for now). Btw, it's a bit on offtopic, but could you enlighten me as to how package ownership works at atom.io? Specifically, is it possible to give someone permissions to publish updates without sharing credentials and/or API keys? |
|
Yes, ideally we'd be able to deploy updates to the package; it sounds like he will do an update for this PR, but after that, perhaps not. As for this PR, the only big issue that I haven't addressed yet is the fact that this package still requires you to manually deactivate and then activate ide-haskell before it "kicks in" after opening a project. Thats pretty annoying, and I haven't delved into it yet, but I don't think it will be too hard to fix. There are a couple other minor bugs that I have noticed that don't have fixes here either. |
|
If @chaika2013 would give anyone on this thread commit rights to this repo, they could then publish package releases |
|
Sounds like @chaika2013 should take himself off the critical path if he doesn't have time and consider adding one or both of you as a committer. |
|
I'll discuss it with him when I have a chance. |
- Eliminates the need for a ".haskellController" field on the editor - Unfortunately we still need to hook the editor delete event, so that we can clean up the controller (if it simply disappears from weakmap, a bunch of things it creates won't get cleaned up).
- Eliminates the need for a ".haskellCompletionProvider" field on the editor - As with the editor controllers, because these objects require explicit cleanup, we need to do so in the editor destroy event.
… project settings object - the project settings are per-project, and are initialized by by ide-haskell::activate
…a cabal project is detected when atom is first loaded - The issue is that activate() is only called once, and if the package did not detect a cabal file, it would never re-scan - fixed by watching for opened files, and activating if it isn't already active but a haskell file is opened - couldn't find a specific bug that mentions this, but atom-haskell#27 sounds like it (in addition to describing the disable/enable workaround that was required to fix it)
|
Some more changes:
@lierdakil, can you pull down to review? I'd appreciate it. |
|
Looking right now. |
| @@ -8,9 +8,17 @@ | |||
| "engines": { | |||
| "atom": ">=0.116.0" | |||
There was a problem hiding this comment.
Please bump atom version to 0.177.0 or whatever version you are using. It's definitely not 0.116.0. I believe apm just recently implemented checking this version, but it's a good practice nonetheless.
There was a problem hiding this comment.
I'm pretty sure apm doesn't respect this yet. But yes, it's good hygiene.
|
I'd also appreciate if you updated changelog (I reckon this should be a minor release, but that's up for debate). Other than that, it looks good to me. |
|
I pushed these updates, thanks for reviewing |
|
All right. Thanks for your work @jmquigs! Now to get this deployed... |
|
Right. Merged and deployed. I've also got commit permissions for this repo, so feel free to create other pull requests if you so desire. |
|
@lierdakil, cool! Thanks for doing that. There are a few other issues I'm going to look at. They should be smaller PRs than this one. |
This upgrades ide-haskell to atom api 1.0. It fixes most deprecation warnings and issues with the shadow DOM. I didn't fix any other issues that I've noticed - such as the need to disable and re-enable the package before it activates - but I will look at those next, now that I sort of know my way around.
This also upgrades to the new autocomplete API; autocomplete no longer needs to be a package dependency.