-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Adding indenttabchar, indentspacechar and spacechar options #3760
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
Conversation
22c9b6f to
f2c18b2
Compare
f2c18b2 to
5acc8f4
Compare
5acc8f4 to
8b92972
Compare
runtime/help/options.md
Outdated
| default value: ` ` (space) | ||
|
|
||
| * `indentspacechar`: same as `indenttabchar` except for spaces that are at | ||
| locations that are divisible by `tabsize`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if tabsize doesn't match the actual indentation used in the given file (which happens more often than not)? If my detectindent plugin is used, it tries to detect the actual indentation width and adjusts tabsize accordingly, but this plugin is not even built-in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what the problem is here.
The user (or any plugins) can always change the tabsize and the indentspacechar will be displayed accordingly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that the default behavior is confusing. For example, the default value of tabsize is 4, the user edits a file with 2-space indentation and doesn't bother changing the tabsize value (because it doesn't matter to him), then the user sets this indentspacechar option to highlight indentation, but only half of the indentation is highlighted.
I have no better suggestions right now, so I'm fine with it.
runtime/help/options.md
Outdated
| setting available in many other text editors. The color of this character is | ||
| determined by the `indent-char` field in the current theme rather than the | ||
| default text color. | ||
| * `indenttabchar`: sets the indentation character for tabs. This will not be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is "indentation character for tabs"? AFAICS indenttabchar does exactly what indentchar currently does, i.e. simply displays all tabs, so why not call it just tabchar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess technically they are the same? Since the tab character is always (displayed) aligned to tabsize?
I can change it to tabsize, I named it this ways just (trying) to be consistent to what it was called previously.
|
Do we need to deprecate It feels that by adding that many new options we are creating even more mess and confusion. Why not just add BTW another request in #431 (comment) was to display other non-printable characters, which is quite reasonable, so we might add another option e.g. |
Repurposing a setting would be confusing since the user is not expecting it unless they read the documentation. I guess if we want to truly do what the name says, we can have a setting
Okay, I will add that as well. |
It would be "backward compatible" in a sense: tabs would be still highlighted. Spaces at the position of tabs would become highlighted as well, but if the user changed the default value of
Let's not overcomplicate without real need. |
Okay, I see. But if I am fine to do it either way tbh. Just a question. The reason I deprecated Another reason is what if the user actually wants space (the default) instead of
Okay. |
Its primary role is to simply specify which character to use to visualize tabs. It is not primarily about indentation. What do we gain by making its name more verbose?
It doesn't need to be hidden, it can (and probably should) be documented as well. |
I know, but that's what it was called. That's why I said maybe we could have 2 settings for tabs, 1 act like
To be less confusing. The previous behavior of
I would prefer the behavior to be mentioned in the name rather that requiring the user to read the documentation (although they should be to be fair). This is gonna be my last attempt to see if we can stick to the current proposed names. |
|
Simple question: do we want an option that allows the user to simply display all tabs (no matter at the beginning, at the end or in the middle of a line)? If the answer is yes, why not call this option If what we want instead (or in addition) is an option that only displays tabs at the beginning of lines, then yes, it would make sense to name it |
That's what it does currently, both
That's why I was asking maybe if I should have Actually, maybe it is better if I rename the new options to the following:
Do you think this is better? |
What's the point of being consistent with it if you are deprecating it?
I'm not suggesting to implement it, I'm saying that if we wanted to implement it, then
Ok, I've just noticed that you already have Question (not a rhetorical one): are you going to use |
Yeah, I am, so that I can tell any hidden tabs in space-indented files. |
For that purpose, wouldn't |
True. I should probably turn that on as well. But they will still be different values, since I use The main thing here is we are mixing the ability to see tab character and indented blocks into a single option ( If we reuse If we are reusing I think it is good to have a new version/breaking changes message anyway for future breaking changes if any. Also if we need to account for a special case where Or we can avoid all of this headache by just having 4 options, |
|
To be clear, when I suggested reusing
This is a UI change, not an API change. The user is gonna notice it anyway. We've done such "breaking" UI changes before. It is less breaking than #3335, for example.
Because with the "extended" definition of indentation character (see my confusion above), any tab is an indent char, but not any space is... Anyway, screw that, yes, it would be messy. Now I think it's better to let
I'm almost fine with that, but... Think of what would be the most useful and pleasant behavior for the user. The user wants to see "indentation columns", displayed with the On the other hand... I've recalled once again that it's not that simple, since So, right now I'm inclined to agree that having 4 options
It still doesn't answer what to do with I don't think silently removing it from the documentation (what your PR seems to be doing) is an option. It seems we should just keep it, and say in its documentation that it is just an alias for |
Agree, but while we are at it...the |
If we're going to keep adding more of these maybe grouping them together under one setting would be better for discoverability? Similar to what vim does with |
Hm, good idea. |
I will need an example in order to implement grouping for the options. I am not following how I did say this PR addresses the issues but doesn't mean I am solving all of them completely (for instance #431 (comment) is asking for displaying a hex code, that's not gonna be done in this PR). I am open to ideas but don't want to get feature-creeped here 😅 I will try to implement the ideas mentioned here if relatively straightforward, if not it's better to have a dedicated PR for those features. The main goal for this PR (to me) is to have configurable indent characters for both space and tab indented files. |
Good point. Originally I was mildly opposed to doing something similar to vim's |
Something like I'm not sure what would be the name of this generic option though. |
I see. In this case, I will need to store these options as global variables (since we don't want to do string parsing in every display call) instead of getting it from the settings map. |
Hah, you are sure it would be slower than the settings map lookup? Especially considering how many times we do the settings map lookup in every display call? :D We are extremely wasteful there. See #3227 and #3228 (and #2970 for the background) (Just in case, I'm not saying we should not store those parsed settings.) |
No because What you shown would be true if |
Hmm... this is why IIRC I was originally asking you if there are really use cases for having two separate "scope indicators" for tabs and spaces, rather than just one for both. You said you had use cases for that. So... instead of requiring to the user to "mimic scope indicator in other text editor" by setting both |
I see, that's pretty weird (does any other editor work like that?). I had incorrectly assumed
I like this idea of |
Yes, I still do. My point was (when deciding to have 2 indicators or not) the user can choose if they want to differentiate tab indent or space indent. My point I just gave (wrt
You mean the other way around? If But anyway, the behavior that
idk, the fact that the user told the editor if the file is tab indented or not and yet it can still show space indented character is weird to me since the "indent" part is explicitly said in the name. Anyway, we are spending time dicussing this which would probably barely come up. I am fine to drop the |
For me, I just want to be able to see VS Code is different because it can render a tab character on top of the scope line, we can't do that here since we can only render 1 character. |
cb54c9a to
a559fc4
Compare
What's weird about it? Space indentation in a tab-indented file it still an indentation, just not a "desired" one.
The user hardly told anything. The |
|
Yeah I dropped the |
runtime/help/options.md
Outdated
|
|
||
| * `showchars`: sets what characters to be shown to display various invisible | ||
| characters in the file. The characters shown will not be inserted into files. | ||
| (This option overrides the `indentchar` option if that exists) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both options always exist, it's not like indentchar may cease to exist, it just may change its value?
And this statement is not quite true: 1. it is just the tab key, not the showchars option in its entirety, that overrides indentchar. 2. if the tab key is present, it overrides indentchar always, regardless of what value the indentchar itself is set to. Right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reworded it.
internal/display/bufwindow.go
Outdated
| if indenttabchars != "" { | ||
| indentrunes = []rune(indenttabchars) | ||
| } else { | ||
| indentrunes = []rune(tabchars) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of having this else branch which duplicates the outer else branch below, we can just add indenttabchars != "" to the outer if condition?
Ditto for spacechars below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
internal/display/bufwindow.go
Outdated
| fg, _, _ := s.Decompose() | ||
| style = style.Foreground(fg) | ||
| case ' ': | ||
| if bloc.X%tabsize == 0 && bloc.X < leadingwsEnd { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simply checking bloc.X%tabsize == 0 is buggy, since bloc.X is not necessarily the visual position of the character in the line (since there may be tab characters in the line before this space character). Try it: put some space characters after some tab characters (before the first visible character in a line) -> result: ispace is displayed at wrong locations.
We should probably use util.StringWidth().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am using the line width (totalwidth) now, doing util.StringWidth() every time would be overkill.
internal/display/bufwindow.go
Outdated
| fg, _, _ := s.Decompose() | ||
| style = style.Foreground(fg) | ||
| } | ||
| var returnrune rune |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe drawrune instead (in line with the name used by the callers, and because the meaning of this variable is precisely: the rune to be drawn)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
a559fc4 to
e660625
Compare
| } | ||
| bloc.X = bslice | ||
|
|
||
| // returns the rune to be drawn, style of it and if the bg should be preserved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the point of this comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point of the comment is to describe the return values, otherwise you have no idea what it returns unless you read the rest of the impl/call sites.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what we do - look at the call sites to see how the function is actually used and what for. If we are not interested in the call sites, why would we be interested in the function itself?
If the problem is just that we don't see the names of the return values here in the function declaration, we could use named return values? Or the problem is that we don't want to use them since the line would become enormously long (which it already is)?
Anyway, I'm ok with keeping it as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But again, why are you doing that in this commit "Simplifying draw to be less nested" which is supposed to be just about simplifying draw() to be less nested? This comment should added in the first commit, where getRuneStyle() itself is added?
And same about renaming bgoverridable and returnrune - why do that in this commit, and why do that at all? If we agree they should be preservebg and drawrune, we should name them preservebg and drawrune from the beginning, in the first commit?
I genuinely don't get it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fine, just merged to the wrong commit, have moved it to the first one. Should be good now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, now looks good.
Except that, the commit message of the 2nd commit includes an outdated sentence?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, missed that. Have removed it.
internal/display/bufwindow.go
Outdated
| if !preservebg { | ||
| // syntax or hlsearch highlighting with non-default background takes precedence | ||
| // over cursor-line and color-column | ||
| preservebg = origBg != defBg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also just realized: this can be even simpler:
// syntax or hlsearch highlighting with non-default background takes precedence
// over cursor-line and color-column
if !preservebg && origBg != defBg {
preservebg = true
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated, both reads fine to me tbh.
internal/display/bufwindow.go
Outdated
|
|
||
| // returns the rune to be drawn, style of it and if the bg should be preserved | ||
| getRuneStyle := func(r rune, style tcell.Style, isplaceholder bool) (rune, tcell.Style, bool) { | ||
| getRuneStyle := func(r rune, style tcell.Style, drawoffset int, linex int, isplaceholder bool) (rune, tcell.Style, bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why you renamed showoffset to drawoffset?
Neither name seems quite intuitive to me, but at least showoffset somehow relates to showchars (and showchars is the only reason for the existence of this argument, right?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
|
|
||
| width := 0 | ||
|
|
||
| linex := totalwidth |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
visualx? (like in VLocFromLoc() for example)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't use visiualx because it's quite easy to get mixed up with vloc.X, which is different.
vloc.X resets on wrap but this doesn't, this is x (draw/visual?) position in a line.
runtime/help/options.md
Outdated
|
|
||
| Here are the list of keys: | ||
| - `space`: space characters | ||
| - `tab`: tab characters, this option overrides the `indentchar` option |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"this key"?
Or, cleaner, in a separate sentence: "If set, overrides the indentchar option."?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
f872c3b to
814322a
Compare
|
Hey @JoeKar I want to get this merged so would be nice if you can take a look at this PR and approve/merge it when you are free. (and also I can start the next PR of adding Thanks |
814322a to
1ef6459
Compare
This PR added the options of
indentspacecharandspacecharand movedindentchartoindenttabcharto avoid confusion.indenttabcharwill display the specified character when\tis encountered.indentspacecharwill display the specified character when' 'is encountered attabsizelocationsspacecharwill display the specified character when' 'is encountered, butindentspacechartakes precedence.Created this PR because I wanted the feature to view files with deeply nested blocks.
Gonna merge it to my own fork https://github.com/Neko-Box-Coder/micro-dev for whoever needs it while waiting for this to be merged.
This should address long-standing requests of #2539 , #2707 and #431
I am happy to revert the prompt message of telling user
indentcharis deprecated, if people/maintainer finds it controversial.