Skip to content

Conversation

@rhelder
Copy link
Contributor

@rhelder rhelder commented Jan 27, 2025

A Side Effect

Before VimtexIndent() returns the number of spaces by which the current line ought to be indented, it strips comments from the line using s:clean_line(). This is necessary for proper indentation on the basis of environments, delimiters, and so on, because we don't want indentation to be influenced by a commented-out \begin or \end command, a commented-out delimiter, or whatever else.

A side effect of the current implementation is that the conditional expression if l:line =~# '^\s*%' (on line 55) is never true. (For good measure, I tested this on csquotes.sty, which is 2499 lines long and contains a lot of commented lines. I put an echomsg within the conditional and used gg=G, and no message was printed.) Because comments are removed before that point (at line 47), for any line that begins with a %, l:line is empty. As a result, commented lines sometimes do not use the indentation of the previous line, which the comment at line 54 indicates is the intended behavior.

Affected Use Cases

Generally, this quirk in the code is not consequential. I had never noticed it until recently, and even then I thought it was the intended behavior, until I saw the comment on line 54. But one case where the current implementation can be a bummer (and where I first noticed it) is if you are editing a .dtx file. Suppose I were to start by inserting the following:

% Here begins the uninteresting documentation of the uninteresting macro
% \cs{foo}.
%    \begin{macrocode}
\NewDocumentCommand{\foo}{m}{%
%    \end{macrocode}

Suppose also I have formatoptions o and/or r set. If I have my cursor at the end of the last line of the above code, and I press o in normal mode or <Return> in insert mode, then the following occurs:

% Here begins the uninteresting documentation of the uninteresting macro
% \cs{foo}.
%    \begin{macrocode}
\NewDocumentCommand{\foo}{m}{%
%    \end{macrocode}
  %

This is initially an inconvenience because you have to manually realign the comment character with the start of the line. But, alas, the real bummer begins once you've swallowed the minor inconvenience and keep going. If ever after you press o in normal mode or <Return> in insert mode, the comment character will again be indented as above. Worse (or, at least, more unpredictable) is if you insert an indentkeys character like }. Suppose I've realigned the comment character in the above code and I continue editing like so:

% Here begins the uninteresting documentation of the uninteresting macro
% \cs{foo}.
%    \begin{macrocode}
\NewDocumentCommand{\foo}{m}{%
%    \end{macrocode}
%    Let me tell you something about the macro \cs{foo

As soon as you insert }, this happens:

% Here begins the uninteresting documentation of the uninteresting macro
% \cs{foo}.
%    \begin{macrocode}
\NewDocumentCommand{\foo}{m}{%
%    \end{macrocode}
  %    Let me tell you something about the macro \cs{foo}

(Note: for this example to work, you need to have returned to normal mode at some point. If, for example, you realign the comment character by pressing CTRL-D in insert mode, indentkeys respects your manual indentation. But if you do something more labor-intensive like using << to realign the comment character in normal mode, or you exit insert mode for some other reason after using CTRL-D, the above behavior will occur).

The cause is simply that, because the commented lines are considered to be empty, they are indented by one shiftwidth after the opening brace at the end of \NewDocumentCommand like normal, rather than just inheriting the indent of the previous line, as intended. Every time we press o in normal mode or <Return> in insert mode, or we insert an indentkeys character, the line will be (re)indented to one shiftwidth. And as long as the lines continued to be commented, the indent is never changed. So the current implementation does make it rather inconvenient to edit .dtx files.

It should be noted (at the risk of obsessively documenting the issue ...) that the bug affects indentation in verbatim environments as well, although I doubt anyone is at much risk of being inconvenienced by it. Suppose I were to start typing the following:

\begin{verbatim}
  % For some reason, starting with an indented comment

Again, if I have the o and/or r options turned on, and I press o in normal mode or <Return> in insert mode, the following occurs:

\begin{verbatim}
  % For some reason, starting with an indented comment
%

The cause is that the last line is considered to be a blank line, and therefore it receives the indentation of the 'previous' line, which in a verbatim environment is considered to be the line that begins the environment. In the code above, that indentation is zero.

Usually, a new line in a verbatim environment is given the indent of the (actual) previous line after o in normal mode or <Return> in insert mode, so this behavior is unique to commented lines. Because there is no distinction between comments and code in a verbatim environment, I figured that there should be no difference in how they are indented in a verbatim environment, so this PR 'fixes' (at least according to that reasoning) indentation in verbatim environments as well.

Suggested Fix

Despite the verbosity of the foregoing discussion (sorry), my suggested fix is simple. We can start by setting l:line to getline(a:lnum), and then set l:line to s:clean_line(l:line) after we've checked for verbatim environments and commented lines. I think this is organized and readable, and it guarantees that everything other than verbatim environments and commented lines will be indented in the expected way.

Because this PR affects how comments are indented, it causes the tikz indentation test to fail. If you want to accept the PR, we would have to alter the reference file -- but I haven't touched that in this PR because I don't know what your workflow is for altering tests. If you end up wanting to approve the PR, let me know how you'd like me to proceed.

(Also, novice contributor note: I reasoned that, if I had a fix in mind, it would be more helpful to suggest the fix rather than opening a bug report and saying 'fix this please'. If I got it wrong, let me know!)

Thanks for your work, as always, and I hope this is helpful!

@lervag
Copy link
Owner

lervag commented Jan 28, 2025

Thanks for the in-depth description and suggested fix!

(Also, novice contributor note: I reasoned that, if I had a fix in mind, it would be more helpful to suggest the fix rather than opening a bug report and saying 'fix this please'. If I got it wrong, let me know!)

I don't mind having the issue and suggested fix raised in the same thread, so no worries! I think you reasoned perfectly here; although I won't claim that everyone else agrees on that 😅

Suggested Fix

Despite the verbosity of the foregoing discussion (sorry), my suggested fix is simple. We can start by setting l:line to getline(a:lnum), and then set l:line to s:clean_line(l:line) after we've checked for verbatim environments and commented lines. I think this is organized and readable, and it guarantees that everything other than verbatim environments and commented lines will be indented in the expected way.

It seems this is a breaking change. Some people may be customed to having comments being automatically indented. However, I'm inclined to agree that it is a desired behaviour.

Can you explain again why you want the comments to start at the beginning of the line? I mean, you already say this is relevant for .dtx files, but it is not immediately clear to my why (I don't have much experience with writing latex packages).

Thanks for your work, as always, and I hope this is helpful!

Glad for the kind words and thanks for contributing!

@lervag
Copy link
Owner

lervag commented Jan 28, 2025

A minor comment, since I already notice you are almost following my commit message convention - I don't hold contributors to this convention, so feel free to ignore me! I try to adhere to convention commits, and as such, I prefer to use lower case in the commit type - thus, instead of "Fix: indent ...", I would prefer "fix: indent ...".

If you were to rebase and fix that, then also feel free to add an exclamation mark to indicate breaking change, so "fix!: indent ...".

@rhelder
Copy link
Contributor Author

rhelder commented Jan 29, 2025

Can you explain again why you want the comments to start at the beginning of the line? I mean, you already say this is relevant for .dtx files, but it is not immediately clear to my why (I don't have much experience with writing latex packages).

If the comment doesn't align with the start of the line, it ends up in the .sty file generated from the .dtx file by docstrip (even if nothing follows the comment character! So if, in your .dtx file, you have a line consisting of a % indented by two spaces, you will have a weird, out-of-place % indented by two spaces in your .sty file).

A minor comment, since I already notice you are almost following my commit message convention - I don't hold contributors to this convention, so feel free to ignore me! I try to adhere to convention commits, and as such, I prefer to use lower case in the commit type - thus, instead of "Fix: indent ...", I would prefer "fix: indent ...".

If you were to rebase and fix that, then also feel free to add an exclamation mark to indicate breaking change, so "fix!: indent ...".

👍 Noted – I will save us all an extra step and follow this convention in future contributions.

So I will rebase accordingly, but before I do, I want to ask again about the failing test – would you like me to include a corrected version of the reference file in the new commit? Or would you like to deal with the test separately?

@lervag
Copy link
Owner

lervag commented Jan 29, 2025

Can you explain again why you want the comments to start at the beginning of the line? I mean, you already say this is relevant for .dtx files, but it is not immediately clear to my why (I don't have much experience with writing latex packages).

If the comment doesn't align with the start of the line, it ends up in the .sty file generated from the .dtx file by docstrip (even if nothing follows the comment character! So if, in your .dtx file, you have a line consisting of a % indented by two spaces, you will have a weird, out-of-place % indented by two spaces in your .sty file).

Ok, makes sense.

Now, should we have a different behaviour for .dtx files vs. .tex files? I think perhaps not; I think the implementation would be annoyingly complicated and perhaps not worth it.

So I will rebase accordingly, but before I do, I want to ask again about the failing test – would you like me to include a corrected version of the reference file in the new commit? Or would you like to deal with the test separately?

Great; yes, please deal with the test. I agree that you can just update the reference file.

@rhelder
Copy link
Contributor Author

rhelder commented Jan 29, 2025

Now, should we have a different behaviour for .dtx files vs. .tex files? I think perhaps not; I think the implementation would be annoyingly complicated and perhaps not worth it.

I agree. I think the proposed implementation is going to be flexible and intuitive enough that it won't inconvenience most people. Because of the action of autoindent, comments will be automatically aligned with the previous line after e.g. o or <Return>, which should feel natural for most people while editing a .tex file. And, if someone prefers a different indentation for a given comment and indents it manually, because the indent of a comment is returned by indent(), the manual indentation will be respected even after using e.g. the = operator.

Great; yes, please deal with the test. I agree that you can just update the reference file.

Sounds good – I will shuffle things around and rebase shortly.

rhelder added a commit to rhelder/vimtex that referenced this pull request Jan 29, 2025
@rhelder
Copy link
Contributor Author

rhelder commented Jan 29, 2025

Ok, done!

@rhelder
Copy link
Contributor Author

rhelder commented Jan 29, 2025

Wait, sorry, I changed the input file, not the reference file. I will correct shortly.

rhelder added a commit to rhelder/vimtex that referenced this pull request Jan 29, 2025
@lervag
Copy link
Owner

lervag commented Jan 29, 2025

Great, thanks; I'll merge this when you've corrected the input -> reference-thing.

@rhelder
Copy link
Contributor Author

rhelder commented Jan 29, 2025

Well that was embarrassing – I had made that mistake on another branch several days ago, and I just remembered I had forgotten to fix it. But we should be good to go now!

@lervag
Copy link
Owner

lervag commented Jan 29, 2025

Great! Thanks again!

@lervag lervag merged commit 3a9f47c into lervag:master Jan 29, 2025
1 check passed
@rhelder rhelder deleted the indent branch January 29, 2025 17:30
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 this pull request may close these issues.

2 participants