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

Cell highlighting #9

Merged
merged 10 commits into from
Oct 25, 2023
Merged

Conversation

cnrrobertson
Copy link
Contributor

Addresses #8.

A few design notes for discussion:

  • The highlighting is updated with all text changes and when entering a buffer. This may be a little heavier than needed and could just be changed to whenever saving?
  • The highlight config option is just the group but if you set it to nil there won't be any highlighting (saves having two config options)
  • The default highlight group I set as CursorLineNr because it usually gives vibrant visible text but a more subtle highlighting. Definitely could be another group though

@GCBallesteros
Copy link
Owner

GCBallesteros commented Sep 23, 2023

My current approach to this is to add a syntax rule on an autocmd.

api.nvim_create_autocmd(
  { "BufEnter", "BufRead", "BufNewFile" },
  { pattern = "*.py", command = [[syn match HYDROGEN /^\s*# %%.*$/]] }
)

HYDROGEN is just a highlight group I made. This only matches # %% but it could be very easily be extended for the general case. The advantages to this method are less code and no performance worries. The downside is that the syntax match is only against the # %% and the highlight doesn't cover the whole line.

I'm not convinced about running a regex against all the file every time text gets inserted, and doing it only on save feels a bit hacky and unconventional on how it behaves. I wonder if one can limit the search to just the visible buffer or have an autocmd that informs of where changes have happened so that the search for the cell marker can be limited to a small portion of the buffer. Another possibility I thought about yesterday is to use the conceal functionality to replace the text with a nicer cell marker. I've never played around with conceal but maybe it gives the best of both worlds? I will do some research on it.

Edit: No dice with conceal. It will only replace the text for one character. The documentation seems to be clear in that syntax highlighting will never overextend the match.

@cnrrobertson
Copy link
Contributor Author

Yeah, I completely agree with the computational overhead of the regex check at every text change. I have only been using BufEnter and BufWrite in my own config.

I do like the full line highlight though, which is why I use the extmarks.

It may not be a big deal, but I was just worried about incorrect highlights after cell markers are deleted or added. A possibility I've considered before would be to keep track of lines of cell markers so that edited lines could be quickly searched. As a procedure:

  1. Run a regex on BufRead to find cell locations
  2. On TextChanged or TextChangedI or maybe BufModifiedSet? check :changes to see if a cell location was removed or added
  3. Add/remove highlights accordingly

The benefit here is checking the changes list (which vim already keeps) really narrows down the regex search. However, this would mean having a cell location list for each buffer, which could be a bit hairy to keep track of. Let me know your thoughts and maybe I can try this approach.

@cnrrobertson
Copy link
Contributor Author

There is also a possible Neovim API approach using nvim_buf_attach to subscribe to nvim_buf_lines_event for when the cell marker lines change. I've never noticed this before, but it looks like the specific lines that are changed can be passed to a callback function -> we could just check those lines to see if they have a cell marker and highlight/de-highlight as needed. I may try this approach just to better understand these buffer attachments.

@cnrrobertson
Copy link
Contributor Author

Okay, I've updated the implementation to highlight the cells when the FileType is set and to only search for cell markers on the lines being edited but there remain a few cases that aren't handled:

  • When lines are added/removed, I havent found a good way of updating the marker locations (the locations are needed for when a marker is deleted while in insert mode)
  • When the line below a marker enters the marker line it grabs and persists the extmark

I think both can be addressed, but I wanted to give you a view of the updated regex only on edited lines.

@cnrrobertson
Copy link
Contributor Author

Alternatively, given that you already have some integration for mini.ai, instead of using my implementation and working through all the edge cases, we could use mini.hipatterns with a setup like:

require('mini.hipatterns').setup({
  highlighters = {
    notebook_cells = {
      pattern = function(buf_id)
        local buf_ft = vim.bo[buf_id].filetype
          if require"notebook-navigator".config.cell_markers[buf_ft] then
            local regex_cell_marker = string.gsub("^"..cell_marker, "%%", "%%%%")
            return regex_cell_marker
         else
           return nil
         end
      end,
      group = require"notebook-navigator".config.cell_highlight_group
    }
  }
})

So there would basically be a minihipatterns_spec for the notebook_cells table just like the miniai_spec.

The only downside that I've seen so far (for me) is that there isn't an option for highlighting the entire line. But mini.hipatterns does use extmarks so it may just be a small PR away to get that option exposed there.

@cnrrobertson
Copy link
Contributor Author

Full line highlighting is now possible in mini.hipatterns after echasnovski/mini.nvim@4e33725, so I'd recommend going with that as the best option (if you are okay with another optional mini dependency). I'll update the branch accordingly so you can take a look.

@GCBallesteros
Copy link
Owner

GCBallesteros commented Oct 24, 2023

Sounds good, I don't mind the extra dependency and it can always be made optional if someone really needs it by having a simply highlight rule as fallback.

Also, how? I will check how that was implemented.

Edit: How? By running on a timer and having a debounce period that will eat the TextChanged events that happen in close succession.

@GCBallesteros GCBallesteros merged commit 2f69d13 into GCBallesteros:main Oct 25, 2023
@cnrrobertson
Copy link
Contributor Author

Glad this made it in! Sorry for some of the extra cruft lying around after changing directions a couple of time (i.e. the unused all_cell_positions). I would have done a little refactoring before merging if I knew it was up to your spec.

Thanks again!

@GCBallesteros
Copy link
Owner

Thanks to you!

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