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

Add Win.linemap, which translates visual line numbers to file line numbers #1181

Merged
merged 1 commit into from
Apr 25, 2024

Conversation

dther
Copy link
Contributor

@dther dther commented Apr 23, 2024

Win.linemap contains a Lua array (1-initiated) with one element for every visual line. Index 1 corresponds to the window's topmost line, and index #linemap corresponds to the last line in the viewport. The value of each index corresponds to the the value in win->view->lines->...->lineno. Combined with PR #1180, linemap can be combined with output from, say, a compiler, to highlight line numbers of important lines whenever they appear in the viewport, without needing to calculate the exact location of the line.

For example:

-- translate line from file to line displayed in the view of win
-- returns an integer relative to the window if line is in view
-- returns nil otherwise
function get_visual_line(win, line)
	if not win or not win.linemap then
		return
	end

	local linemap = win.linemap
	for viewline=1,#linemap do
		if linemap[viewline] == line then
			return viewline
		end
	end
end

importantline = 200
vis.events.subscribe(vis.events.WIN_STATUS, function(win)
	visline = get_visual_line(win, importantline) 
	if visline then
		vis:info(importantline.." is visible on row "..visline)
	end
end)

Prior to this change, get_visual_line would be a very large Lua function that would need to calculate line breaks and account for multiple windows.

Copy link
Collaborator

@rnpnr rnpnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is overly complicated and unnecessary. win->view->topline->lineno gives you the on disk line number of the top line in the current view. Similarly for view->bottomline (though that might need to be checked with view->lastline). If you have that and you want the on screen line number for importantline its some very basic math:

function get_visual_line(win, line)
	...
	if ... then
		return line - win.viewport_lines.start
	end
	...
end

I'm sure you can fill in the rest.

Now we could have a reasonable discussion about changing the viewport member so that it was something like:

local line_range = win.viewport.lines
local byte_range = win.viewport.bytes

@dther
Copy link
Contributor Author

dther commented Apr 23, 2024

This is overly complicated and unnecessary. win->view->topline->lineno gives you the on disk line number of the top line in the current view. Similarly for view->bottomline (though that might need to be checked with view->lastline). If you have that and you want the on screen line number for importantline its some very basic math:

You're missing the case for long broken lines. Consider this example: You have a file open in a window that's only somewhat small: less than 100 columns, a bit more than 80. You want to highlight line 5. Before styling, it looks like this.
image

The difference in result is illustrated here. If we were to highlight the lines simply by using line - viewport_lines.start, the result would be the red marker on line 3. With my example from the first comment, the result is the green marker on line 5. Correcting the red marker without linemap would require taking into account wrapcolumn, win.height and breakat, effectively duplicating data which was already calculated in the process of drawing the view.
image

For completeness, here's the code I used to test this. I simulated viewport_lines.start by accessing the first value of linemap.

function get_visual_line(win, line)
	if not win or not win.linemap then
		return
	end

	local linemap = win.linemap
	for viewline=1,#linemap do
		if linemap[viewline] == line then
			return viewline
		end
	end
end

function get_visual_line2(win, line)
	if not win or not win.linemap then
		return
	end

	local linemap = win.linemap
	local viewport_start = linemap[1]
	return line - viewport_start
end

importantline = 5
vis.events.subscribe(vis.events.UI_DRAW, function()
	local win = vis.win
	if not win then
		return
	end

	win:style_define(win.STYLE_LEXER_MAX, "back:green")
	win:style_define(win.STYLE_LEXER_MAX-1, "back:red")

	local visline = get_visual_line(win, importantline) 
	if visline then
		-- subtract 1 because style_pos has (0, 0) origin
		win:style_pos(win.STYLE_LEXER_MAX, 0, visline - 1)
	end

	local visline2 = get_visual_line2(win, importantline)
	if visline2 then
		-- subtract 1 because style_pos has (0, 0) origin
		win:style_pos(win.STYLE_LEXER_MAX-1, 0, visline2 - 1)
	end
end)

@rnpnr
Copy link
Collaborator

rnpnr commented Apr 23, 2024

You're missing the case for long broken lines.

Yes you are right, but that doesn't really change what I'm suggesting. If you have the first on disk line number and the last on disk line number and the height of the window (in rows) it is trivial to determine if there are any long lines in view. Now there are two cases, the one I gave above, and the long line case. In the second case you will need to iterate over the lines in File.lines using the line indexes that you already have (viewport_lines.start to importantline) and add an extra n to your returned line number. Here n is determined from the line's length (free in lua) and the window width.

Why is this better? Because you still have the fast first case and the second case now requires a single loop instead of two and doesn't require you to construct some dynamic array that is only valid for a single operation.

@rnpnr
Copy link
Collaborator

rnpnr commented Apr 23, 2024

I should add that I wouldn't be that against your approach if Lines was stored as a structure of arrays (SOA) rather then a linked list of structures. The reason I don't suggest that is because it would require a fair amount of work to change it right now.

Edit: I'm also not saying we should do this. It would just simplify this particular case. I would have to actually read through the uses of Line to see if it makes sense in the context of the rest of vis.

@rnpnr
Copy link
Collaborator

rnpnr commented Apr 23, 2024

Sorry, I guess I forgot that the viewport height and width isn't visible from lua right now, only the height/width of windows. Personally I would be ok if those were exposed separately. If someone were to do the server client thing the window height/width would be deleted but the viewport height/width would still be useful.

@dther
Copy link
Contributor Author

dther commented Apr 24, 2024

Here n is determined from the line's length (free in lua) and the window width.

Shoot, that's a good point. Better to do some quick arithmetic rather than have a list that's O(height) to access. This new commit should cover everything: view_lines as a range, as well as view_height and view_width, allowing plugin developers to calculate sidebar width. I'll probably hold off on making an entire class for View until there's reason to, say, implement view-specific method functions (can't think of any right now). In any case, thanks for the advice!

@rnpnr
Copy link
Collaborator

rnpnr commented Apr 24, 2024

Can you keep the UI stuff relegated to your other pull request? Sorry to ping-pong back and forth but its unrelated to this. I will leave a further comment there.

As for the view_lines, after thinking about it more I think viewport.lines and viewport.bytes makes the most sense and keeps it clear that these refer to the same section of the file. It breaks the existing API but we made no guarantees about that.

It doesn't need to be a separate class, you can just push a table with both ranges when win.viewport is referenced. With that change I will commit this.

@dther dther requested a review from rnpnr April 25, 2024 12:52
@dther
Copy link
Contributor Author

dther commented Apr 25, 2024

Rebased and force-pushed in order to get rid of a commit that's been moved to #1180. Hope the plugin maintainers don't get too upset about the API change. 😛

This will break all plugins which currently use Win.viewport.
@rnpnr rnpnr merged commit 1fc1756 into martanne:master Apr 25, 2024
19 checks passed
@rnpnr
Copy link
Collaborator

rnpnr commented Apr 25, 2024

Applied thanks!

Hope the plugin maintainers don't get too upset about the API change.

If they do they can get mad at me, There are tons of cool ideas for plugins that could actually be created if the API was a little more expressive. Some things will have to be broken to achieve that.

@dther dther deleted the win-linemap branch April 26, 2024 03:57
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