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

New feature: Highlighting whitespace errors #1897

Merged
merged 8 commits into from
Mar 14, 2024
Merged

Conversation

dmaluka
Copy link
Collaborator

@dmaluka dmaluka commented Oct 21, 2020

This PR implements highlighting of common whitespace problems (trailing whitespaces and tab/space errors), making it easy to spot such problems. It is somewhat similar to highlighting of whitespace errors by git diff.

Namely, added 2 new options hltaberrors and hltrailingws (both disabled by default).

  • hltaberrors highlights tabs used instead of spaces and vice versa. It uses the value of tabstospaces option as a criterion whether a tab or space character is an error or not.
    If tabstospaces is on, we probably expect that the file should contain no tab characters, so any tab character is highlighted as an error.
    If tabstospaces is off, we probably expect that the file uses indentation with tabs, so space characters in the initial indent part of lines are highlighted as errors.

  • hltrailingws highlights trailing whitespaces at the end of lines. Note that it behaves in a "smart" way. It doesn't highlight newly added (transient) trailing whitespaces that naturally occur while typing text. It would be annoying to see transient highlighting every time we enter a space at the end of a line while typing.
    So a newly added trailing whitespace starts being highlighted only after the cursor moves to another line. Thus the highlighting serves its purpose: it draws our attention to annoying sloppy forgotten trailing whitespaces.

I don't really know if such smart highlighting of trailingws is implemented in any of the existing editors. If it's not, micro can be the pioneer.

A screenshot with hltaberrors and hltrailingws working on a real-life example: micro's help (it does have some trailing spaces as well as some mixture of tab and space indentation):

image

@hhromic
Copy link
Contributor

hhromic commented Dec 11, 2020

I don't really know if such smart highlighting of trailingws is implemented in any of the existing editors. If it's not, micro can be the pioneer.

Nano does highlighting of trailing whitespace only. As I'm transitioning from Nano to Micro, your PR is godsend!

@dmaluka
Copy link
Collaborator Author

dmaluka commented Aug 30, 2021

@zyedidia what do you think about this?

@Justinzobel
Copy link

Justinzobel commented Jan 31, 2023

Ping. Are there any blockers to this being merged?

@jemus42
Copy link

jemus42 commented Oct 20, 2023

Just stumbled upon this and would welcome this addition. Is this still viable?

@dmaluka
Copy link
Collaborator Author

dmaluka commented Oct 20, 2023

I'm still using it all the time. If you feel like compiling micro yourself, you can also pull this PR and try and use it.

@zyedidia Any chance you review this PR?

@JoeKar
Copy link
Collaborator

JoeKar commented Nov 1, 2023

I would like to see this feature in the near future too, because it's a really good idea to add this in a generic way into the micro code base so it doesn't need to be considered by additional checks in the syntax files.

But just one question from my side:
Wouldn't it be more comfortable to call the option/flag hlwserrors instead of hltaberrors?
This would fit to hltrailingws more...I guess and leaves some space for extension e.g. highlighting some text with two spaces added.

@dmaluka
Copy link
Collaborator Author

dmaluka commented Nov 1, 2023

I consider trailing whitespace an error too, so tab errors are just a special case of whitespace errors.

Or are you suggesting to combine both options into a single option, which would be not boolean but allowing to specify what to highlight: trailing ws, tab errors, or both, or neither, and perhaps others types of ws errors in the future? Maybe it's a good idea. Or maybe it isn't worth it. (BTW, the hltrailingws option name is in line with rmtrailingws.)

@JoeKar
Copy link
Collaborator

JoeKar commented Nov 1, 2023

No, combining hltaberrors and hltrailingws wasn't the intention of my question...at least not in the first place.
In case we would combine it we most probably run in to the situation in which one would use two out of three or more possibilities/modes, which then could become complicated (by using a list of values, like in #2857), but it would reduce the amount of additional options.

So yes, that took an interesting direction.

@dustdfg
Copy link
Contributor

dustdfg commented Dec 12, 2023

I see here one weird thing. If we consider something as an error, we shouldn't reproduce that error but auto-indentation just copies previous line indentation so if it contained error, new line will contain the same error

@dmaluka
Copy link
Collaborator Author

dmaluka commented Dec 12, 2023

If we consider something as an error, we shouldn't reproduce that error but auto-indentation just copies previous line indentation

Shouldn't we? Consider such a code:

image

The for loop indentation is highlighted as an error because, for example, it is indented with spaces, while the rest of the file is indented with tabs. But, if we add more lines of code to this for loop, do we really want to indent those new lines with tabs, not spaces? If we do, the existing mess does not go away (the previously existing lines in the for loop are still indented with spaces), and we in fact create more mess, since we introduce an "inner" mess inside the for loop, which is now gonna be indented partially with spaces, partially with tabs.

So I think it's better to keep things simple and keep these 2 features (highlighting indentation errors and auto-indentation) orthogonal to each other, i.e. avoid introducing interdependence rules with unexpected consequences.

@dustdfg
Copy link
Contributor

dustdfg commented Dec 13, 2023

Ok I think you are right

@dmaluka
Copy link
Collaborator Author

dmaluka commented Mar 3, 2024

Rebased on top of the newest master (just to clean up the commit history, no functional changes).

@dustdfg
Copy link
Contributor

dustdfg commented Mar 4, 2024

When do you expect this to be merged into the main codebase? Additionally, how long has the community been waiting for this merge?

Community asks! One more reason to ping @zyedidia

@taconi
Copy link
Contributor

taconi commented Mar 4, 2024

These things have bothered me a lot in the micro repos, there are PR's like this one almost 4 years old where the "owner" of the editor doesn't show up in the discussions even when called while others the merge is done in less than a month.
This can discourage the community from contributing to PRs or plugins that are in the same state (that's why I forked the plugin-channel to put the "rejected" plugins).

I believe that in the beginning only one person used this editor, but that's not the case today.
Many people spend their time helping with issues/discussions and pr's or with the creation of plugins and in addition to being valued these people should be respected.
A simple "I don't want that in my editor" would be better than 4 years of silence.

I don't want to get into the merits of individual time spent reviewing and responding to each topic.
The point is that it's no longer up to the creator to decide what goes into the editor or not.
I think we could solve this by decentralizing this power, moving the repo to an org and adding admins and builders who are motivated to improve the editor.

I hope we can achieve this without having to fork the project, but if we do that's great too, it would be a plus for the community to know that it could be active and welcomed in this fork.

It reinforced the question that my friend deleted:

[...] how long has the community been waiting [...]?

And how much longer does he intend to wait?

@taconi
Copy link
Contributor

taconi commented Mar 4, 2024

I created a discussion to centralize this.

@JoeKar
Copy link
Collaborator

JoeKar commented Mar 12, 2024

@dmaluka:
I think the honor is yours now. 😉

@ the rest:
Be patient a few more days and I assume the we don't need to wait any longer.

dmaluka added 8 commits March 14, 2024 03:09
Added option `hltaberrors` which helps to spot sloppy whitespace errors
with tabs used instead of spaces or vice versa.

It uses the value of `tabstospaces` option as a criterion whether a
tab or space character is an error or not.
If `tabstospaces` is on, we probably expect that the file should contain
no tab characters, so any tab character is highlighted as an error.
If `tabstospaces` is off, we probably expect that the file uses
indentation with tabs, so space characters in the initial indent part
of lines are highlighted as errors.
Added option `hltrailingws` for highlighting trailing whitespaces
at the end of lines. Note that it behaves in a "smart" way.
It doesn't highlight newly added (transient) trailing whitespaces
that naturally occur while typing text. It would be annoying to
see transient highlighting every time we enter a space at the end
of a line while typing.
So a newly added trailing whitespace starts being highlighting
only after the cursor moves to another line. Thus the highlighting
serves its purpose: it draws our attention to annoying sloppy
forgotten trailing whitespaces.
Fix unwanted highlighting of whitespace in the new line when inserting
a newline after a bracket (when hltrailingws is on). To fix it, change
the order of operations: insert the new empty line after all other
things, to avoid moving the cursor between lines after that.
Handle the case when the cursor itself hasn't really moved to
another line, but its line number has changed due to insert
or remove of some lines above.
In this case, if the cursor is still at its new trailingws,
we should not reset NewTrailingWsY to -1 but update it to the
new line number.

A scenario exemplifying this issue:
Bind some key, e.g. Alt-r, to such a lua function:

function insertNewlineAbove(bp)
    bp.Buf:Insert(buffer.Loc(0, bp.Cursor.Y), "\n")
end

Then in a file containing these lines:

aaa
bbb
ccc

insert a space at the end of bbb line, and then press Alt-r.
bbb and ccc are moved one line down, but also the trailing space
after bbb becomes highlighted, which isn't what we expect.
This commit fixes that.
Improve user experience: if we are at a line with a new (i.e.
not highlighted yet) trailingws and we begin selecting text,
don't highlight the trailingws until we are done with selection,
even if we moved the cursor to another line while selecting.
@dmaluka dmaluka merged commit 7b718cb into zyedidia:master Mar 14, 2024
3 checks passed
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.

7 participants