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

PR: Add multi-cursor support to the editor #22996

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

Conversation

athompson673
Copy link
Contributor

@athompson673 athompson673 commented Nov 16, 2024

Description of Changes

Multi-line editing has been a feature I've missed having in spyder for a long time. I frequently have a notepad++ scratchpad alongside spyder simply to copy blocks of text back and forth so that I can use the column cursor. I haven't been able to contribute money to the spyder project despite using it for about a decade now, but I can hopefully at least contribute some time.

I am an engineer, not a programmer so please criticize my work so we can make it better :)

I decided to implement the functionality directly inside the codeeditor.CodeEditor class as making it an editor extension would still require a great many edits to CodeEditor anyway. The basic implementation is to hide the original cursor when there are multiples, and manually draw the primary cursor and all extras on a timer. For many editor functions a for_each_cursor wrapper does a lot of heavy lifting. Other functions need to be slightly tweaked directly. I think it is not particularly productive to try to implement code completion (snippets, call-tips, etc) during multi-cursor sessions, so I attempt to disable those functions appropriately.

These are some deep changes to one of the most complicated parts of Spyder, so I will naturally need some help getting this code to a state where it's ready to be integrated. I know it is not yet fully ready, but I wanted to get the PR in so that more people could see it and maybe get interested in testing it.

Media1.mp4
TODO list:
  • Ctrl-Alt click to add/remove cursor
  • Ctrl-Alt-Shift click to add column cursor
  • Add multi-cursor toggle to editor preferences
  • Add selection highlight color to theme preferences? (not now. hardcoded.)
  • Handle cursors moving into folded code
  • Render multiple cursors and selections
  • render cursor when dragging text selection to new position
  • Render overtype cursors (insert)
  • Arrow keys, Home/End cursor movement
  • Backspace / Delete
  • Text typing
  • Undo / Redo
  • Cut Copy Paste
  • Smart insertion of characters (colons, quotes, parenthesis, etc..)
  • Smart backspace
  • Automatic whitespace insertion
  • Cursor position history
  • Last edit position
  • How to deal with auto-completions, call tips, snippets, etc. (disable them basically)
  • Write many tests
  • Evaluate all shortcuts (add multi-cursor handling, only handle primary cursor, or disable when multi-cursor):
    • code completion (disabled)
    • duplicate line up/down (handles multi-cursor)
    • delete line (handles multi-cursor)
    • move line up/down (handles multi-cursor)
    • go to new line (handles multi-cursor)
    • line number/definition (from cursor)/next, previous cell (clears multi-cursor)
    • toggle comment (handles multi-cursor)
    • create new cell (handles multi-cursor)
    • (un)blockcomment (handles multi-cursor)
    • transform to upper/lowercase (handles multi-cursor)
    • (un)indent (handles multi-cursor)
    • start/end of document (clears multi-cursor)
    • start/end, prev/next line (handles multi-cursor)
    • prev/next char/word (handles multi-cursor)
    • next/prev warning (clears multi-cursor)
    • killring (disabled)
    • undo/redo (handles multi-cursor)
    • cut/copy/paste (handles multi-cursor)
    • delete (handles multi-cursor)
    • select all (clears multi-cursor)
    • docstring (handles multi-cursor)
    • autoformatting (clears multi-cursor)
    • scroll line up/down (not applicable)
    • enter array inline/table (clears multi-cursor)
    • inspect current object (handles main cursor)
    • last edit location (handles multi-cursor)
    • next/prev cursor position (handles multi-cursor)
    • Run Cell (and advance) (in debugger) (handles main cursor)
    • Run Selection (and advance) (from line) (in debugger) (up to line) (handles main cursor)

Issue(s) Resolved

Fixes #2112

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct: athompson673

Required manually tracking overwriteMode and leaving it disabled except for during keyEvent. This might be a fragile solution...
…to new line, and clears extra_cursors on goto_definition
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Second round of suggestions for you @athompson673. Again, mainly code style improvements except for one important comment.

athompson673 and others added 4 commits January 3, 2025 17:02
missed a few style edits in prior batch

Co-authored-by: Carlos Cordoba <[email protected]>
…Simplify call path to get font in paint event.
@athompson673
Copy link
Contributor Author

athompson673 commented Jan 4, 2025

@ccordoba12

Second round of suggestions for you @athompson673. Again, mainly code style improvements except for one important comment.

I've done some digging to try and find where the selection color is set, and I have found where the colors are defined, but not where they are used. The foreground and background are accessible as SpyderPalette.COLOR_TEXT_1 and SpyderPalette.COLOR_ACCENT_2 respectively, but I can't find where they are used to set the stylesheet. Should I just use those two values directly? It seems like the css_path in "spyder.ini" has the possibility of conflicting maybe?

Edit.. I found the call to qdarkstyle.load_stylesheet(palette=SpyderPalette) in spyder.utils.stylesheet.AppStylesheet. Is it more appropriate to access the colors from spyder.utils.stylesheet.APP_STYLESHEET? I'm not sure what's going to be most correct in terms of the plans you have on changing for sypder 6.2.

Edit 2.. Maybe most appropriate to use CodeEditor.palette().highlightedText().color() and CodeEditor.palette().highlight().color()? I had to do some learning on how qt styles work to find that was available. maybe not.. wrong color is returned:
image

Edit 3. Getting the colors from the widget.palette works if I delay the call (and I went ahead and cached the result because it could get called frequently).

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks for your continued work on this @athompson673!

@athompson673
Copy link
Contributor Author

Thanks for your continued work on this @athompson673!

The holidays are always a bit slow at work 😅

@ccordoba12
Copy link
Member

Some suggestions after manually checking your work:

  • I think we should use the Alt+Click shortcut to set multiple cursors.
    • It's easier to use than Ctrl+Alt+Click
    • It's the one used by VSCode, so it'd make easier for users to switch between VSCode and Spyder for different tasks.
    • It'd prevent to underline symbols when trying to set multiple cursors in some siuations, which is distracting.
  • We should add Shift+Alt+Up/Down to insert cursors above/below the current one (as VSCode and Sublime Text do).
    • It's something that probably any user would expect from multicursor support (pressing Alt+Click to have many cursors is not so efficient).
    • I don't know how hard it could be to do (we could leave it for another PR if you think it is).

@athompson673
Copy link
Contributor Author

athompson673 commented Jan 7, 2025

@ccordoba12

Some suggestions after manually checking your work:

* I think we should use the `Alt+Click` shortcut to set multiple cursors.
  
  * It's easier to use than `Ctrl+Alt+Click`
  * It's the one used by VSCode, so it'd make easier for users to switch between VSCode and Spyder for different tasks.
  * It'd prevent to underline symbols when trying to set multiple cursors in some siuations, which is distracting.

Currently Alt+Click is already used to jump within a document. Should the two be swapped? It is also Alt+Click (and drag) in Notepad++, so I agree for the purpose of extra cursors it would be better. It should also be possible to prevent underlining links while Ctrl+Alt is held by changing the mouseMoveEvent in CodeEditor. I wonder if at some point, having enough mouse interactions would justify allowing users to customize which modifiers perform which actions?

* We should add `Shift+Alt+Up/Down` to insert  cursors above/below the current one (as VSCode and Sublime Text do).
  
  * It's something that probably any user would expect from multicursor support (pressing `Alt+Click` to have many cursors is not so efficient).
  * I don't know how hard it could be to do (we could leave it for another PR if you think it is).

I think this should be an easy enough addition. I did also add the Ctrl+Alt+Shift+Click which adds a column of cursors (one on each row) from the primary cursor to the click location. I was trying to somewhat emulate the Notepad++ function of Alt click and drag to create a box selection. I didn't however want to go through the different drag states including adding extra (possibly temporary) spaces to the end of each line, which is why I chose a second click interaction.

@ccordoba12
Copy link
Member

ccordoba12 commented Jan 8, 2025

Currently Alt+Click is already used to jump within a document. Should the two be swapped?

Just pressing Alt shows the scrollflag position:

image

But doing Alt+Click does nothing for me. So I agree, we should remap that shortcut to Ctrl+Alt+Click and use Alt+Click for multicursors.

It is also Alt+Click (and drag) in Notepad++, so I agree for the purpose of extra cursors it would be better.

Ok, since it's kind of standard in other editors, there's more reason to use it.

It should also be possible to prevent underlining links while Ctrl+Alt is held by changing the mouseMoveEvent in CodeEditor.

Good idea, but we should leave it for another PR.

I wonder if at some point, having enough mouse interactions would justify allowing users to customize which modifiers perform which actions?

Good point too. You could work later on that if you want.

I think this should be an easy enough addition.

Unfortunately, Shift+Alt+Up/Down are already taken by duplicate line up/down on Linux/macOS, so they need to be remapped. Let's leave them as CTRL+Alt+PgUp/PgDown for all OSes (notice the capital CTRL, which maps to Cmd on Mac).

That also requires bumping the CONF_VERSION number (read the comment above it in spyder/config/main.py to learn how to do it).

I did also add the Ctrl+Alt+Shift+Click which adds a column of cursors (one on each row) from the primary cursor to the click location. I was trying to somewhat emulate the Notepad++ function of Alt click and drag to create a box selection.

It seems you forgot to push that change because I can't see it here.

@athompson673
Copy link
Contributor Author

athompson673 commented Jan 8, 2025

Currently Alt+Click is already used to jump within a document. Should the two be swapped?

Just pressing Alt shows the scrollflag position:

But doing Alt+Click does nothing for me. So I agree, we should remap that shortcut to Ctrl+Alt+Click and use Alt+Click for multicursors.

That's surprising to me, as currently CodeEditor.mousePressEvent should emit sig_alt_left_mouse_pressed which is connected to ScrollFlagArea.mousePressEvent, and I don't see any configuration options to disable that.

It should also be possible to prevent underlining links while Ctrl+Alt is held by changing the mouseMoveEvent in CodeEditor.

Good idea, but we should leave it for another PR.

If I'm going to edit the mouse click interaction to change it to Alt+Click I kinda want to re-factor anyway (to move the extra cursor creation into MultiCursorMixin from CodeEditor.mousePressEvent), and it's a small change to basically prevent overlapping if-conditions with multiple modifiers held. It's the same case with mouseMoveEvent.

Unfortunately, Shift+Alt+Up/Down are already taken by duplicate line up/down on Linux/macOS, so they need to be remapped. Let's leave them as CTRL+Alt+PgUp/PgDown for all OSes (notice the capital CTRL, which maps to Cmd on Mac).

That also requires bumping the CONF_VERSION number (read the comment above it in spyder/config/main.py to learn how to do it).

I can put that in, but the config version note specified if simply adding new items, a version bump was not needed. Does the minor version bump apply to changes between un-merged commits too? I'm also a bit unclear based on your wording what you intend for the default to be: 'editor/add cursor up': CTRL + '+Alt+PgUp' or 'Ctrl+Alt+PgUp'.

I did also add the Ctrl+Alt+Shift+Click which adds a column of cursors (one on each row) from the primary cursor to the click location. I was trying to somewhat emulate the Notepad++ function of Alt click and drag to create a box selection.

It seems you forgot to push that change because I can't see it here.

This was a feature I added early-on. It is implemented in CodeEditor.mousePressEvent (now moved to MultiCursorMixin with last refactor commit 1ec8b41) along with adding a single cursor with Ctrl+Alt+Click

Media1.mp4

@ccordoba12
Copy link
Member

ccordoba12 commented Jan 9, 2025

That's surprising to me, as currently CodeEditor.mousePressEvent should emit sig_alt_left_mouse_pressed which is connected to ScrollFlagArea.mousePressEvent

Yep, I saw that. But you can try it yourself: doing Alt+Click over the scrollflag area does nothing for me (it doesn't work in Spyder 5 either).

and I don't see any configuration options to disable that.

Yeah, it's hard-coded. And I think it should remain hard-coded when you change it to add multicursors.

If I'm going to edit the mouse click interaction to change it to Alt+Click I kinda want to re-factor anyway (to move the extra cursor creation into MultiCursorMixin from CodeEditor.mousePressEvent), and it's a small change to basically prevent overlapping if-conditions with multiple modifiers held. It's the same case with mouseMoveEvent.

Ok, if you think it's better, then please do it here.

I can put that in, but the config version note specified if simply adding new items, a version bump was not needed. Does the minor version bump apply to changes between un-merged commits too?

Since the current shortcuts for duplicate line up/down will be changed to new settings, CONF_VERSION needs a minor version bump, i.e. to 85.1.0

I'm also a bit unclear based on your wording what you intend for the default to be: 'editor/add cursor up': CTRL + '+Alt+PgUp' or 'Ctrl+Alt+PgUp'.

CTRL + '+Alt+PgUp' and CTRL + '+Alt+PgDown'

This was a feature I added early-on. It is implemented in CodeEditor.mousePressEvent

Ok, that's really cool, thanks!

@athompson673
Copy link
Contributor Author

athompson673 commented Jan 9, 2025

Alt-Click is absolutely working fine for me, and I've been using it for a long time.. That's why I initially didn't overwrite it, and chose Ctrl-Alt-Click to use for multicursor:

Media2.mp4

Regarding shortcuts: I didn't realize you meant to change those shortcuts as well, I thought you were referring to changing the shortcuts to add a cursor up and down so it wouldn't conflict.

Just to be sure; This is what you had in mind?

'editor/duplicate line up': (CTRL + "+Alt+PgUp"),
'editor/duplicate line down': (CTRL + "+Alt+PgDown"),
...
'editor/add cursor up': 'Alt+Shift+Up',
'editor/add cursor down': 'Alt+Shift+Down',

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add multiline editing to the Editor
3 participants