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

LSPDocument needs to be in scide_vscode folder #17

Open
cdbzb opened this issue Nov 7, 2023 · 5 comments
Open

LSPDocument needs to be in scide_vscode folder #17

cdbzb opened this issue Nov 7, 2023 · 5 comments

Comments

@cdbzb
Copy link

cdbzb commented Nov 7, 2023

just a friendly reminder!

LSPDocument.sc needs to be in scide_vscode folder or sclang will not boot from SCIde

@jamshark70
Copy link
Contributor

To explain this a bit further... it might have seemed necessary to move only the document class stub into scide_vscode, but it looks like it's actually necessary to move LSPDocument's definition as well.

    *initClass{
        asyncActions = IdentityDictionary.new;
        Document.implementingClass = LSPDocument;
    }

implementingClass_ does not exist in the main class library.

So it is currently not supported for sclang running via SCIDE to attempt to initialize the LSPDocument class. The way to prevent sclang-via-SCIDE from loading this class definition at all is to put it into scide_vscode along with the Document stub.

Or, if this isn't desirable -- that is, if there may be a need for an editor other than VSCode to use LSPDocument -- then at least tryPerform so that SCIDE doesn't barf.

        Document.tryPerform(\implementingClass_, LSPDocument);

@cdbzb
Copy link
Author

cdbzb commented Nov 8, 2023

@davidgranstrom !

@jamshark70
Copy link
Contributor

Or, if this isn't desirable -- that is, if there may be a need for an editor other than VSCode to use LSPDocument -- then at least tryPerform so that SCIDE doesn't barf.

I realized later, this is something that needs some careful thought.

Let's say that Document is refactored so that there's always a minimal stub called Document, which delegates its functionality to an implementationClass or implementingClass (side note: currently both of these exist, and may be in conflict).

Let's say also that we want LSPDocument to be available to multiple editors that support the language server protocol. In that case, it would be wrong to put LSPDocument into an editor-specific folder -- so we wouldn't be able to rely on that mechanism to prevent LSPDocument in the IDE.

That raises the prospect of a user who sometimes uses a language-server environment (with LSPDocument), and sometimes uses SC-IDE. This user would (I guess) have no choice but to have two sclang_conf files (and problems syncing quark config) because if they don't, then, in SCIDE, sclang would try to load both ScIDEDocument and LSPDocument, both of which would would set implementingClass -- and the "winner" would be indeterminate.

In fact, at one time, there was a minimal Document class, with a subclass ScIDEDocument (i.e. it used to be factored in the way Scott is proposing). This was removed (see supercollider/supercollider@5033eb3). One valid reason for removing it is that it eliminates confusion from potentially having multiple implementing subclasses loading at the same time. So, re-factoring to return to that state might turn out not to be ideal. (Or, if we are sure it's the way t to go, then there has to be a bulletproof way to ensure that only one implementing subclass can ever be active at one time, and that this always matches the editor being used. TBH this sounds like it could be more complicated than the current approach -- so the engineering concern of "prefer the simplest design that gets the job done" might apply here.)

@cdbzb
Copy link
Author

cdbzb commented Nov 8, 2023

veering a little OT bit if multiple sclang_conf files turns out to be the answer, this PR supercollider/supercollider#6068 allows for additive conf files ie VSCode (or SCNVim) could add the language server quark to an existing sclang_conf file.

@themissingcow
Copy link

Let's say also that we want LSPDocument to be available to multiple editors that support the language server protocol. In that case, it would be wrong to put LSPDocument into an editor-specific folder -- so we wouldn't be able to rely on that mechanism to prevent LSPDocument in the IDE.

Fwiw, came across this attempting to get the language server working with David's excellent scnvim (which requires its own -i scnvim + classes), so being able to enable the language server without it requiring to its own IDE specifier would be very useful.

dannyZyg pushed a commit to dannyZyg/LanguageServer.quark that referenced this issue Mar 20, 2024
The long-term goal seems to be to have a base class in Sc itself, shared
by the various IDE specific document classes to reuse some of the code.

Until this happens upstream, calling it 'Document' creates collisions
and forces it to be moved to an IDE specific folder. This in itself
causes challenges when you wish to share config between different
environments. For example scnvim and an LSP instance - as scnvim uses
its own IDE identifier/classes.

For now, rename this to BaseDocument, and move it back out of the vscode
specific folder, so it can be readily used in any sclang instance, and
there is no need for a specific config to avoid loading the quark.

Relates to issue scztt#17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants