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

Prefer lead whitespace character over trail whitespace character #11876

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

timgianitsos
Copy link

See description in commit message.

Lines that consist of only spaces treat the spaces
as trailing characters as opposed to leading
characters. This is technically an ambiguous
edgecase because the spaces are both leading AND
trailing. However, presenting the characters as
leading is preferred.
https://vi.stackexchange.com/q/39755/45006
@timgianitsos
Copy link
Author

@brammool
Copy link
Contributor

The diff mainly shows lines that have been indented differently. Please use a 'shiftwidth' value of four.
Once that is fixed we can better see what you are actually changing.
This also deserves a change in the help for 'listchars' and a test case.

@zeertzjq
Copy link
Member

Why is it preferred to present them as leading?

@chrisbra
Copy link
Member

https://vi.stackexchange.com/questions/39755/how-to-make-lead-override-trail-in-listchars

Ah, I guess my hint was useful :)

This diff here makes it more clear: https://github.com/vim/vim/pull/11876/files?diff=unified&w=1

It basically just switches the elseif conditions.

However, if we change this now, I am sure other people will complain :(
So this should be made configurable. But just adding yet another option for this minor change is also not nice...

So perhaps it makes sense to enhance the 'listchars' option to somehow allow to configure priorities for 'lead'/'trail'?

I vaguely remember this being discussed when the 'lead' or 'trail' option was added to 'listchars', but cannot find references.

@timgianitsos
Copy link
Author

Ah, I guess my hint was useful :)

Yup, thanks @chrisbra :)

This diff here makes it more clear: https://github.com/vim/vim/pull/11876/files?diff=unified&w=1

I also delineated into two separate commits to make it easier to view. The more important commit is here.

That's a cool trick with the w=1 query param @chrisbra. For future observers' reference, I found a source explaining this feature of Github to ignore whitespace discrepancies in diffs here.

Why is it preferred to present them as leading?

Whenever one starts typing at the end of an indented line, there is a sudden conversion of all whitespace characters from trailing form to leading form. IMO it would be preferable as default behavior to have them be leading form the whole time instead of this sudden conversion from trailing form to leading form.

What is lost by doing this? Well if we follow the convention of my pull request, then any scenario where you start typing at the beginning of a line that already has indentation will convert leading form into trailing form. But it is almost never the case that you start typing at the beginning of a line before the whitespace.

Having precedence between whitespace characters is not without... precedent :)
In the docs from :help listchars, multiple of the options "override" each other. There is no configurability, but the precedence/overriding behavior makes intuitive sense.

@zeertzjq
Copy link
Member

  1. I don't think this PR works at all. You didn't change line 1522.
  2. One may set "trail" in 'listchars' to a character that is easy to spot, so they can remove them. Hardly anyone wants to remove leading spaces.

@timgianitsos
Copy link
Author

timgianitsos commented Jan 25, 2023

  1. Maybe it is presumptive of me to assume that lead should override - since this precedence is recognized in more than one place (line 1522 as you said), was there originally a deeper reason why trail is preferred to lead?

Hardly anyone wants to remove leading spaces.

I agree! They are just not being recognized as leading spaces until you type another character after. While you think about what to type on an indented line, you have trailing characters staring at you that you are tempted to delete. Typing a dummy character after the whitespace is the only remedy.

@brammool
Copy link
Contributor

brammool commented Jan 25, 2023 via email

@k-takata
Copy link
Member

The commit e85b23b should be removed. The commit message says:

Fix lines with mixed tab and space to only use tabs

but Vim's source code intentionally uses mixed tab and space.

@codecov
Copy link

codecov bot commented Jan 25, 2023

Codecov Report

Merging #11876 (dd432ad) into master (62a6923) will increase coverage by 0.00%.
The diff coverage is 95.65%.

❗ Current head dd432ad differs from pull request most recent head ba41868. Consider uploading reports for the commit ba41868 to get more accurate results

@@           Coverage Diff           @@
##           master   #11876   +/-   ##
=======================================
  Coverage   81.88%   81.89%           
=======================================
  Files         164      164           
  Lines      193236   193236           
  Branches    43790    43790           
=======================================
+ Hits       158236   158251   +15     
+ Misses      22182    22170   -12     
+ Partials    12818    12815    -3     
Flag Coverage Δ
huge-clang-none 82.60% <95.65%> (+<0.01%) ⬆️
huge-gcc-none 53.72% <95.45%> (-0.01%) ⬇️
huge-gcc-testgui 52.19% <95.45%> (+<0.01%) ⬆️
huge-gcc-unittests 0.29% <0.00%> (ø)
linux 82.31% <95.65%> (+<0.01%) ⬆️
mingw-x64-HUGE 76.49% <94.73%> (+<0.01%) ⬆️
mingw-x86-HUGE 76.94% <94.73%> (+0.01%) ⬆️
windows 78.09% <94.73%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/drawline.c 84.08% <95.65%> (ø)
src/spellfile.c 80.36% <0.00%> (-0.14%) ⬇️
src/netbeans.c 72.10% <0.00%> (-0.08%) ⬇️
src/terminal.c 77.93% <0.00%> (ø)
src/gui_gtk_x11.c 51.73% <0.00%> (ø)
src/channel.c 83.19% <0.00%> (+0.04%) ⬆️
src/gui.c 72.72% <0.00%> (+0.04%) ⬆️
src/undo.c 75.83% <0.00%> (+0.06%) ⬆️
src/misc2.c 89.19% <0.00%> (+0.09%) ⬆️
src/os_win32.c 67.62% <0.00%> (+0.09%) ⬆️
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@timgianitsos
Copy link
Author

timgianitsos commented Jan 25, 2023

Vim's source code intentionally uses mixed tab and space

I reverted to using mixed tabs and spaces.

I am not sure how to address comments from @zeertzjq about the potential for the convention (i.e. trail overrides lead) to be assumed elsewhere in the code.

@brammool
Copy link
Contributor

I have doubts whether this is an improvement. For me both trailing spaces after text and a line with just spaces are characters that should not be there. Leading spaces are different. Therefore using "trail" for spaces in a line without text makes sense to me.

If some users don't like this, how about adding another character for spaces in a line without any text?
It could be called "empty" perhaps. That also makes it backwards compatible.

@yegappan yegappan added this to the backlog milestone Aug 13, 2023
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.

None yet

6 participants