martanne / vis

A vi-like editor based on Plan 9's structural regular expressions
Other
4.19k stars 260 forks source link

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

Closed dther closed 2 months ago

dther commented 2 months ago

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.

dther commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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. :stuck_out_tongue:

rnpnr commented 2 months ago

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.