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: Copy/cut entire line if nothing is selected (Editor) #22480

Merged
merged 9 commits into from
Nov 6, 2024

Conversation

The-Ludwig
Copy link
Contributor

@The-Ludwig The-Ludwig commented Sep 10, 2024

Description of Changes

  • Wrote at least one-line docstrings (for any new functions)
  • Added unit test(s) covering the changes (if testable)
  • Included a screenshot or animation (if affecting the UI, see Licecap)
Kooha-2024-09-10-17-26-08.webm

This addition allows to copy and cut a whole line in the editor, with Ctr+C and Ctr+X as suggested in #21264.
This is similar to the behavior e.g. VSCode has.

Why select_current_line_and_sep is needed.

The most basic implementation is very straightforward (look after my first two commitss e08c076 and fdfd925). This PR is a bit longer, since we usually want to include the lineseperator from the current line, not the lineseperator from the previous line when copying/cutting whole lines. The exception there is if there is only one line or we are at the last line of the document.

Differences to standard editors like VSCode

This feature is already quite usable and improves my personal workflow greatly. To get exactly the same behavior as VSCode, CodeEditor.Paste needs to be modified. VSCode remembers if the clipboard was filled by not having anything selected and then selecting the whole line, and then on pasting inserts the line above the current line and adds the lineseperator at the end correctly.
I do not know what best-practise to save a state in this codebase is and the implementation of CodeEditor.Paste is a bit above my head right now. I need more experienced spyder developers to look at this in an upcoming PR.

Issue(s) Resolved

Fixes #21264
Fixes #8574

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:
Ludwig Neste

@dalthviz dalthviz changed the title Copy/cut entire line if nothing is selected PR: Copy/cut entire line if nothing is selected (Editor) Sep 16, 2024
@dalthviz dalthviz added this to the v6.1.0 milestone Sep 16, 2024
@The-Ludwig
Copy link
Contributor Author

I do not understand why the fast linux tests pass, while the slow one don't. Any idea?

@ccordoba12
Copy link
Member

Hey @The-Ludwig, thanks a lot for your contribution! And sorry for the late reply, we were quite busy fixing several bugs that surfaced after releasing 6.0.0.

@dalthviz, could you review this one give @The-Ludwig a hand with our tests? Thanks!

@dalthviz
Copy link
Member

dalthviz commented Oct 1, 2024

So, I gave the CI failure a check and seems like the failing test was being forced to pass by setting text in the clipboard directly. The test fails since the changes here cause that calling copy without a selection actually copy things. Created #22618 to fix the failing test. When that gets merged, this PR can be updated with latest master and hopefully all tests should pass

@ccordoba12
Copy link
Member

@The-Ludwig, please merge with master to get the fix done by @dalthviz in PR #22618.

@The-Ludwig
Copy link
Contributor Author

@The-Ludwig, please merge with master to get the fix done by @dalthviz in PR #22618.

Did so! :)

@The-Ludwig
Copy link
Contributor Author

@dalthviz @ccordoba12 is this mergeable?

@ccordoba12
Copy link
Member

@dalthviz, please give @The-Ludwig's work a look to see if we can merge it.

Copy link
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

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

Thank you @The-Ludwig for all your work here and sorry for the late review! Gave a check locally to this and I would say it works nicely 👍

The only nitpick I have is to add in the new select_current_line_and_sep method docstring a description of its params (cursor and set_cursor). Other than that, and since the docstring is quite descriptive as it is, this LGTM so leaving approved 👍

If you think is ready from your side, feel free to merge this @ccordoba12

@ccordoba12
Copy link
Member

If you think is ready from your side, feel free to merge this @ccordoba12

I'm ok with it, so let's merge it! Thanks @The-Ludwig for this great contribution!!

@ccordoba12 ccordoba12 merged commit a672bb7 into spyder-ide:master Nov 6, 2024
17 checks passed
@The-Ludwig
Copy link
Contributor Author

Thanks for merging this and also for the support during development!

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.

Request: copy entire line with CTRL+C and no selection Feature Suggestion: Cut current line
3 participants