davidgranstrom / scnvim

Neovim frontend for SuperCollider.
GNU General Public License v3.0
197 stars 26 forks source link

[BUG] scala file type overrides .sc filetype mapping #156

Closed eirikblekesaune closed 2 years ago

eirikblekesaune commented 2 years ago

Describe the bug When opening a .sc file the detected file type becomes scala. This is not a problem with .scd files. Not entirely sure whether this is a bug or a missing feature in the README.

Expected behavior I would like to be able to see in the README how to efficiently override this nvim behaviour, and be able to set .sc files to be openend as filetype supercollider.

Steps to reproduce Open .sc file with nvim.

Additional context The filetype value is set through an autocmd from augroup filetypedetect in /usr/share/nvim/runtime/filetype.vim. This loads the /usr/share/nvim/runtime/ftplugin/scala.vim, which for instance overrides the indentation settings for scnvim (which is why I discovered this behaviour)

Information

eirikblekesaune commented 2 years ago

This is related to this change in neovim source code: https://github.com/neovim/neovim/commit/7a239a8a9affc2bac215dec1d08e55e1b065a4c4

eirikblekesaune commented 2 years ago

I'm sorry but I have to admit that I struggle with reproducing this everytime I open a .sc file. Sometime the filetype is set to scala and sometimes to supercollider. What is consistent though are the indentation settings, they constantly follow the indentation settings from scala.vim.

madskjeldgaard commented 2 years ago

In my setup it always sets the filetype as supercollider but the indentation settings from scala prevail.

davidgranstrom commented 2 years ago

@eirikblekesaune Thanks for the report. It looks indeed that there might be a conflict with that recent change in the vim runtime files. But I'm not exactly sure how to proceed.. I have a vague recollection that there is a way to parse part of the buffer to detect the filetype if there is an ambiguity with the file extension. I'll have to read up on that.

@madskjeldgaard Are you also on nvim 0.6? What is the value of set indentexpr? after you have opened a .sc file and comparing with a .scd?

madskjeldgaard commented 2 years ago

I am on 0.5 as well.

Kenneth Flak found out the solution:

Add

setlocal shiftwidth=4
setlocal tabstop=4
setlocal expandtab

to <your_nvim_config_dir>/after/ftplugin/supercollider.vim

Perhaps this should be done by scnvim?

salkin-mada commented 2 years ago

i think we need to make sure it is configurable with global options if local ones are not set. like vim.o

madskjeldgaard commented 2 years ago

Side note @davidgranstrom but perhaps we should include package manager in the bug report template? It seems to make a big difference in behaviour sometimes whether it is installed using vim plug or packer

davidgranstrom commented 2 years ago

I am on 0.5 as well.

Kenneth Flak found out the solution:

Add

setlocal shiftwidth=4
setlocal tabstop=4
setlocal expandtab

to <your_nvim_config_dir>/after/ftplugin/supercollider.vim

Perhaps this should be done by scnvim?

This was actually set by scnvim in an earlier version, but I have to agree with the outcome of this discussion that its better to let the user set this.

There is mention on how to set this is up in this wiki page but maybe this could be enhanced to provide a more literal example on how to configure this?

davidgranstrom commented 2 years ago

Side note @davidgranstrom but perhaps we should include package manager in the bug report template? It seems to make a big difference in behaviour sometimes whether it is installed using vim plug or packer

Sounds like a good idea! I'll add a separate issue for this.

salkin-mada commented 2 years ago

Im getting expected behavior with .sc class files with this (using packer.nvim)

            run = ":call scnvim#install()",
            config = function ()
                vim.g.scnvim_snippet_format = "luasnip"
                vim.g.scnvim etc etc etc. settings.
                ....
                ...
                vim.cmd "autocmd FileType supercollider setlocal tabstop=4 softtabstop=4 shiftwidth=4"
            end,

as long as the filetype is detected as supercollider. of course.

madskjeldgaard commented 2 years ago

Just ran nvim with --startuptime flag and found this. Here you can see the scala files being sourced before scnvim:

437.028  001.039  001.039: sourcing /usr/share/nvim/runtime/syntax/scala.vim
439.122  000.038  000.038: sourcing /usr/share/nvim/runtime/ftplugin/scala.vim
442.222  000.017  000.017: sourcing /usr/share/nvim/runtime/indent/scala.vim
454.272  003.716  003.716: sourcing /home/mads/.local/share/nvim/site/pack/packer/opt/scnvim/syntax/classes.vim
454.896  004.384  000.668: sourcing /home/mads/.local/share/nvim/site/pack/packer/opt/scnvim/syntax/supercollider.vim
456.725  000.025  000.025: sourcing /home/mads/.local/share/nvim/site/pack/packer/opt/scnvim/ftplugin/supercollider/commands.vim
456.770  000.028  000.028: sourcing /home/mads/.local/share/nvim/site/pack/packer/opt/scnvim/ftplugin/supercollider/mappings.vim
456.973  000.188  000.188: sourcing /home/mads/.local/share/nvim/site/pack/packer/opt/scnvim/ftplugin/supercollider/supercollider.vim
458.053  000.010  000.010: sourcing /home/mads/.config/nvim/after/ftplugin/supercollider.vim
460.793  000.017  000.017: sourcing /home/mads/.local/share/nvim/site/pack/packer/opt/scnvim/indent/supercollider.vim
465.565  003.238  003.238: sourcing /home/mads/.local/share/nvim/site/pack/packer/opt/scnvim/syntax/classes.vim
466.187  003.902  000.665: sourcing /home/mads/.local/share/nvim/site/pack/packer/opt/scnvim/syntax/supercollider.vim
476.816  003.269  003.269: sourcing /home/mads/.local/share/nvim/site/pack/packer/opt/scnvim/syntax/classes.vim
477.435  003.930  000.662: sourcing /home/mads/.local/share/nvim/site/pack/packer/opt/scnvim/syntax/supercollider.vim
479.250  000.010  000.010: sourcing /home/mads/.local/share/nvim/site/pack/packer/opt/scnvim/ftplugin/supercollider/commands.vim
479.273  000.008  000.008: sourcing /home/mads/.local/share/nvim/site/pack/packer/opt/scnvim/ftplugin/supercollider/mappings.vim
479.294  000.007  000.007: sourcing /home/mads/.local/share/nvim/site/pack/packer/opt/scnvim/ftplugin/supercollider/supercollider.vim
480.313  000.009  000.009: sourcing /home/mads/.config/nvim/after/ftplugin/supercollider.vim
482.950  000.008  000.008: sourcing /home/mads/.local/share/nvim/site/pack/packer/opt/scnvim/indent/supercollider.vim
487.669  003.263  003.263: sourcing /home/mads/.local/share/nvim/site/pack/packer/opt/scnvim/syntax/classes.vim
488.281  003.916  000.654: sourcing /home/mads/.local/share/nvim/site/pack/packer/opt/scnvim/syntax/supercollider.vim
4
davidgranstrom commented 2 years ago

@madskjeldgaard I have similar output as well, I guess that is to be expected since filetype.vim comes first in the runtime order. But for me the ftdetection for supercollider kicks in as expected. I see that you are lazy loading scnvim (the files are sourced from opt rather than start), I'll have to see if this changes the behaviour for me as well.

davidgranstrom commented 2 years ago

@eirikblekesaune @madskjeldgaard @salkin-mada

If you have time to test I would be interested in seeing the result of this command:

nvim Test.sc -c "verbose set filetype? indentexpr?"

For me it is:

  filetype=supercollider
        Last set from ~/code/vim/scnvim/ftdetect/supercollider.vim line 2
  indentexpr=GetSCIndent()
        Last set from ~/code/vim/scnvim/indent/supercollider.vim line 12
eirikblekesaune commented 2 years ago

If you have time to test I would be interested in seeing the result of this command:

nvim Test.sc -c "verbose set filetype? indentexpr?"

I get the same result here. I have tested with and without a supercollider.vim file at the path: ~/.config/nvim/after/ftplugin/supercollider.vim with this content:

setlocal shiftwidth=4
setlocal tabstop=4
setlocal noexpandtab

The only difference is that the indentation follows scala without the ~/.config/nvim/after/ftplugin/supercollider.vim-file.

When I start nvim with this command:

nvim Test.sc -c "verbose set filetype? indentexpr? shiftwidth?"

I get this without the ~/.config/nvim/after/ftplugin/supercollider.vim-file.`:

filetype=supercollider                                                                                                                                       
        Last set from ~/.local/share/nvim/plugged/scnvim/ftdetect/supercollider.vim line 2                                                                     
  indentexpr=GetSCIndent()                                                                                                                                     
        Last set from ~/.local/share/nvim/plugged/scnvim/indent/supercollider.vim line 12                                                                      
  shiftwidth=2                                                                                                                                                 
        Last set from /usr/share/nvim/runtime/ftplugin/scala.vim line 27     

When I add the file again, I get:

filetype=supercollider                                                                                                                                       
        Last set from ~/.local/share/nvim/plugged/scnvim/ftdetect/supercollider.vim line 2                                                                     
  indentexpr=GetSCIndent()                                                                                                                                     
        Last set from ~/.local/share/nvim/plugged/scnvim/indent/supercollider.vim line 12                                                                      
  shiftwidth=4                                                                                                                                                 
        Last set from ~/git/eirikblekesaune/dotfiles/dots/config/nvim/after/ftplugin/supercollider.vim line 1

(My .config is of course symlinked to my dotfiles according to the last path here)

davidgranstrom commented 2 years ago

@eirikblekesaune Thanks, I think it makes sense now. One could maybe argue that sw shouldn't be set in ftplugin/scala.vim at all, but I guess its only a problem since we have this conflict with the file extension. Nvim recognizes the scala file first from filetype.vim (since it comes first in the runtime path) and then the supercollider filetype detection kicks in, but since we no longer set sw (et al) in scnvim it will use the last set value from scala instead.

One solution could be that we override all of the setlocal stuff that scale does in the corresponding scnvim files. I will have to take a closer look..

eirikblekesaune commented 2 years ago

@davidgranstrom One possible solution is to just override the *.sc mapping for the filetype detection by changing ftdetect/supercollider.vim to:

autocmd! BufEnter,BufWinEnter,BufNewFile,BufRead *.sc set filetype=supercollider
autocmd BufEnter,BufWinEnter,BufNewFile,BufRead *.scd set filetype=supercollider
autocmd BufEnter,BufWinEnter,BufNewFile,BufRead *.schelp set filetype=scdoc

Could that be a solution?

davidgranstrom commented 2 years ago

@eirikblekesaune Yes, that seems to work! When starting nvim with nvim Test.sc -c "verbose set" I no longer see the scala related options. If you want you could make a PR with your solution and we can try it out?

eirikblekesaune commented 2 years ago

Yes, sure. Please see my PR. Thanks for the help with this @davidgranstrom ! Good times.

daveriedstra commented 2 years ago

I think this bug might still be in the wild. Running nvim Test.sc -c "verbose set filetype? indentexpr?" gives

  filetype=scala
        Last set from /usr/share/nvim/runtime/autoload/dist/ft.vim line 785
  indentexpr=GetScalaIndent()
        Last set from /usr/share/nvim/runtime/indent/scala.vim line 14

but adding autocmd! BufEnter,BufWinEnter,BufNewFile,BufRead *.sc set filetype=supercollider to my .vimrc "fixes" things, ie forces the correct filetype:

  filetype=supercollider
        Last set from ~/.vimrc line 489
  indentexpr=nvim_treesitter#indent()
        Last set from Lua

.scd files work fine, polyglot is not installed (per #182), and I get the same results when using a .vimrc that does nothing but load scnvim.

davidgranstrom commented 2 years ago

@daveriedstra Thanks for the report, I can replicate it here as well. Filetype detection has moved from scnvim and is now handled by the nvim runtime filetype detection (https://github.com/davidgranstrom/scnvim/pull/180)

Not sure how to proceed, best would be to fix the filetype detection upstream, but maybe an intermediate solution will be needed in the until the next (Neovim) release?

cc: @ranjithshegde

ranjithshegde commented 2 years ago

This is not a bug. The PR I submitted to both vim and nvim by default sets *.scd and *.quark to supercollider.

*.sc is a different story. If a *.sc file has anything in the 1st 25 lines to do with supercollider, i.e class/method definition, args declaration, basically anything related to supercollider, then it detects *.sc as supercollider. Else scala.

So as in @daveriedstra's example, since an empty *.sc file has no supercollider contents, it sets to scala, but of course, if you enter even a single line of sc code and save it (:w), it will immediately switch it to supercollider.

Once again, this is only for *.sc. *.scd is supercollider

This was a compromise I had to settle for after discussion on nvim matrix channel with the devs, for resolving conflict with scala.

One option that was proposed, that I can still do, but is really hacky and unnecessary is declaring a let g:sc_is_scd=1 so that when that var is declared, sc will always be scd no matter what aucmds are set.

@davidgranstrom any thoughts? I personally am okay with the current state, but if people feel differently, lets address it

EDIT: There is an active PR on neovim to upstream many features of lspconfig including start_or_attach client. Natively recognizing supercollider would mean being able to directly attach the lsp (ongoing work at LSP-quark and stdio ) on sc filetype. That was my main motivation in upstreaming the ftdetect

davidgranstrom commented 2 years ago

@ranjithshegde I see, and I understand that for files without content there's not much to do. I'm personally fine with this approach as well.

@daveriedstra Does it work for you after typing some content, saving the file and then call :e to reload the buffer?

We could maybe have an option in the new (lua) config that overrides *.sc file type detection to always use supercollider, that can be opt-in by default but for those working with both supercollider and scala could opt-out and use the runtime ftdetect? That would not require any upstream changes at least..

daveriedstra commented 2 years ago

Thanks for the clarification @ranjithshegde. Seems like a bit of a headache all around!

Does it work for you after typing some content, saving the file and then call :e to reload the buffer?

No, at least not with the following trivial test code:

s.boot;

(
  SynthDef(\testsynth, {
    args freq=220, amp=0.8;
    SinOsc.ar(freq) * amp
  }).add
)

(If that behaviour seems incorrect, I'm happy to file a bug in the right place.)

If a .sc file has anything in the 1st 25 lines to do with supercollider, i.e class/method definition, args declaration, basically anything related to supercollider, then it detects .sc as supercollider. Else scala.

I'm not fully convinced by this rubric, though if I was back and forth between supercollider and scala maybe I would be. I first noticed this issue editing a supercollider .sc file prefaced with >25 lines of comments, so it would have failed the rubric even if it was working properly on my system. More importantly, the live coding orientation of supercollider wants a workflow that treats the editor also like a REPL, so a blank document can be treated like a musical instrument and what's entered into it in a session may never be written to a file (and I think users expect this from the supercollider IDE's behaviour). Needing to save and reload a document gets in the way of this, as does not having the right syntax initially.

an option in the new (lua) config that overrides *.sc file type detection to always use supercollider

I like this idea and would use it. The autocmd I mentioned above is also doing it for me. Another solution might just be to explain the situation in scnvim's readme and suggest the autocmd or forcing the filetype with a keybind.

(As a side note, if there's some relevant distinction between *.sc and *.scd that I'm missing, that might warrant a mention in the readme as well. As far as I can tell, the two extensions are otherwise treated interchangeably.)

ranjithshegde commented 2 years ago
s.boot;

(
  SynthDef(\testsynth, {
    args freq=220, amp=0.8;
    SinOsc.ar(freq) * amp
  }).add
)

(If that behaviour seems incorrect, I'm happy to file a bug in the right place.)

Let me check this out and get back to you

I'm not fully convinced by this rubric, though if I was back and forth between supercollider and scala maybe I would be. I first noticed this issue editing a supercollider .sc file prefaced with >25 lines of comments, so it would have failed the rubric even if it was working properly on my system.

Comments are not parsed. 1st 25 non-empty, non-commented lines are counted

More importantly, the live coding orientation of supercollider wants a workflow that treats the editor also like a REPL, so a blank document can be treated like a musical instrument and what's entered into it in a session may never be written to a file (and I think users expect this from the supercollider IDE's behaviour). Needing to save and reload a document gets in the way of this, as does not having the right syntax initially.

Well, firstly, you dont need to redraw or reload :e for the detection to work, just saving does it. Besides, I would assume that REPL would be useful after the buffer has some code, not before?

an option in the new (lua) config that overrides *.sc file type detection to always use supercollider

I like this idea and would use it. The autocmd I mentioned above is also doing it for me. Another solution might just be to explain the situation in scnvim's readme and suggest the autocmd or forcing the filetype with a keybind.

There is an argument for this, but do keep in mind, upstream stuff are for usecases as general as possible, at which point you cant rule out that people can and will switch between scala and supercollider, even though I, or you, personally dont. So the solution proposed in the previous comment, i.e handling this only in the plugin but not upstream would be ideal

daveriedstra commented 2 years ago

Let me check this out and get back to you

Thanks

Besides, I would assume that REPL would be useful after the buffer has some code, not before?

Yes, but I'd still need to enter that code. The point is more that as a supercollider user, a) I want to start with a blank slate that understands the code I'm about to enter (via features from scnvim, treesitter, or LSP, which need the filetype to be set correctly) and b) I shouldn't need to write a file before I execute a statement in the buffer (via eg scnvim-send-line / scnvim-send-block / scnvim-send-selection).

There is an argument for this, but do keep in mind, upstream stuff are for usecases as general as possible, at which point you can't rule out that people can and will switch between scala and supercollider, even though I, or you, personally don't. So the solution proposed in the previous comment, i.e handling this only in the plugin but not upstream would be ideal

100% agree, I'm definitely thinking of opt-in solutions for plugin users.

davidgranstrom commented 2 years ago

@daveriedstra @ranjithshegde I've added a config option (opt-in by default) that treats .sc files as supercollider. (https://github.com/davidgranstrom/scnvim/pull/153/commits/7c62b2f02e7b9733640249243c9e360100a0735b)