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

fix: make comment ranges work #807

Merged
merged 2 commits into from
Jan 22, 2025

Conversation

ldelossa
Copy link
Contributor

@ldelossa ldelossa commented Jan 17, 2025

Previous to this comment highlighting several lines and running the command "Octo comment add" would never result in a multi-line comment.

This is seemingly because as soon as you enter the command line with ':' Neovim is switched back into normal mode and the "v" and "." attributes to vim.fn.line no longer register for the line range.

Instead, we can use the explicit start and end marks for a selected range to capture the currently selected range despite being in normal mode at time of command run.

It is now possible to highlight several lines in a review and then call "Octo comment add" to create a multi-line comment.

Describe what this PR does / why we need it

Does this pull request fix one issue?

Describe how you did it

Describe how to verify it

Special notes for reviews

Checklist

  • Passing tests and linting standards
  • Documentation updates in README.md and doc/octo.txt

@wd60622
Copy link
Collaborator

wd60622 commented Jan 17, 2025

Were there any issues for this?

@ldelossa
Copy link
Contributor Author

Only relevant issue is: #303

But there seems to be a lot of refactoring since then. Possible it broke or most people are using keymaps to create multiline comments? I'm not sure.

I'd want the command to work for multi-line comments, as I already have far too many keymaps and just typing things out are becoming an easier approach.

Previous to this comment highlighting several lines and running the
command "Octo comment add" would never result in a multi-line comment.

This is seemingly because as soon as you enter the command line with
':' Neovim is switched back into normal mode and the "v" and "."
attributes to `vim.fn.line` no longer register for the line range.

Instead, we can use the explicit start and end marks for a selected
range to capture the currently selected range despite being in normal
mode at time of command run.

Signed-off-by: ldelossa <[email protected]>
@ldelossa ldelossa force-pushed the ldelossa/make-comment-ranges-work branch from 559e06e to f369961 Compare January 18, 2025 00:55
@wd60622
Copy link
Collaborator

wd60622 commented Jan 21, 2025

Awesome. Let me take a look tomorrow

Copy link
Collaborator

@wd60622 wd60622 left a comment

Choose a reason for hiding this comment

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

Works like a charm. Nice live quality improvement!

@wd60622
Copy link
Collaborator

wd60622 commented Jan 22, 2025

Closes #587

@wd60622 wd60622 linked an issue Jan 22, 2025 that may be closed by this pull request
@wd60622 wd60622 merged commit e20b4f0 into pwntester:master Jan 22, 2025
3 checks passed
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

@ldelossa ldelossa mentioned this pull request Jan 27, 2025
2 tasks
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.

comment add doesn't work for review's visual block
3 participants