scztt / LanguageServer.quark

16 stars 10 forks source link

LSPDocument needs to be in scide_vscode folder #17

Open cdbzb opened 10 months ago

cdbzb commented 10 months ago

just a friendly reminder!

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

jamshark70 commented 10 months ago

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 commented 10 months ago

@davidgranstrom !

jamshark70 commented 10 months ago

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 https://github.com/supercollider/supercollider/commit/5033eb36f77428a5734ff00df04a2ec1af8367b6). 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 commented 10 months ago

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

themissingcow commented 5 months ago

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.