Unexpected result of getSelectText()

[AgatZan]

What I mean by "unexpected".

Maybe it's just because I don't really understand the use case of getSelectedText(), but I guess that function will catch the text now under selection, but the '< mark contain last selected region.

[AgatZan]

Bug, when :'<,'>Scratch

vim.api.nvim_get_mode().mode == "n"

[AgatZan]

Bug, when :'<,'>Scratch

vim.api.nvim_get_mode().mode == "n"

Also, find 1 if cause. It is obvious (but not for me) that when using keybinds, args.range == 0, so either make something with keymaps or specify it in the user command. I choose 2 and add this if-branch.

And another problem is that the use of :'<,'>Scratch also fails to select because, it is humorous, but the visual selection at call time is not visually represented AS-IS, and the saved cursor position at the beginning of the selection point. I have fixed this problem.

[AgatZan]

Sometimes, that test suits works, sometimes it doesn't, but I have refactored the function for that use case, so this doesn't matter.

[LintaoAmons]

Hi, this looks really good!

I could merge this PR right now~ But there is a little problem I met when I was testing it locally (:1,5Scratch).

Here is the error msg and some info might be helpful. But if you think it not a big issue, I also think there's no need to bother with it right now.(Cause I don't think many user will user this command this way)

Error executing Lua callback: ...uments/oatnil/release/scratch.nvim/lua/scratch/utils.lua:89: attempt to index a nil value
stack traceback:
...uments/oatnil/release/scratch.nvim/lua/scratch/utils.lua:89: in function 'getSelectedText'
...ts/oatnil/release/scratch.nvim/plugin/scratch_plugin.lua:24: in function <...ts/oatnil/release/scratch.nvim/plugin/scratch_plugin.lua:18>

CodeCompanion ─

Looking at the error and the code, the issue is in the getSelectedText function where it's trying to access text[1] but text might be empty or nil. Let's add some error checking to make the function more robust.

Here's the fixed version of the getSelectedText function:

local function getSelectedText(mark, selection_mode)
    local pos1 = vim.fn.getpos("v")
    local pos2 = vim.fn.getpos(mark)
    local lines = {}
    
    -- Validate positions
    if not pos1 or not pos2 then
        return lines
    end
    
    local start_row, start_col = pos1[2], pos1[3]
    local end_row, end_col = pos2[2], pos2[3]
    
    -- Get text from buffer
    local text = vim.api.nvim_buf_get_lines(0, start_row - 1, end_row, true)
    
    -- Check if text is empty
    if not text or #text == 0 then
        return lines
    end
    
    end_row = end_row - start_row + 1
    start_row = 1
    
    if selection_mode == "v" then
        if text[1] then
            table.insert(lines, text[1]:sub(start_col))
            for i = start_row + 1, end_row do
                if text[i] then
                    table.insert(lines, text[i])
                end
            end
            if lines[end_row] then
                lines[end_row] = lines[end_row]:sub(1, end_col)
            end
        end
    elseif selection_mode == "V" then
        for i = start_row, end_row do
            if text[i] then
                table.insert(lines, text[i])
            end
        end
    elseif selection_mode == vim.api.nvim_replace_termcodes("<C-V>", true, true, true) then
        for i = start_row, end_row do
            if text[i] then
                table.insert(lines, text[i]:sub(start_col, end_col))
            end
        end
    end
    
    return lines
end

[AgatZan]

@LintaoAmons Reason of these issue because i'm not reacting correctly in :Scratch.

There are two use-flow branches: select and range. When I process a range of branches, I think of it as selection, and it is a bug.

  if mode ~= "n" then
    opts = { content = utils.getSelectedText(".", mode) }
  elseif args.range > 0 then
    opts = { content = vim.api.nvim_buf_get_lines(0, args.line1 - 1, args.line2, true) }
  end

I guess that correct way. But now you must use keybinding to perform select branch. Maybe there is a magical way to determine in command mode whether it is selected or just an range, but I don't know of that way.

Also, it is necessary to implement tests that will test this, and for that reason, scratch_api needs to be mocked. I guess, but I don't know how to do that. Maybe replace package.loaded["scratch.api"] and then dofile("plugin/scratch_plugin.lua")

What about your CodeCompanion. I don't really know how vim.fn.getpos can return nil in my opinion. I think that it is bound to nil happen when you pass an incorrect argument, but in this function, you basically do not pass them on. Of course, I should remove the mark argument because it is really not needed. For this reason, vim.api.buf_get_lines cannot give non-existent lines, because there is no such way to get unexpected position. By the way, i agree that it is necessary to handle the case of an empty file

[LintaoAmons]

I saw you pushed some new commits, let me know if you think it's ready to merge

[AgatZan]

@LintaoAmons Specifically within the work of this pull request, I wanted to fix getSelectText, I fixed it, and made some set of tests that would verify something.

Generally speaking, it is necessary to write tests for the plugin/scratch_plugin.lua file, but I have no idea whether they are required within this pull request or whether they should be implemented separately.

[LintaoAmons]

I would say it's not required within this PR. I tested my workflow and it's fine. So if you are confident (comfortable) I could merge it anytime

[AgatZan]

And how to understand it.

The mode is determined correctly.
If call getSelectedText with a api-call to get mode, it gives the correct result

NOTE in tests/test_plugin.lua | Scratch | select_branch | parameter | some_text + args { "\22", { 1, 1, 4, 4 } }: |1>{
  blocking = false,
  mode = "\22"
}
NOTE in tests/test_plugin.lua | Scratch | select_branch | parameter | some_text + args { "\22", { 1, 1, 4, 4 } }: |2>{ "some", "text", "inse", "now" }

but if you do it using keymap, it does not give the correct result

 Failed expectation for equality.
  Left:  { "some", "text", "inse", "now" }
  Right: { "some", "text here will be", "inserted", "now" }
  Traceback:
    tests/test_plugin.lua:112

[LintaoAmons]

Hi @AgatZan , I saw you haven't pushed any new commits now,
Should I merge it?

I tested it locally with my workflow. It's working fine.
I haven't spent time getting into the tests, so it's fine to have those tests failing now.

[AgatZan]

Hi @LintaoAmons. In general, I see no reason to not push. All commit from those i think can actually be omitted, it's just my inner perfectionism that haunts me.