Reflow doesn't work consistently with comments

[bcspragu]

Summary

Most of the time, when I select a comment (e.g. with x) and run :reflow, the comment prefix (e.g. //) isn't added to the reflowed lines. It seems to happen most when reflowing a single line into multiple lines.

Reproduction Steps

I tried creating a basic Go file, the LSP functionality is working correctly in this file (in case that matters).

After selecting the comment and running :reflow, I expected this to happen:

Instead, this happened (ignore the difference in highlighting)

Helix log

~/.cache/helix/helix.log

2022-08-04T12:02:40.098 helix_view::clipboard [INFO] Using xclip to interact with the system and selection (primary) clipboard
2022-08-04T12:02:40.104 helix_lsp::transport [INFO] -> {"jsonrpc":"2.0","method":"initialize","params":{"capabilities":{"textDocument":{"codeAction":{"codeActionLiteralSupport":{"codeActionKind":{"valueSet":["","quickfix","refactor","refactor.extract","refactor.inline","refactor.rewrite","source","source.organizeImports"]}}},"completion":{"completionItem":{"resolveSupport":{"properties":["documentation","detail","additionalTextEdits"]},"snippetSupport":false},"completionItemKind":{}},"hover":{"contentFormat":["markdown"]},"publishDiagnostics":{},"rename":{"dynamicRegistration":false,"honorsChangeAnnotations":false,"prepareSupport":false},"signatureHelp":{"signatureInformation":{"activeParameterSupport":true,"documentationFormat":["markdown"],"parameterInformation":{"labelOffsetSupport":true}}}},"window":{"workDoneProgress":true},"workspace":{"applyEdit":true,"configuration":true,"didChangeConfiguration":{"dynamicRegistration":false},"workspaceFolders":true}},"processId":10781,"rootPath":"/home/$USER/test","rootUri":"file:///home/$USER/test","workspaceFolders":[{"name":"test","uri":"file:///home/$USER/test"}]},"id":0}
2022-08-04T12:02:40.172 helix_lsp::transport [INFO] <- {"jsonrpc":"2.0","result":{"capabilities":{"textDocumentSync":{"openClose":true,"change":2,"save":{}},"completionProvider":{"triggerCharacters":["."],"completionItem":{}},"hoverProvider":true,"signatureHelpProvider":{"triggerCharacters":["(",","]},"definitionProvider":true,"typeDefinitionProvider":true,"implementationProvider":true,"referencesProvider":true,"documentHighlightProvider":true,"documentSymbolProvider":true,"codeActionProvider":{"codeActionKinds":["quickfix","refactor.extract","refactor.rewrite","source.fixAll","source.organizeImports"]},"codeLensProvider":{},"documentLinkProvider":{},"workspaceSymbolProvider":true,"documentFormattingProvider":true,"documentOnTypeFormattingProvider":{"firstTriggerCharacter":""},"renameProvider":true,"foldingRangeProvider":true,"executeCommandProvider":{"commands":["gopls.add_dependency","gopls.add_import","gopls.apply_fix","gopls.check_upgrades","gopls.edit_go_directive","gopls.gc_details","gopls.generate","gopls.generate_gopls_mod","gopls.go_get_package","gopls.list_imports","gopls.list_known_packages","gopls.regenerate_cgo","gopls.remove_dependency","gopls.run_tests","gopls.run_vulncheck_exp","gopls.start_debugging","gopls.test","gopls.tidy","gopls.toggle_gc_details","gopls.update_go_sum","gopls.upgrade_dependency","gopls.vendor"]},"callHierarchyProvider":true,"workspace":{"workspaceFolders":{"supported":true,"changeNotifications":"workspace/didChangeWorkspaceFolders"}},"inlayHintProvider":{}},"serverInfo":{"name":"gopls","version":"{\"GoVersion\":\"go1.18.4\",\"Path\":\"golang.org/x/tools/gopls\",\"Main\":{\"Path\":\"golang.org/x/tools/gopls\",\"Version\":\"v0.9.1\",\"Sum\":\"h1:SigsTL4Hpv3a6b/a7oPCLRv5QUeSM6PZNdta1oKY4B0=\",\"Replace\":null},\"Deps\":[...removed...],\"Settings\":[{\"Key\":\"-compiler\",\"Value\":\"gc\"},{\"Key\":\"CGO_ENABLED\",\"Value\":\"1\"},{\"Key\":\"CGO_CFLAGS\",\"Value\":\"\"},{\"Key\":\"CGO_CPPFLAGS\",\"Value\":\"\"},{\"Key\":\"CGO_CXXFLAGS\",\"Value\":\"\"},{\"Key\":\"CGO_LDFLAGS\",\"Value\":\"\"},{\"Key\":\"GOARCH\",\"Value\":\"amd64\"},{\"Key\":\"GOOS\",\"Value\":\"linux\"},{\"Key\":\"GOAMD64\",\"Value\":\"v1\"}],\"Version\":\"v0.9.1\"}"}},"id":0}
2022-08-04T12:02:40.173 helix_lsp::transport [INFO] <- {"capabilities":{"callHierarchyProvider":true,"codeActionProvider":{"codeActionKinds":["quickfix","refactor.extract","refactor.rewrite","source.fixAll","source.organizeImports"]},"codeLensProvider":{},"completionProvider":{"completionItem":{},"triggerCharacters":["."]},"definitionProvider":true,"documentFormattingProvider":true,"documentHighlightProvider":true,"documentLinkProvider":{},"documentOnTypeFormattingProvider":{"firstTriggerCharacter":""},"documentSymbolProvider":true,"executeCommandProvider":{"commands":["gopls.add_dependency","gopls.add_import","gopls.apply_fix","gopls.check_upgrades","gopls.edit_go_directive","gopls.gc_details","gopls.generate","gopls.generate_gopls_mod","gopls.go_get_package","gopls.list_imports","gopls.list_known_packages","gopls.regenerate_cgo","gopls.remove_dependency","gopls.run_tests","gopls.run_vulncheck_exp","gopls.start_debugging","gopls.test","gopls.tidy","gopls.toggle_gc_details","gopls.update_go_sum","gopls.upgrade_dependency","gopls.vendor"]},"foldingRangeProvider":true,"hoverProvider":true,"implementationProvider":true,"inlayHintProvider":{},"referencesProvider":true,"renameProvider":true,"signatureHelpProvider":{"triggerCharacters":["(",","]},"textDocumentSync":{"change":2,"openClose":true,"save":{}},"typeDefinitionProvider":true,"workspace":{"workspaceFolders":{"changeNotifications":"workspace/didChangeWorkspaceFolders","supported":true}},"workspaceSymbolProvider":true},"serverInfo":{"name":"gopls","version":"{\"GoVersion\":\"go1.18.4\",\"Path\":\"golang.org/x/tools/gopls\",\"Main\":{\"Path\":\"golang.org/x/tools/gopls\",\"Version\":\"v0.9.1\",\"Sum\":\"h1:SigsTL4Hpv3a6b/a7oPCLRv5QUeSM6PZNdta1oKY4B0=\",\"Replace\":null},\"Deps\":[...removed...],\"Settings\":[{\"Key\":\"-compiler\",\"Value\":\"gc\"},{\"Key\":\"CGO_ENABLED\",\"Value\":\"1\"},{\"Key\":\"CGO_CFLAGS\",\"Value\":\"\"},{\"Key\":\"CGO_CPPFLAGS\",\"Value\":\"\"},{\"Key\":\"CGO_CXXFLAGS\",\"Value\":\"\"},{\"Key\":\"CGO_LDFLAGS\",\"Value\":\"\"},{\"Key\":\"GOARCH\",\"Value\":\"amd64\"},{\"Key\":\"GOOS\",\"Value\":\"linux\"},{\"Key\":\"GOAMD64\",\"Value\":\"v1\"}],\"Version\":\"v0.9.1\"}"}}
2022-08-04T12:02:40.173 helix_lsp::transport [INFO] -> {"jsonrpc":"2.0","method":"initialized","params":{}}
2022-08-04T12:02:40.173 helix_lsp::transport [INFO] -> {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"languageId":"go","text":"package main\n\nimport \"fmt\"\n\n// This is a long comment that will need to be wrapped once it is finished being written. Here is some more text and other things as well. And one more sentence so that this goes to three lines, for demonstration purposes.\nfunc main() {\n\tfmt.Println(\"Hello Helix!\")\n}\n","uri":"file:///home/$USER/test/main.go","version":0}}}
2022-08-04T12:02:40.173 helix_lsp::transport [INFO] <- {"jsonrpc":"2.0","method":"window/workDoneProgress/create","params":{"token":"5577006791947779410"},"id":1}
2022-08-04T12:02:40.174 helix_lsp::transport [INFO] -> {"jsonrpc":"2.0","result":null,"id":1}
2022-08-04T12:02:40.174 helix_lsp::transport [INFO] <- {"jsonrpc":"2.0","method":"$/progress","params":{"token":"5577006791947779410","value":{"kind":"begin","title":"Setting up workspace","message":"Loading packages..."}}}
2022-08-04T12:02:40.175 helix_lsp::transport [INFO] <- {"jsonrpc":"2.0","method":"workspace/configuration","params":{"items":[{"scopeUri":"file:///home/$USER/test","section":"gopls"}]},"id":2}
2022-08-04T12:02:40.175 helix_lsp::transport [INFO] -> {"jsonrpc":"2.0","result":[null],"id":2}
2022-08-04T12:02:40.188 helix_lsp::transport [INFO] <- {"jsonrpc":"2.0","method":"window/logMessage","params":{"type":3,"message":"2022/08/04 12:02:40 go env for /home/$USER/test\n(root /home/$USER/test)\n(go version go version go1.19 linux/amd64)\n(valid build configuration = true)\n(build flags: [])\nGOCACHE=/home/$USER/.cache/go-build\nGOMODCACHE=/home/$USER/.local/go/pkg/mod\nGOPROXY=https://athens.$USER.com\nGOWORK=\nGO111MODULE=\nGOROOT=/usr/lib/go\nGONOPROXY=\nGOMOD=/home/$USER/test/go.mod\nGOSUMDB=sum.golang.org\nGOPATH=/home/$USER/.local/go\nGONOSUMDB=\nGOPRIVATE=\nGOFLAGS=\nGOINSECURE=\n\n"}}
2022-08-04T12:02:40.188 helix_term::application [INFO] window/logMessage: LogMessageParams { typ: Info, message: "2022/08/04 12:02:40 go env for /home/$USER/test\n(root /home/$USER/test)\n(go version go version go1.19 linux/amd64)\n(valid build configuration = true)\n(build flags: [])\nGOCACHE=/home/$USER/.cache/go-build\nGOMODCACHE=/home/$USER/.local/go/pkg/mod\nGOPROXY=https://athens.$USER.com\nGOWORK=\nGO111MODULE=\nGOROOT=/usr/lib/go\nGONOPROXY=\nGOMOD=/home/$USER/test/go.mod\nGOSUMDB=sum.golang.org\nGOPATH=/home/$USER/.local/go\nGONOSUMDB=\nGOPRIVATE=\nGOFLAGS=\nGOINSECURE=\n\n" }
2022-08-04T12:02:40.273 helix_lsp::transport [INFO] <- {"jsonrpc":"2.0","method":"window/logMessage","params":{"type":3,"message":"2022/08/04 12:02:40 go/packages.Load #1\n\tsnapshot=0\n\tdirectory=/home/$USER/test\n\tquery=[builtin example.com/...]\n\tpackages=2\n"}}
2022-08-04T12:02:40.273 helix_term::application [INFO] window/logMessage: LogMessageParams { typ: Info, message: "2022/08/04 12:02:40 go/packages.Load #1\n\tsnapshot=0\n\tdirectory=/home/$USER/test\n\tquery=[builtin example.com/...]\n\tpackages=2\n" }
2022-08-04T12:02:40.274 helix_lsp::transport [INFO] <- {"jsonrpc":"2.0","method":"window/logMessage","params":{"type":3,"message":"2022/08/04 12:02:40 go/packages.Load #1: updating metadata for 40 packages\n"}}
2022-08-04T12:02:40.274 helix_term::application [INFO] window/logMessage: LogMessageParams { typ: Info, message: "2022/08/04 12:02:40 go/packages.Load #1: updating metadata for 40 packages\n" }
2022-08-04T12:02:40.297 helix_lsp::transport [INFO] <- {"jsonrpc":"2.0","method":"$/progress","params":{"token":"5577006791947779410","value":{"kind":"end","message":"Finished loading packages."}}}
2022-08-04T12:02:40.377 helix_lsp::transport [INFO] <- {"jsonrpc":"2.0","method":"window/logMessage","params":{"type":3,"message":"2022/08/04 12:02:40 falling back to safe trimming due to type errors: [/usr/lib/go/src/runtime/vdso_linux.go:53:38: invalid operation: division by zero /usr/lib/go/src/runtime/vdso_linux.go:54:38: invalid operation: division by zero] or still-missing identifiers: map[memRecordCycle:true pageBits:true]\n\tpackage=\"runtime\"\n"}}
2022-08-04T12:02:40.377 helix_term::application [INFO] window/logMessage: LogMessageParams { typ: Info, message: "2022/08/04 12:02:40 falling back to safe trimming due to type errors: [/usr/lib/go/src/runtime/vdso_linux.go:53:38: invalid operation: division by zero /usr/lib/go/src/runtime/vdso_linux.go:54:38: invalid operation: division by zero] or still-missing identifiers: map[memRecordCycle:true pageBits:true]\n\tpackage=\"runtime\"\n" }
2022-08-04T12:02:46.953 helix_lsp::transport [INFO] -> {"jsonrpc":"2.0","method":"textDocument/didChange","params":{"contentChanges":[{"range":{"end":{"character":0,"line":5},"start":{"character":0,"line":4}},"text":"// This is a long comment that will need to be wrapped once it is finished\nbeing written. Here is some more text and other things as well. And one more\nsentence so that this goes to three lines, for demonstration purposes.\n"}],"textDocument":{"uri":"file:///home/$USER/test/main.go","version":1}}}
2022-08-04T12:02:46.955 helix_lsp::transport [INFO] <- {"jsonrpc":"2.0","method":"textDocument/publishDiagnostics","params":{"uri":"file:///home/$USER/test/main.go","version":1,"diagnostics":[{"range":{"start":{"line":5,"character":0},"end":{"line":5,"character":0}},"severity":1,"source":"syntax","message":"expected declaration, found being"}]}}
2022-08-04T12:02:50.197 helix_lsp::transport [INFO] -> {"jsonrpc":"2.0","method":"shutdown","params":null,"id":1}
2022-08-04T12:02:50.197 helix_lsp::transport [INFO] <- {"jsonrpc":"2.0","result":null,"id":1}
2022-08-04T12:02:50.197 helix_lsp::transport [INFO] <- null
2022-08-04T12:02:50.197 helix_lsp::transport [INFO] <- {"jsonrpc":"2.0","method":"window/logMessage","params":{"type":3,"message":"2022/08/04 12:02:50 Shutdown session\n\tshutdown_session=1\n"}}
2022-08-04T12:02:50.198 helix_lsp::transport [INFO] -> {"jsonrpc":"2.0","method":"exit","params":null}
2022-08-04T12:02:50.198 helix_lsp::transport [ERROR] err: <- StreamClosed

Platform

Linux

Terminal Emulator

st 0.8.4

Helix Version

helix 22.05 (c5f8a835)

[the-mikedavis]

The current implementation of :reflow uses common prefixes between lines rather than knowing about a language's comment token(s). So this is expected behavior when reflowing single-line comments but shouldn't happen with multiple single-line comments (block comments are also not supported yet but that's another issue).

See also https://github.com/helix-editor/helix/pull/1937 which is a different feature but may be useful implementation-wise.

[bcspragu]

Thanks for the reference, that does indeed look useful. I basically just want to mimic Vim/NeoVim's behavior here, which I've rarely ever had to think about. Once #1937 lands, I can take a look into reusing that for :reflow and/or seeing if textwrap can be made to do the right thing here.

[vlmutolo]

This is a known limitation of the current version of reflow. I believe the plan is to work with the textwrap crate to allow specifying custom prefix strings. See this discussion for context.

[epbuennig]

I'd like to add that running :reflow on inner rust doc comments considered only // as the separator for me and inferred ! to be part of the text to reflow.

Running :reflow 100 here:

//! Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt
//! ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris
//! nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit
//! esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in
//! culpa qui officia deserunt mollit anim id est laborum.

produces the following result:

//! Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt !
//ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco
//laboris ! nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in
//voluptate velit ! esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat
//non proident, sunt in ! culpa qui officia deserunt mollit anim id est laborum.

[k12ish]

I'd like to add that :reflow should perhaps consider separators before newlines, as in languages where \ is used as an escape sequence:

"Lorem ipsum dolor sit amet, consectetur adipiscing elit. Phasellus sed finibus \
sem, luctus dapibus odio. Etiam a sodales quam. Suspendisse auctor, diam eget \
facilisis mattis, justo sem pellentesque quam, a bibendum purus ipsum at justo. \
Etiam odio risus, consectetur nec varius quis, semper in elit. Mauris convallis \
arcu ut elit facilisis, at pretium felis pellentesque. "

[vlmutolo]

I'd like to add that :reflow should perhaps consider separators before newlines

This is an excellent point. I hadn't considered this at all. I think the general plan for making prefix recognition more robust was to work with the textwrap library to expose an API that lets the caller specify what to consider a prefix. I think we could (probably?) allow configuration of the suffix as well.

[bonsairobo]

Is there any issue in textwrap that covers the Helix use cases mentioned here? I didn't see anything that jumped out at me. I could try pushing the ball a little further, since I consider the current :reflow functionality a time-sink for editing Rust.

[bonsairobo]

Hello again. I've had some discussion with @mgeisler in this textwrap issue to make some progress. Before we go further, I'd appreciate if a Helix maintainer could read through that issue and give a :+1: to our general path of implementation.

[the-mikedavis]

I talked about this a bit with @pascalkuthe and in the long run I think we want to transition to having custom hard-wrapping code in helix-core rather than using the textwrap crate. There's a good chance we'll be able to re-use some of what we already have for soft-wrap and we could tailor the code to the features we need and info we have available like knowing about syntax via tree-sitter and about comment tokens once #4718 is merged.

[mgeisler]

I talked about this a bit with @pascalkuthe and in the long run I think we want to transition to having custom hard-wrapping code in helix-core rather than using the textwrap crate.

I would be interested to hear if this is because the simple wrapping logic is simple enough that it can just be re-implemented?

Personally, I would love to see a real editor with support for balanced wrapping :slightly_smiling_face: but that's of course only if you find the feature interesting in the first place. I used LaTeX a ton at my university days, so that is why I'm fascinated by the idea of optimizing wrapping across an entire paragraph. I think it would be quite a unique selling point.

If that feature is uninteresting, then I agree that it's easy to implement normal greedy-wrapping by hand. I implemented a quick-and-dirty wrapping function here so I could compare the binary sizes of programs with and without Textwrap.

[pascalkuthe]

We

I talked about this a bit with @pascalkuthe and in the long run I think we want to transition to having custom hard-wrapping code in helix-core rather than using the textwrap crate.

I would be interested to hear if this is because the simple wrapping logic is simple enough that it can just be re-implemented?

Personally, I would love to see a real editor with support for balanced wrapping 🙂 but that's of course only if you find the feature interesting in the first place. I used LaTeX a ton at my university days, so that is why I'm fascinated by the idea of optimizing wrapping across an entire paragraph. I think it would be quite a unique selling point.

If that feature is uninteresting, then I agree that it's easy to implement normal greedy-wrapping by hand. I implemented a quick-and-dirty wrapping function here so I could compare the binary sizes of programs with and without Textwrap.

Both contigous hardwrap and softwrapping can not support optimal wrapping (they must be local per defintion) so our softwrap implementation already has it's own wrapping implementation. To cut down on compile time, code size and maintainancr effort it would be ideal to only have one wrapping implementation.

I also think it's just kind of odd/inconsistent if softwrap/contiguous hardwrap lead to different results compared to calling the hardwrap command.

I also don't really think optimal wrapping makes too much sense for a plaintext editor. Where I think it makes sense is richtext editors or a latex compiler that render to a PDF. I write a ton of LaTeX too but I never really care how my plaintext is wrapped. The wrapping is down by the Compiler later. Same for markdown and other formats. For programming languages textwrap doesn't really work well land something TS based would likely be much better). Linear wrapping is also used by all other comparable editors.

So yeah I think linear wrapping (at word boundaries) is good enough for us. Much more important then optimal wrapping is integration with the rest of the editor like comment continuation, wrapping a tree sitter, using non-contigous text as input (for efficency), properly deltified transactions, ...

All of these things are vastly simpler to implement (and more importantly maintain) if they are implemented in tree using a single wrapping implementation.

[bonsairobo]

@the-mikedavis @pascalkuthe Thanks for taking a look. I agree that it makes sense to use the same implementation for softwrap and hardwrap. Let me know if you need any help.

[mgeisler]

Much more important then optimal wrapping is integration with the rest of the editor like comment continuation, wrapping a tree sitter, using non-contigous text as input (for efficency), properly deltified transactions, ...

Yes, I totally understand this reasoning!

[bonsairobo]

@pascalkuthe Is this what you are calling "contiguous hard wrap"?

https://github.com/helix-editor/helix/pull/8664

[the-mikedavis]

That's https://github.com/helix-editor/helix/issues/2274

[bonsairobo]

@the-mikedavis I see. So the only difference between #2274 and #1730 is the former deals with all text and the latter deals only with comments (i.e. detecting that the comment prefix should be added after wrapping)?

[the-mikedavis]

They have some overlap but I see them as separate features. #1730 is just for adding in indentation plus the comment token when you hit enter (or equivalent) if you're on a comment line. #2274 would remove the need for you to hit enter in insert mode. It would care about indentation too but is more about balancing the words across the lines automatically while #1730 shouldn't automatically change line contents other than indents and the comment-token