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

Highlight current indent (no color) with a lower priority than scope (with color) #649

Open
Danielkonge opened this issue Sep 28, 2023 · 25 comments
Labels
enhancement New feature or request

Comments

@Danielkonge
Copy link
Contributor

Problem

I just updated to version 3 today and haven't yet played around with all the options, but I couldn't find something doing what I am asking for in the following. If it already exists, could someone let me know how to do it?

I really like the new highlighting via scope (especially in Rust). Even in Lua, I think this is helpful:
Screenshot 2023-09-28 at 23 34 33

So I plan to keep the highlighting on (I am matching it with rainbow-delimiters).

But whenever there is no scope highlighting I would like to fall back to slight highlighting of my current indentation level. I.e., in this picture:
Screenshot 2023-09-28 at 23 34 56

I would like to have some slight highlighting of the second indent line to the left of my cursor.

Preferably I would probably want to always have a slight highlighting of the current indent level unless that highlighting is overwritten by the scope highlighting. So sometimes the might be two highlighted indent lines at the same time. E.g., here:

Screenshot 2023-09-28 at 23 46 15

I would like to keep the first indent red, but have the second indent slightly light up to indicate the current indent level at the cursor position.

Is there already a way to do this? If not, I am willing to spend some time to try helping with implementation or testing of something like that. And if nothing else, I would like to request a feature like that.

Expected behavior

Always slightly highlight (e.g., just brighter color) the current indent level unless the current indent level is also the scope level in which case the scope is preferred for highlighting.

(Alternatively, when not using the current scope to highlight anything, highlight slightly the current indent level.)

@Danielkonge Danielkonge added the enhancement New feature or request label Sep 28, 2023
@lukas-reineke
Copy link
Owner

No, it doesn't exist. There is a similar discussion in #632 to give some context.
In short, this would be difficult to implement and maintain to the level of quality I want. I don't think I will work on this for now.

Of course if someone makes a great PR I am open to add this. If anyone wants to try, please discuss the implementation with me first, so you don't waste time on a suboptimal solution.

@Danielkonge
Copy link
Contributor Author

I am willing to try to implement this. How would you want something like this to be implemented?

I haven't used priority in my own setup, so I am not sure if it works exactly as I would expect, but I thought we would be able to use that to prioritize the highlighting of the current indentation level lower than the scope highlighting. Is that correct?

So I assume the main work is to get the highlighting of the current indentation level to work. I haven't looked much at the code yet, so I am sure I will find out that it is much harder than it looks quickly, but I thought we would have access to the indent lines drawn on the current cursor line and could relatively simply "catch" the inner most one and highlight that level of indentation. I assume some of the complexity comes from how the indentation blocks are made, but am I missing anything else?

Also, when I work on this, which files would I need to edit? (Where would you prefer this code to be?)

@lukas-reineke
Copy link
Owner

I thought we would have access to the indent lines drawn on the current cursor line and could relatively simply "catch" the inner most one and highlight that level of indentation.

Yeah it's not that easy. The code iterates only a single time over the lines for performance. So when you reach the cursor, all lines above are already rendered. You can't change the highlight anymore.

The scope works by walking the treesitter tree up from the cursor before looping over the lines, that is very fast.
Any solution for a "current" indent would need to do something like that.

I added wildcards for the scope in version 3.2.0. That gets pretty close to "current" indent, but it has a lot of edge cases.

@Danielkonge
Copy link
Contributor Author

I have looked at the code now and can see that it will be a bit more complicated than I thought. Would something like this work:

  • Make a new file similar to scope.lua, maybe context.lua? (Similar to the old name.) Or would you prefer to include the new code within scope.lua or somewhere else?
  • Add context_highlights to highlights.lua (again similar to what is done for scope).
  • Add what's needed to init.lua under the part making the virtual text.

(I know this is not really mentioning any exact implementation details, but I mostly just want to get a general picture to start with.)

@lukas-reineke
Copy link
Owner

yeah these details don't really matter. The main question is just how the algorithm should work.

@Danielkonge
Copy link
Contributor Author

Danielkonge commented Sep 30, 2023

I think what I want can almost be done with small changes in the scope code. I have uploaded a very rough draft here: #668

It is not at all finished yet, but I do get the behavior I want in most cases I have checked now.

I plan to work more on this tomorrow and maybe the day after, but I thought I would upload it for now, if you had any early comments about it.

@Danielkonge
Copy link
Contributor Author

Danielkonge commented Sep 30, 2023

Here are some comparison pictures.

When the scope is the inner context, we only highlight the scope:
Screenshot 2023-10-01 at 01 42 12

When there is no scope highlighting, we highlight the context:
Screenshot 2023-10-01 at 01 41 19

When the scope is further out than the context, we highlight both:
Screenshot 2023-10-01 at 01 40 32

And again with the scope being the same as the context:
Screenshot 2023-10-01 at 01 40 10

But it will (mistakenly) highlight what is a your current indentation level, even if it is not in the same context:
Screenshot 2023-10-01 at 01 49 30

Though it will be correct as soon as you go into the context:
Screenshot 2023-10-01 at 01 49 45

Screenshot 2023-10-01 at 01 50 00

I don't actually really mind the behavior above, but I don't think it is what most people expect/want.

Maybe if you are fine with it, we could keep a behavior like that and call it something like indent_level to avoid confusion with context.

@lukas-reineke
Copy link
Owner

call it something like indent_level to avoid confusion with context.

Yes, name should definitely be something like current_indent

@Danielkonge
Copy link
Contributor Author

Danielkonge commented Oct 1, 2023

I have renamed it to current_indent and rewritten a little bit now.

The algorithm is still almost an exact copy of scope that just basically counts almost everything as being a "scope". And then you have to for each language figure out how that language's tree sitter implementation represents blank lines and lists of commands, so that you can make sure not to highlight by mistake as above in these cases.

I think a lot of this could be moved into the implementation of scope if you would prefer that, but it makes sense to have these two almost identical fields, since I don't think you can use scope for multiple different things at once in a similar way without it?

I have tested on the following file types now:

  • bash (but only on simple files)
  • c (but only on simple files)
  • haskell (but only on simple files)
  • lua
  • python
  • rust

And most things works as I would want/expect.

I think one problematic thing about the way I am doing it now is that you have to maintain a file like: https://github.com/lukas-reineke/indent-blankline.nvim/blob/master/lua/ibl/scope_languages.lua

And from what I can tell that file was generated, while I don't see anything in neovim's tree sitter implementation that I could use to generate what I use in current_indent.

I haven't run into many extra edge cases after considering the "blank line" types mentioned above, but I have seen a few and I also haven't tested that much.

It is mostly pretty easy to debug. You just have to do something like :lua print( require("nvim-treesitter.ts_utils").get_node_at_cursor():type() ) and/or the tree sitter playground, where you think the highlighting is not as you want, and then you can edit the setup accordingly. But some things can't be easily fixed with what is available in tree sitter.

I agree that this is not at a level, where you would want to accept the draft pull request, but since it works mostly as I wanted now, I don't think I will do much more work on it unless I discover a much better way or you want me to change anything specific and work towards a proper pull request?

I will try to look more at tree sitter, but it doesn't seem to expose the things I would like to use to improve what I have for now.

(Also, I can close/delete the draft pull request if you prefer?)

@lukas-reineke
Copy link
Owner

I think one problematic thing about the way I am doing it now is that you have to maintain a file like

Yeah this is a dealbreaker. Maintenance effort would be way too high. And this would never be perfect.
I think treesitter is fundamentally not the right approach.
It is perfect for scope, but treesitter has no concept of the current indentation. I can format my code differently, and it all falls apart.

@simonhughxyz
Copy link

I would also like to see indentation level "context" be highlighted, I was not even aware that there was a problem with the old current context, it always worked fine for me.

Instead of using treesitter, why not just use old fashioned regex? Why does it have to be treesitter?
Could you not just look at the indent of your current line, and then look above and below your current line till you meet a line that does not match the indentation of your current line?

@lukas-reineke
Copy link
Owner

Could you not just look at the indent of your current line, and then look above and below your current line till you meet a line that does not match the indentation of your current line?

It has to be something like this, yes. But doing it separate from the main loop would kill performance.
The only real solution I see it to split the calculation and rendering into two loops. Then the calc loop can walk back to update previous lines. But this adds a lot of complexity. I am not convinced it is worth it.

@Danielkonge
Copy link
Contributor Author

I had some time today too, so I tried making a version without tree sitter where most of the code is completely separate from the tree sitter stuff, so if you disable `current_indent´ you shouldn't notice any difference in speed although this is slower than tree sitter:
#679
(I have a lot of free time this week...)

On my machine it works fine, but I don't know if that is true for most people. It is again not ready for pull request yet, but I just thought I would ask if you would be more willing to accept something like that? I am pretty sure there is a slow down when enabling it, but when it is disabled, everything should feel the same, and then it is up to the individual user, if they want to use it. I haven't found any edge cases so far, and I think it should basically always work (can't see what could go wrong if the indents are drawn correctly already).

@lukas-reineke
Copy link
Owner

I commented on the PR

I had a look at the code again. Let me explain what I think is the only reliable solution for this.

  1. When calculating the whitespace, you have to calculate into the future until you find the indent of the cursor, cache all the results, and then pop them off the stack on the next iterations. Similar to how blanklines work.

-- #### calculate indent ####
if not blankline then
whitespace_tbl, indent_state = indent.get(whitespace, indent_opts, indent_state)
elseif empty_line_counter > 0 then
empty_line_counter = empty_line_counter - 1
whitespace_tbl = next_whitespace_tbl
else
if i == #lines then
whitespace_tbl = {}
else
local j = i + 1
while j < #lines and lines[j]:len() == 0 do
j = j + 1
empty_line_counter = empty_line_counter + 1
end
local j_whitespace = utils.get_whitespace(lines[j])
whitespace_tbl, indent_state = indent.get(j_whitespace, indent_opts, indent_state)
if utils.has_end(lines[j]) then
local trail = last_whitespace_tbl[indent_state.stack[#indent_state.stack] + 1]
local trail_whitespace = last_whitespace_tbl[indent_state.stack[#indent_state.stack]]
if trail then
table.insert(whitespace_tbl, trail)
elseif trail_whitespace then
if indent.is_space_indent(trail_whitespace) then
table.insert(whitespace_tbl, indent.whitespace.INDENT)
else
table.insert(whitespace_tbl, indent.whitespace.TAB_START)
end
end
end
end
next_whitespace_tbl = whitespace_tbl
end

Basically, next_whitespace_tbl would need to become an array of whitespace_tbl.

  1. Once you know the cursor indent, it should be easy to duplicate the logic of scope

But this is very complicated. It has to work with all the other logic that is already there.
Even if you make a perfect PR I can't promise I'll merge it. If it is too complex, I might decide that the maintenance effort is not worth it.

@Danielkonge
Copy link
Contributor Author

Danielkonge commented Oct 3, 2023

I looked into doing what you said, but it was hard to get it to work with everything else.

What I ended up doing was split the for loop into two loops.

The first loop goes over each line and makes an array of whitespace_tbls and a corresponding array of each table's (i.e., each line's) furthest indent.

Then after the first loop I use the array of each line's furthest indent to calculate the current row's indent and find the start and end of the current indent level (this is easy and quick since I have an array with just numbers to look at for the indent levels).

Then the second loop goes over each line and makes the virtual text and sets up extmarks using the array of whitespace_tbls.

Realistically keeping the array of whitespace tables is not optimal, but I don't notice any slowdown when using this setup on my computer and I have everything working as I wanted now.

As mentioned in the previous pull request, I don't really expect this to be committed here, but I wanted something like this for my own editing and I am also just trying to get used to writing more complicated Lua code. I will probably just keep my fork that is slightly slower (I think, it doesn't feel slower), but does what I want for now, but if I figure out a smarter way to do this I will probably ask if you will accept a pull request.

(I don't know a good way to compare code without making a pull request, but here is the new init.lua (where most of the changes are) that I am using myself: https://github.com/Danielkonge/indent-blankline.nvim/blob/no-treesitter/lua/ibl/init.lua )

By the way, while I kept looking at this and asking about it, I just want to reiterate that I think the scope highlighting of v3 of ibl is really good! So even if you never add a feature like this yourself, I think it is worth the upgrade.

@lukas-reineke
Copy link
Owner

It's definitely the best attempt so far. But the 2 loop solution is not ideal. You now do duplicate work in both.

@Danielkonge
Copy link
Contributor Author

I changed it slightly by adding a boolean array keeping track of which is are skipped (since they already do what they need to in the first loop), but I don't think you can get around doing a little bit of extra work by doing two loops (unless you add even more arrays, but that doesn't seem like a good idea either, except for some computationally heavy stuff).

@Danielkonge
Copy link
Contributor Author

Danielkonge commented Oct 4, 2023

Actually, what I said is not entirely correct. With only adding a few arrays containing either booleans or numbers (i.e., arrays that should have no influence on the performance when we work with less than a thousand lines and do other more calculation heavy stuff), I got it so that I don't do any calculation twice (other than an integer addition since it seemed like overkill to save that in an array...).

I think now the only thing that might impact performance is keeping around the array of whitespace_tbls, since you could have a large number of indents. Though that array should realistically almost always be under 100 kB in size (you make the whitespace_tbl with numbers of 8 bytes each and most files will have an average indent of less than 50 spaces and need less than 200 lines highlighted).

@lukas-reineke
Copy link
Owner

Yeah, keeping the whitespace_tbl is fine.

You can improve the calculation for the current indent by having a stack that mirrors indent_state.stack, but with the line numbers. If you add a new indent to the indent_state stack, then you add the line number to the current_indent stack. You pop one of, you pop one of from the other as well.
That way, when you reach the cursor, you can just take the line number that's on top of the stack and that's your start line. And then finding the end is easy.
This way, you don't have to do any extra looping for the current indent. (it probably won't make a big difference, but might as well)

Overall, this solution is probably as good as it will get. Please make a PR.
Before I merge it, I'll test it properly and might refactor it a bit, but not sure when I have time for that.

Thanks for being persistent, I think there is a good chance we can get this merged.

@chaneyzorn
Copy link

chaneyzorn commented Oct 5, 2023

Hi, I've used both indent-blankline.nvim and mini.indent at same time. And so far everything looks fine.

Look at following screenshot :

  • dim indent line comes from indent-blankline.nvim;
  • solid scope line comes from indent-blankline.nvim, for treesitter scope;
  • dotted indent line comes from mini.indent, for current indentlevel; (with config.options.try_as_border enabled)

image

Just sharing, also expect the discuss of here.

@Danielkonge
Copy link
Contributor Author

You can improve the calculation for the current indent by having a stack that mirrors indent_state.stack, but with the line numbers. If you add a new indent to the indent_state stack, then you add the line number to the current_indent stack. You pop one of, you pop one of from the other as well. That way, when you reach the cursor, you can just take the line number that's on top of the stack and that's your start line. And then finding the end is easy. This way, you don't have to do any extra looping for the current indent. (it probably won't make a big difference, but might as well)

I have added a stack now, but I am not sure if you meant to add it in the indent_state? (That's not what I did.)

Overall, this solution is probably as good as it will get. Please make a PR. Before I merge it, I'll test it properly and might refactor it a bit, but not sure when I have time for that.

Thanks for being persistent, I think there is a good chance we can get this merged.

I have made a pull request now. If there is anything specific you would like me to edit, you can just let me know, and I can do some stuff at first before you go over it yourself.

@ZeChArtiahSaher

This comment has been minimized.

@Danielkonge

This comment has been minimized.

@ZeChArtiahSaher

This comment has been minimized.

@PerchunPak

This comment was marked as off-topic.

PerchunPak added a commit to PerchunPak/nixos-dotfiles that referenced this issue Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants