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.

Sign up for GitHub

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

fix: make comment ranges work #807

Merged
merged 2 commits into from
Jan 22, 2025
Merged

fix: make comment ranges work #807

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f369961
fix: make comment ranges work
ldelossa Jan 17, 2025
df0e1b1
Merge branch 'master' into ldelossa/make-comment-ranges-work
wd60622 Jan 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lua/octo/commands.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ end
function M.setup()
vim.api.nvim_create_user_command("Octo", function(opts)
require("octo.commands").octo(unpack(opts.fargs))
end, { complete = require("octo.completion").octo_command_complete, nargs = "*" })
end, { complete = require("octo.completion").octo_command_complete, nargs = "*", range = true })
local conf = config.values

local card_commands
Expand Down
4 changes: 2 additions & 2 deletions lua/octo/utils.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1570,8 +1570,8 @@ function M.get_lines_from_context(calling_context)
line_number_start = vim.fn.line "."
line_number_end = line_number_start
elseif calling_context == "visual" then
line_number_start = vim.fn.line "v"
line_number_end = vim.fn.line "."
line_number_start = vim.fn.line "'<"
line_number_end = vim.fn.line "'>"
Copy link
Contributor

Choose a reason for hiding this comment

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

hey @wd60622 @ldelossa , looks like this PR broke non-range selection.
Because the caller always pass "visual" as the calling_context, when not in the visual mode, this function will always return 0, 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you say "non range selections" what do you mean? Can you give me steps to reproduce and ill take a look

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant in the normal mode. When I add comment without any visual selection, the comment buffer can still pop up. But when I save the comment, it will fail with an error "failed to create thread".
I dug into this issue and it turned out that the bug is because the line numbers are 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And is this with a keybind or you type the add comment command?

Copy link
Contributor

Choose a reason for hiding this comment

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

it's with a keybind

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup I see. Thanks for info. I think youre onto it with maybe using "visual" argument in every scenario.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay I am recalling why I have not ran into this.

Canonically, a command figures out if a selection is being provided to it based on the arguments the command is called with.

For instance when you highlight a few lines and you enter the command line with ":" there is a reason you see this:
image

Its because Neovim doesn't really provide a way for you to access "the previous mode before you entered the command line". Instead, if you entered with a a line range you get the range as arguments to the command.

The issue I see is that Octo does not plumb user command arguments down into command handlers.
I can see how easily we can do this, it would be the "right" for this, and Octo can stop trying to retroactively understand whether visual lines are highlighted or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened a PR with a proposed fix for this: #825

elseif calling_context == "motion" then
line_number_start = vim.fn.getpos("'[")[2]
line_number_end = vim.fn.getpos("']")[2]
Expand Down
Loading