Skip to content

Conversation

@MiguelRoldao
Copy link

@MiguelRoldao MiguelRoldao commented Nov 21, 2024

To add multi-cursor support I had to re-engineer the algorithm of the plug-in quite a bit. I tried to reuse as much code that was already implemented as possible to avoid introducing new bugs. I've tested some use cases and I haven't detected any problems so far.

The new algorithm has the following workings:

  1. All the selected lines are taken from all the cursors and compiled into a table.
  2. The lines table will be fed to toggleCommentSelection() which will (un)comment all of the lines, keeping indentation formatting.
  3. The cursors will be restored to their correct position.

Note: All cursor logic is now contained in comment().

I also sneakily added a comment type for the language I'm developing in my master's thesis :P (https://freest-lang.github.io/)

Copy link
Contributor

@Andriamanitra Andriamanitra left a comment

Choose a reason for hiding this comment

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

I played around with this a bit and everything seems to work alright even in weird edge cases like multiple cursors on the same line, selections with comments inside them etc. It's really nice that the cursor position is now updated accordingly when a line gets commented.

The only slight annoyance I found is that if the selection ends in a newline, the line immediately after the selection gets commented too. This is not how it used to work, and not how I would like it to work. To reproduce:

  1. Open a file like the one below
# some comment
print("hello")

print("world")
  1. Select the first two lines by holding shift and pressing arrow down twice.
  2. Comment selection. Before this PR you would get comments only on the first two lines, but now you get a # on the empty line too.

ft["dockerfile"] = "# %s"
ft["elm"] = "-- %s"
ft["fish"] = "# %s"
ft["freest"] = "-- %s"
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems unrelated to the content of this PR. What's freest? Micro doesn't have built-in support for a file type called "freest". If it gets special treatment in the comment plugin it should probably have a file type (syntax file under micro/runtime/syntax/) as well. Whether or not it's popular enough to warrant built-in support could be a separate PR and discussion.

Copy link
Author

Choose a reason for hiding this comment

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

Because the end of a selection happens to be on the new empty line it would be commented as well.

I agree that what you commented should be the correct behavior. In the last commit I added a fix for this. I added an exception to not comment the last line of a selection, if it is an empty line.

Regarding FreeST, you're right. I removed the changes. May add full language support in another PR at some point ;)

Copy link
Collaborator

Choose a reason for hiding this comment

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

So 05a6870, f794609 & a023a00 can be removed from the PRs history.

Copy link
Collaborator

@JoeKar JoeKar Nov 19, 2025

Choose a reason for hiding this comment

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

So 05a6870, f794609 & a023a00 can be removed from the PRs history.

These commits are still lurking around in the history.
Also the...

I also sneakily added a comment type for the language I'm developing in my master's thesis :P (https://freest-lang.github.io/)

...can then be removed from the PR's description.

Copy link
Contributor

@Andriamanitra Andriamanitra left a comment

Choose a reason for hiding this comment

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

The latest changes introduced some bugs. I opened a PR in your fork with suggested fixes that you may use if you wish.

Comment on lines 167 to 175
if cursor.CurSelection[endSel].X == 0 then
lines[cursor.CurSelection[endSel].Y] = nil
staticEnd = true
end
Copy link
Contributor

Choose a reason for hiding this comment

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

This can end up overwriting lines that were already set to true by another cursor.

Demo: https://asciinema.org/a/VB99WIg0Oo6HJAkqc4KVeHEPP

Copy link
Author

Choose a reason for hiding this comment

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

Well caught! I have merged your suggestions. It solves the bug, and didn't seem to change any other behavior.

@Gavin-Holt
Copy link

Hi,

Sorry to be late to this contribution.

But can I re-state a strong preference for comment characters at the start of the line. #2282. This is especially useful for commenting out sections that already contain comments:

void somefunc(int a)
{
//  if (a) {
//    a += 2;
// // After adding two we can print
//    printf("a = %d\n", a);
//  } else {
// // Print "none" if not a
//    printf("none");
//  }
}

I know others feel differently, so an optional setting seems the sensible solution.

Would it be possible to use the commenttype setting to indicate that comments reside at the start of the line?
e.g.

      "*.lua": {
        "commenttype": "^-- %s"
    },

Or would another setting item be required to indicate comment location?

      "*.lua": {
        "commenttype": "-- %s",
        "commentloc": "^"
    },

Sorry I can't offer a solution, I don't think I have the knowledge to get it close to right.

Kind Regards Gavin Holt

@MiguelRoldao
Copy link
Author

I honestly prefer the way it is implemented as of now. Having the least amount of white space between the comment characters and the text (while keeping all the comment chars of a multiline comment in the same column).

However, it should be pretty straightforward to implement a "commentindent" option:

  • true (default): implemented behaviour.
  • false: ignores indentation and places the comment characters in the beginning or the line.

I'd have no problem dedicating an hour to this. However, I don't know if this feature is inside the scope of this PR. Maybe @Andriamanitra has something to say about it.

@Andriamanitra
Copy link
Contributor

The indentation issue (#2282) is mostly unrelated so I would fix it in a separate PR, but I don't know the preferences of the maintainers (@dmaluka and @JoeKar).

Personally I think indented comments are way more readable, but I guess there's no harm in adding an option as long as the default behavior stays the same.

@JoeKar
Copy link
Collaborator

JoeKar commented Nov 29, 2024

The indentation issue (#2282) is mostly unrelated so I would fix it in a separate PR, but I don't know the preferences of the maintainers (@dmaluka and @JoeKar).

I like as it is by default right now including the alignment. Furthermore I support this comment.
When I remember right, then there are languages and/or parsers getting into trouble in the moment the syntactic indentation is interrupted by a comment started at the beginning of the line, but I could remember wrong.

I guess there's no harm in adding an option as long as the default behavior stays the same.

Yep. But if so then independent of this PR.

@MiguelRoldao MiguelRoldao changed the title Added support for multi-cursor comments to the comment plugin (V1.1.0) Added support for multi-cursor comments to the comment plugin Nov 11, 2025
@MiguelRoldao
Copy link
Author

Sorry I have been away. Sometimes life gets busy, and suddenly its been almost a year.

Anyways, the version is back to how it's supposed to be. Also merged with the master branch to resolve conflicts.

I have been using this PR since and haven't found any further issues.

Anything else that needs to be dealt with before merging @Andriamanitra @JoeKar ?

local curData = {}
-- gather cursor data and lines to (un)comment
for i = 0,#bp.Buf:getCursors()-1 do
local cursor = bp.Buf:getCursor(i)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line introduces trailing whitespaces, which are removed in 0165e41.
Looks like the whole history needs a cleanup.

local config = import("micro/config")
local buffer = import("micro/buffer")
local micro = import("micro")

Copy link
Collaborator

@JoeKar JoeKar Nov 19, 2025

Choose a reason for hiding this comment

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

Instead of merging the master branch into the PR branch it is more preferred to rebase the PR branch against the actual upstream master.
This allows us to extract a clean set of patches, which can be applied easily.

This also helps to cleanup the PR's history.

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.

4 participants