Multiple cursors support #8023
Multiple cursors support #8023
Conversation
I have a weeks-long PoC that I haven't published. Guess I should get on that. It will take time to go through yours, but I'll note that mine did not involve a tree-wide change; instead the cursors are implemented "mostly" out-of-band, they work with the legacy code transparently.
No thanks, see #153 . We would like to move towards using Lua for internal logic where possible, and we are working towards that.
Indeed. If we haven't already proven by construction that such an "incremental" refactor that you alluded to is a delicate endeavor that involves much, much, much more work that you think, then I don't know how else to express it. |
|
Does this PR implement the following:
|
|
Looks like cursors are per-window, instead of per-buffer, any reason you chose to do that? Also I have been wondering if there is really any reason to support cursor persistence after buffer-switch or window-switch. |
I didn't read the whole thread, but as I said I am not suggesting a rewrite, just allowing to use some features here and there. As for the understandability of C++, it's not hard, if one can understand C, they can understand C++, it's just a matter of learning it. Anyway, I'll stop here on c++ since it's not the main point of this.
It is just because it's a delicate endeavor that I think an incremental approach is the best. I'm sure it involves much more work than I can think of, but the question is: will there be any insurmountable roadblock? For per-cursor registers, can't we store the registers in the cursor_T structure? I do know that it is not a simple thing, but I still think that it can be done incrementally. Such a reasoning can surely be applied to undo, dot-repeat and completion. Support for multi-cursor for these features can be done step-by-step.
As I said, this is my first time hacking into vim, I only did what seemed to be the simplest approach. However, I think this is the most intuitive approach. As vim does not keep memory of what was the visual selection when leaving a buffer (or does it?), I don't think we should keep memory of multiple cursors in a buffer that's not on the screen.
If you skip the [sed] commits, there shouldn't be much to review.
That's another approach I did not think of, I would like to see how you did that. Doesn't it add tons of code? I was hoping that my approach would help at refactoring existing code to make it more maintainable and actually reduce the number of lines. |
|
I don't like the |
We already use klib.h for some simple generic datatypes, including |
I think this should be temporary and is only meant as a way to avoid breaking code. As the code gets refactored to support multiple cursors, hardcoded accesses to that first element should decrease. My opinion is that functions that modify the cursor state through the window_S structure should instead access the cursor_T structure only, which can only be done if the "cursor" part of the window structure is split apart. I did duplicate the oneleft and oneright function, but my hope is that oneleft could be written later as
My knowledge of vim's codebase prevents me from understanding what's really going on in these two PRs. I understand your approach, but shouldn't we refactor that code instead of adding more code to handle new features? Are you planning to remove the old global grid in the future and make all code depend on your new grid system?
Thanks, that's good to know. However that doesn't help with memory management which would be so much easier to do in a constructor/destructor instead of everywhere in the program. |
Would be interested in a separate PR for that change. |
I think I understand your rationale better now. But temporary state has a tendency to become permanent... It would be really interesting to see @justinmk 's approach to determine the tradeoffs :) |
I am not sure what you mean... If we want a oneleft function that acts on a cursor structure, that structure must be split out from the window structure first which is what I start to do in 2e40a74 but that commit isn't complete without the following 4 which include the big [sed] commits, so the PR for that change would be almost as big as this one. |
|
@blastrock When you mentioned "pure function" I thought you meant a function that takes a position + other parameters and returns an integer. If it modifies e.g. a window structure then I misunderstood. |
|
|
|
If we make such a function and leave the cursors variables as they are in the current window struct (so, we don't take the big changes I propose), when the user presses 'h', we would have to make one call for the main cursor with its variables in the window struct and one different call in a loop for all the additional cursors. Then to support all other move and edit actions the user can do, we would have to write two of such calls. The pure function you guys are suggesting would indeed be better than the current function, but I don't think it would help much to reduce the quantity of code for multicursor support. |
sed -ri 's/\bw_valid_leftcol\b/w_cursors[0].w_valid_leftcol/g' src/**/*.c sed -ri 's/\bw_cline_height\b/w_cursors[0].w_cline_height/g' src/**/*.c sed -ri 's/\bw_cline_folded\b/w_cursors[0].w_cline_folded/g' src/**/*.c sed -ri 's/\bw_cline_row\b/w_cursors[0].w_cline_row/g' src/**/*.c sed -ri 's/\bw_virtcol\b/w_cursors[0].w_virtcol/g' src/**/*.c
vim_do_replace() {
sed -ri 's/\boap->'"$1"'\b/oap->cursors[0].'"$1"'/g' src/**/*.c
sed -ri 's/\boparg\.'"$1"'\b/oparg.cursors[0].'"$1"'/g' src/**/*.c
sed -ri 's/\boa\.'"$1"'\b/oa.cursors[0].'"$1"'/g' src/**/*.c
}
vim_do_replace start
vim_do_replace end
vim_do_replace cursor_start
vim_do_replace line_count
vim_do_replace empty
vim_do_replace start_vcol
vim_do_replace end_vcol
blastrock
force-pushed
the
multi_cursor3
branch
from
1d4e1dc
to
132caa4
Compare
|
I fixed the unit tests and the ui refresh thing. There is still a memory leak that makes the CI fail. |
|
Gentle ping, @blastrock. Also, @justinmk, could you please clarify whether you've decided to go further with this pull request or favor your own PoC that you mentioned earlier? |
|
@balta2ar This PR isn't anywhere close to complete AFAICT. That's not meant as a slight, just making clear my current understanding of the status. I would not encourage further work on this PR until I can revisit my local branch. I don't have a timeline for that, I am juggling many things. |
justinmk
added
needs:discussion
Motivation
I spent some time using kakoune in the past and I liked the multicursor approach a lot. It gives you much more power, feedback and undo-capacity than simple vim usage (e.g. with macros and regexes). Most of the modern editors have multi-cursor support and I think neovim needs that support to stay in the competition.
There are plugins like vim-multiple-cursors to do that. vim-multiple-cursors is a great plugin, but it does crash sometimes, leaving the editor in a strange state with virtualedit enabled. The undo command feels weird and the plugin does seem like a hack.
I think multiple cursors must be implemented as a core feature to be usable.
What is this?
I don't mean to merge this pull request to master as-is. This is a weekend-long PoC to try to implement multiple cursors in neovim.
I wanted to see if multiple cursors could be implemented incrementally without breaking existing code and features.
This branch binds ^A to spawn a cursor on the line below. It handles moving the cursor left and right and using x to delete a character.
Demo time!
Review instructions
I split my work in multiple commits, some of which don't compile.
Commits tagged with [sed] are just search-and-replace commits, you don't need to read them. Some commits are tagged with [failcompile], as the name implies they don't compile and are split just to ease the review.
Commits should be reviewed with the "ignore whitespace changes" option enabled because a lot of code has been indented to be put in a loop. I am not sure github supports that, but you can use the -w switch on most git commands that show a diff for that.
Issues
I could not find how to refresh specific lines when cursors are spawned or moved, so some functions call ui_schedule_refresh() which redraw the whole screen.FIXEDSome functions leak. I didn't implement proper memory management. They will be easy to find with clang ASAN anyway.
I couldn't manage to run the tests but they are most likely broken since this is still a draft.FIXEDMy impressions
This is the first time I hack into neovim and I find it quite hard to get into. Yet, I think this feature can be implemented incrementally. The add_cursor function could be hidden behind a feature gate until the feature is fully usable and neovim could be refactored progressively to support multiple cursors.
Despite all the work that has been done, neovim still needs lots of refactoring. There are still a lot of global variable accesses, a lot of ways of doing the same thing, a lot of very very long functions (what is that 7500 lines long screen.c with that 2200 lines long function there!). I assume all this is due to its legacy.
As a side-note, I am a C++ developper and I think that C++ could really make the project smaller and more maintainable. Similarly to the multiple cursor migration, I think that neovim's code can be slowly migrated to C++ without some major rewrite. It doesn't need to make use of all of C++ advanced features but I would have liked using
std::vectorhere and there, letting the compiler do the memory management for me. Constructors would have helped with the structure I changed, where cursors are now allocated, I needed to find and modify everywhere such a structure was allocated and I probably missed some of them.I don't think I will be able to spend much time on this feature in the near-future, forget about finishing the feature alone. I just hope this can help figure out if and how this feature can be implemented.
So, what do you think of all this? Are there some parts of neovim that may be much harder to migrate to multiple cursors? Is there any chance that this feature lands on master someday? Do you think this is the right approach?
Related issues
Related to: #211, #3845, #7257
PS: Actually I just discovered #7257 while writing this post, I didn't know that someone was actually working on this ^^