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

feat(commenting): Add 'inner comment' text object #29428

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

yochem
Copy link
Contributor

@yochem yochem commented Jun 20, 2024

Problem

No way to change a comment while keeping the markers.

Currently changing a html comment:

<!-- a |comment -->

Type: cgc<!-- new comment -->

<!-- new comment -->|

I think changing an entire comment is done quite often by programmers.

Solution

Add an 'inner comment' text object, which is in line with it, i{, i[, and other surrounding text objects.

The default keymap for this text object is igc.

With this change:

<!-- a |comment -->

Type: cigcnew comment

<!-- new comment| -->

Fixes #29318.

Problem: No way to change a comment while keeping the markers.

Solution: Add an 'inner comment' text object, which is in line with 'in
tag', 'i{', 'i[', and other surrounding text objects.

The default keymap for this text object is 'igc'.
@github-actions github-actions bot added defaults issues or PRs involving changing the defaults comment labels Jun 20, 2024
@echasnovski
Copy link
Member

Thanks for the PR!

I personally don't think this is a good addition to built-in commenting for at least the following reasons:

  • The "inner comment textobject" is mostly useful only in the cigc situation. The already present cgc preserves the indent (which is a tedious part to add) while inserting comment marks is usually not a big issue. Even if comment marks are verbose, ability to add them in Insert mode looks more ergonomic and can be mostly achieved with manual Insert mode mapping.
  • Adding igc default textobject will interfere having ig textobject for users.

@yochem
Copy link
Contributor Author

yochem commented Jun 20, 2024

The "inner comment textobject" is mostly useful only in the cigc situation.

The cigc point is very true. I do however think this is something that many users do many times: rewriting an entire comment. They might expect the comment object to be in line with other surrounding objects, like tags.

while inserting comment marks is usually not a big issue.

This could also be said about inserting (), [], and (html) tags. It's not that typing the comment marks is a lot of work, but rather that the current situation feels like there is missing something. I also don't think it's ideal to type cgci<!-- comment --> instead of cigc comment.

ability to add them in Insert mode looks more ergonomic and can be mostly achieved with manual Insert mode mapping.

This is an interesting idea, but might be hard for users to add, without the useful functions currently in this module. One would need to get the commentstring, parse left and right parts, insert those around the current cursor position. Not trivial.

  • Adding igc default textobject will interfere having ig textobject for users.

Haven't heard of this textobject, and can't find it in the docs. Where can I find this in the help pages?

Yeah, that is true. Could be changed to ic if we want to prevent that, or not create a default map for it.

@yochem yochem force-pushed the inner-comment-text-object branch from 80ce439 to 928e700 Compare June 20, 2024 10:11
@echasnovski
Copy link
Member

This is an interesting idea, but might be hard for users to add, without the useful functions currently in this module. One would need to get the commentstring, parse left and right parts, insert those around the current cursor position. Not trivial.

I mean, maybe not trivial, but pretty straightforward in my opinion:

local insert_comment_parts = function()
  local cms = vim.bo.commentstring
  local offset = cms:find('%%s')
  local insert = cms:gsub('%%s', '')

  local row, col = unpack(vim.api.nvim_win_get_cursor(0))
  vim.api.nvim_buf_set_text(0, row - 1, col, row - 1, col, { insert })
  vim.api.nvim_win_set_cursor(0, { row, col + offset - 1 })
end
vim.keymap.set('i', '<C-b>', insert_comment_parts)

The only drawback is that it uses buffer's commentstring instead of the one "at cursor", which built-in commenting uses. Exporting it (and the built-in variant of this functionality) was discussed in #28997.

@justinmk
Copy link
Member

I also don't think it's ideal to type cgci<!-- comment --> instead of cigc comment.

Please add that to the Problem description :) That is a stronger, clearer motivating case.

Copy link
Member

@justinmk justinmk left a comment

Choose a reason for hiding this comment

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

Hard to argue against this. Code looks minimal, and for comments like /** ... */ and <!-- ... --> it's definitely useful.

@echasnovski
Copy link
Member

@justinmk, there are issues with this. If this type of functionality is useful, exporting this type of Insert mode mapping is a more versatile approach.

@justinmk
Copy link
Member

@justinmk, there are issues with this. If this type of functionality is useful, exporting this type of Insert mode mapping is a more versatile approach.

Yes, I read that and almost closed this issue, but the use-case is strong enough to make the case for this being a default.

Even if comment marks are verbose, ability to add them in Insert mode looks more ergonomic

I don't get this argument. Textobjects are the primary text manipulation interface for vim. Insert-mode stuff is what vscode does.

Adding igc default textobject will interfere having ig textobject for users.

We added commenting so we need to claim a textobject. We can choose igc or ic. I'm fine with either.

@echasnovski
Copy link
Member

Even if comment marks are verbose, ability to add them in Insert mode looks more ergonomic

I don't get this argument. Textobjects are the primary text manipulation interface for vim. Insert-mode stuff is what vscode does.

The argument is that "insert comment marks" is a more versatile approach. For example, it can be used together with o or O to start a comment.

The suggested "inner textobject" is primarily (if not even only) useful in one particular situation of cigc. This can be the same as cgc followed by an "insert comment marks".

Adding igc default textobject will interfere having ig textobject for users.

We added commenting so we need to claim a textobject. We can choose igc or ic. I'm fine with either.

We did with gc. Claiming igc for built-in will break any potential ig user textobject. Claiming ic without ac is doable, but seems like wasting mapping space.


My suggestion is to do either of the following (from most to least preferred):

  • Export function which will be able to "get commentstring at cursor". Either as a more general "get option at cursor" or the one in vim._comment.
  • Do nothing.
  • Make default Insert mode mapping to insert comment parts at cursor. This is lower than "Do nothing" because this is fairly easy to add to config.

Making built-in separate textobject that is useful only for a single operator is not on the list, I am afraid.

@max397574
Copy link
Contributor

We did with gc. Claiming igc for built-in will break any potential ig user textobject. Claiming ic without ac is doable, but seems like wasting mapping space.

well then we couldn't add any new builtin mappings?
also I can't think of any textobject which you'd abbreviate with ig

and imo we should also add ac or agc as a textobject including comment markers
this is especially usefull to delete comments imo

@yochem
Copy link
Contributor Author

yochem commented Jun 20, 2024

The suggested "inner textobject" is primarily (if not even only) useful in one particular situation of cigc. This can be the same as cgc followed by an "insert comment marks".

Yanking only the text of a comment is definitely also a useful operator to have 'inner comment' for.

@justinmk
Copy link
Member

justinmk commented Jun 20, 2024

btw, why is cgc acceptable but we had to revert the cr mappings?

@max397574
Copy link
Contributor

because we just map gc and not cgc
having a mapping starting with c was the problem described in this issue afaict

@echasnovski
Copy link
Member

well then we couldn't add any new builtin mappings?

Of course adding built-in mappings is possible. But having igc as Operator-pending mode mapping in particular means that users will have to wait for timeoutlen milliseconds after cig if they want to use ig as their textobject. This creates "either ig or igc" type of situation which I described is "breaking potential use of ig"

... also I can't think of any textobject which you'd abbreviate with ig

Me neither off the top of my head. But that doesn't mean not accounting for it is bad. Plus nvim-various-textobjects uses it for "greedyOuterIndentation" together with ag.

and imo we should also add ac or agc as a textobject including comment markers this is especially usefull to delete comments imo

This is already possible with dgc. If I understand correctly the suggestion ("outer" variant of the proposed here igc variant), then I doubt that "delete comment but leave indentation of its first line" is a common operation.

Yanking only the text of a comment is definitely also a useful operator to have 'inner comment' for.

I agree, it is indeed useful. But the proposed igc textobject that works well for cigc will not work well for yigc. Because with multiline comment it will yank the whole charwise region including all comment parts except left of first line and right of last line.

btw, why is cgc acceptable but we had to revert the cr mappings?

There is no cgc Normal mode mapping: there is built-in c operator and gc Operator-pending mode mapping (claimed as it is the established pattern for comment textobject, together with gc operator).

Creating crr/ crn Normal mode mappings also removes the possibility for r and rr / rn Operator-pending mode mappings. Meaning that:

  • If user want to have r textobject, it will work with c built-in operator only after timeoutlen milliseconds wait.
  • User can not use rr/rn textobjects with c operator but can with others.

@yochem
Copy link
Contributor Author

yochem commented Jun 20, 2024

I agree, it is indeed useful. But the proposed igc textobject that works well for cigc will not work well for yigc. Because with multiline comment it will yank the whole charwise region including all comment parts except left of first line and right of last line.

Yes, that's true for multiple singleline comments following each other. However, it's something that could be fixed if this behaviour is desired.

@echasnovski
Copy link
Member

Yes, that's true for multiple singleline comments following each other. However, it's something that could be fixed if this behaviour is desired.

As built-in commenting is designed to be used only with single line comments, I don't see a way to make igc textobject work as desired with both c (to delete comment block except indent and single comment markers) and y (to yank only commented text without per line comment markers) operators.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comment defaults issues or PRs involving changing the defaults
Projects
None yet
Development

Successfully merging this pull request may close these issues.

commenting: add 'inner comment' text object
4 participants