search

benlubas / molten-nvim

A neovim plugin for interactively running code with the jupyter kernel. Fork of magma-nvim with improvements in image rendering, performance, and more
GNU General Public License v3.0
608 stars 33 forks source link

fix: match lines added by image.nvim #201

Closed tszirr closed 1 month ago

tszirr commented 5 months ago

Bugfix

An insufficient number of lines was added to the output buffer for images to place their extmark and virtual lines. This commit always adds one space character and one newline for each image, so that the extmark line sits in front of the image, which is fully placed into virtual lines by image.nvim.

I also modified the lineno to start at 1 in order to account for the leading status line, it would be good to check if this makes sense to you, otherwise I could not make sense of the +1 specifically for images.

benlubas commented 5 months ago

What is the bug? Could you maybe include images? I'm not sure I understand. Is it #197 or something else?

b/c 197 requires a fix in image.nvim as well. I have a fix for 197 ready to go, but without the matching fix in image.nvim, it would make the experience in this plugin strictly worse.

tszirr commented 5 months ago

The bug is that no actual line is currently added for images, which results in the lineno passed to image.nvim to point to other output behind it or invalid lines. You can see that with invalid indices, no virtual lines were added before the fix e.g. here (note the ~): image

With the fix, the virtual lines are properly placed: image

(note you also need the other PR's fix though to actually see this with proper image refereshes)

tszirr commented 5 months ago

(note that I did not change the values of linenos passed image.nvim, I just moved the offset outside to be more consistent with where the output status line is added at the top)

benlubas commented 5 months ago

I see now, thank you. I think this is just a fix for a side effect of #197, and doesn't actually fix #197. I'd rather address 197 directly, I will put up the branch with my fix, and you can test that it also solves this problem.

benlubas commented 5 months ago

here's the branch with the fix (very simple fix) https://github.com/benlubas/molten-nvim/tree/push-otzqzrqwlkzu

I should note that this fix exposes molten to an image.nvim bug that's existed for a long time now, which is why I haven't merged this yet

tszirr commented 5 months ago

Unfortunately just adding -1 does not seem like an effective fix, it just breaks for me in practice: image

In contrast, with the changes in this PR, the necessary lines to match image.nvim are added and the images show up: image