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

Folding ranges should exclude the initial line #915

Closed
jwortmann opened this issue Aug 6, 2023 · 3 comments · Fixed by #918
Closed

Folding ranges should exclude the initial line #915

jwortmann opened this issue Aug 6, 2023 · 3 comments · Fixed by #918

Comments

@jwortmann
Copy link

While trying out the foldingRangeProvider from this server, I noticed that all of the returned folded ranges describe the entire region of the relevant environment/code section, including the very first line of that region. For example the \section{Section Name} or \begin{envname} is fully included in the folded range, and therefore will be folded too.

For example consider this document:

\documentclass{article}
\usepackage[utf8]{inputenc}
\usepackage[T1]{fontenc}
\usepackage{amsmath}


\begin{document}

\section{First Section}

Lorem ipsum dolor sit amet, consectetur adipisicing 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.

\begin{equation}
1 + 1 = 2
\end{equation}

\section{Second Section}

Lorem ipsum dolor sit amet, consectetur adipisicing 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.


\end{document}

The reported folding ranges from the server are:

[
    {
        "kind": "region",
        "endLine": 31,
        "startCharacter": 0,
        "startLine": 6,
        "endCharacter": 14
    },
    {
        "kind": "region",
        "endLine": 19,
        "startCharacter": 0,
        "startLine": 8,
        "endCharacter": 14
    },
    {
        "kind": "region",
        "endLine": 19,
        "startCharacter": 0,
        "startLine": 17,
        "endCharacter": 14
    },
    {
        "kind": "region",
        "endLine": 28,
        "startCharacter": 0,
        "startLine": 21,
        "endCharacter": 72
    }
]

Notice how the "startCharacter": 0 for each of the regions enforces the first line to be included entirely.

It would be better if the folding range starts after the initial line of the region, so that it is still visible even when folded.

For \begin/\end blocks, it could even fold as \begin{envname} ... \end{envname}, where ... is a fold marker (i.e. use "endCharacter": 0). And something else to consider would be whether a potential label{} following on the first line or directly after the first line should be included in the folded range or not.

fold.webm

Notice that I didn't see this behavior when trying with the TexLab extension in VS Code; my guess would be that VS Code simply ignores all startCharacter and endCharacter values (this assumption is also based on the observation that VS Code's built-in JSON language server doesn't report those optional values) - default behavior per LSP specs if startCharacter/endCharacter were missing should be to fallback to the end of the line (this would result in the first line not being included in the folded range, like the behavior observed in VS Code).

@jwortmann jwortmann changed the title Folding ranges should exclude the initial line of the folded range Folding ranges should exclude the initial line Aug 6, 2023
@jwortmann
Copy link
Author

jwortmann commented Aug 11, 2023

There probably exists a more elegant solution, but this could be a possible minimal fix:

--- texlab/crates/texlab/src/features/folding.rs
+++ texlab/crates/texlab/src/features/folding.rs
@@ -53,7 +53,7 @@
 fn create_range(range: Range) -> FoldingRange {
     FoldingRange {
         start_line: range.start.line,
-        start_character: Some(range.start.character),
+        start_character: None,
         end_line: range.end.line,
         end_character: Some(range.end.character),
         collapsed_text: None,

Disclaimer: untested, and I've never used Rust before.

Would a PR with this change be accepted? The folding ranges currently returned by this server make code folding basically unusable.

microsoft/language-server-protocol#1198 (comment) confirms that the entire folding range should be hidden when folded.


Additionally, the "region" kind which TexLab uses for all folding ranges are - as far as I understand - supposed to be used for #region preprocessor directives or specially formated "#region"-comments. The usage here might lead to confusions and/or undesired behavior, if, for example, an editor provides the ability to initially show all "region" ranges folded when a file is opened. I think it would be better if TexLab either defines its own kind(s), e.g. "section" for \chapter/\section/\subsection, etc., or alternatively just reports no kind at all.

pfoerster added a commit that referenced this issue Aug 12, 2023
Do not fold the entire structure but the contents inside.

See #915.
@pfoerster
Copy link
Member

Thanks for the detailed comment.

microsoft/language-server-protocol#1198 (comment) confirms that the entire folding range should be hidden when folded.

Yeah, we should limit the folding range to the contents of the structure being folded and not the entire range.

Would a PR with this change be accepted? The folding ranges currently returned by this server make code folding basically unusable.

One small problem could be if multiple environments are on the same line, then we would have overlapping ranges, which is not allowed. I have a PR here: #918.

if TexLab either defines its own kind(s), e.g. "section" for \chapter/\section/\subsection

Unfortunately, the server cannot define its own kinds, the client needs to announce the supported kinds as part of the initialize request.

or alternatively just reports no kind at all.

I agree, region does not really fit here.

pfoerster added a commit that referenced this issue Aug 12, 2023
Do not fold the entire structure but the contents inside.

See #915.
@jwortmann
Copy link
Author

Thank you for the answer and for the fix!

Unfortunately, the server cannot define its own kinds, the client needs to announce the supported kinds as part of the initialize request.

According to the description of the FoldingRangeClientCapabilities in the LSP specs, it should be fine if servers use kinds which are not announced by the client (more accurately, you could/should check whether foldingRangeKind.valueSet (with any values) in the client capabilities exists, and if this is the case you are free to define your own kinds). In fact, I see that Microsoft's JSON language server uses "array" and "object" kinds, even when the client doesn't explicitly announce support for those.

Otherwise it would be like a hen vs. egg problem, because it's unlikely that clients would declare support for custom kinds which are not used by any server.

        /**
	 * Specific options for the folding range kind.
	 *
	 * @since 3.17.0
	 */
	foldingRangeKind? : {
		/**
		 * The folding range kind values the client supports. When this
		 * property exists the client also guarantees that it will
		 * handle values outside its set gracefully and falls back
		 * to a default value when unknown.
		 */
		valueSet?: [FoldingRangeKind](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#foldingRangeKind)[];
	};

pfoerster added a commit that referenced this issue Aug 12, 2023
Do not fold the entire structure but the contents inside.

See #915.
pfoerster added a commit that referenced this issue Aug 14, 2023
Do not fold the entire structure but the contents inside.

See #915.
pfoerster added a commit that referenced this issue Aug 14, 2023
Do not fold the entire structure but the contents inside.

See #915.
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue Nov 15, 2023
## [5.11.0] - 2023-11-05

### Added

- Add `texlab.inlayHints.labelDefinitions` and `texlab.inlayHints.labelReferences` options ([#753](latex-lsp/texlab#753))
- Display inlay hints for label references by default ([#753](latex-lsp/texlab#753))

### Fixed

- Moving the build logs to the recycle bin will now clear the diagnostics ([texlab-vscode/#825](latex-lsp/texlab-vscode#825))
- Fix false positive when reporting syntax errors for BibTeX accents ([#945](latex-lsp/texlab#945))

## [5.10.1] - 2023-10-10

### Fixed

- Fix regression when renaming commands ([#936](latex-lsp/texlab#936))

## [5.10.0] - 2023-09-30

### Added

- Allow passing additional arguments to `ChkTeX` using `texlab.chktex.additionalArgs` ([#927](latex-lsp/texlab#927))

### Fixed

- Fix loading bibliographies from `kpathsea` search path ([#923](latex-lsp/texlab#923))
- Don't report duplicate results when using goto definition on includes ([#924](latex-lsp/texlab#924))
- Fix project detection when there exist files with the same name ([#923](latex-lsp/texlab#923))
- Do not report parse errors with `$` in paths ([#931](latex-lsp/texlab#931))

## [5.9.2] - 2023-08-14

### Fixed

- Don't crash when using comments inside `\include`-like commands ([#919](latex-lsp/texlab#919))
- Folding ranges include only the contents instead of the entire range of the structure.
  For example, the folding range of an environment will start after the `\begin` and stop before the `\end`
  ([#915](latex-lsp/texlab#915))

## [5.9.1] - 2023-08-11

### Fixed

- Improve performance when completing BibTeX entries ([#493](latex-lsp/texlab#493))
- Don't report unused entries for very large bibliographies
- Avoid redundant reparses after saving documents

## [5.9.0] - 2023-08-06

### Added

- Use bibliographies found in `BIBINPUTS` environment variable ([#493](latex-lsp/texlab#493))
- Add `texlab.build.pdfDirectory` setting ([#911](latex-lsp/texlab#911))

### Fixed

- Fix search path for aux files when using `\include` instead of `\input` ([#906](latex-lsp/texlab#906))

## [5.8.0] - 2023-07-30

### Added

- Report diagnostics for unused and undefined labels
- Report diagnostics for unused BibTeX entries and undefined citations
- Report diagnostics for duplicate BibTeX entries
- Report diagnostics for duplicate labels
- Add `texlab.build.auxDirectory` and `texlab.build.logDirectory` settings ([#906](latex-lsp/texlab#906))

### Deprecated

- Deprecate `texlab.auxDirectory` in favor of `texlab.build.auxDirectory`

### Fixed

- Fix parsing paths with `|` ([#568](latex-lsp/texlab#568))
- Fix parsing LaTeX identifiers with `=` ([#568](latex-lsp/texlab#568))
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

Successfully merging a pull request may close this issue.

2 participants