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 Ctrl+Shift+Enter to enter a new line in the Editor #6423

Merged
merged 3 commits into from
Mar 3, 2018
Merged

PR: Add Ctrl+Shift+Enter to enter a new line in the Editor #6423

merged 3 commits into from
Mar 3, 2018

Conversation

lukruh
Copy link
Contributor

@lukruh lukruh commented Feb 9, 2018

Fixes #5466

A keybind to start a new line without breaking the current (bevore it ends) is included in many editors. This is very handy for people using autocompletition for brackets

Many editors offer the possibility to create new lines without breaking that one the cursor is currently in. This is very handy for people using autocompletition for brackets for example.
@pep8speaks
Copy link

pep8speaks commented Feb 9, 2018

Hello @Sarbot! Thanks for updating the PR.

Line 981:1: W293 blank line contains whitespace

Line 651:80: E501 line too long (81 > 79 characters)

Comment last updated on February 10, 2018 at 13:56 Hours UTC

@lukruh lukruh changed the base branch from master to 3.x February 9, 2018 22:57
to pass the codestyle check
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Can you write a simple unit test for this behavior? Also, can you make the shortcut configurable? I currently use Shift-Ctrl-Enter for run cell, as I'm not a fan of the defaults.

@lukruh
Copy link
Contributor Author

lukruh commented Feb 10, 2018

I'll try to do that in the next days.

@lukruh
Copy link
Contributor Author

lukruh commented Feb 10, 2018

To include this as shortcut instead of hard coding the keybinding, I guess it needs to be included in the create_shortcuts(self) function in widgets/sourcecode/codeeditor.py, doesn't it? But where do I have to define the associated function? For example there is a shortcut definition like

        movelinedown = config_shortcut(self.move_line_down, context='Editor',
                                   name='Move line down', parent=self)

I found a definition in base.py is that the place where I can define a new function?
Just want to be sure, since this is my first view into the spyder sourcecode.

@jnsebgosselin
Copy link
Member

@Sarbot Thanks for your contribution! Is this PR fixing #5466?

@lukruh
Copy link
Contributor Author

lukruh commented Feb 10, 2018

Yes that's exactly what it does, but I still need to fix the auto indent after the line break.
This is the situation for just pressing Return codeeditor.py (line 2809+)

if self.add_colons_enabled and self.is_python_like() and \
              self.autoinsert_colons():
                self.textCursor().beginEditBlock()
                self.insert_text(':' + self.get_line_separator())
                self.fix_indent()
                self.textCursor().endEditBlock()
            elif self.is_completion_widget_visible() \
               and self.codecompletion_enter:
                self.select_completion_list()
            else:
                # Check if we're in a comment or a string at the
                # current position
                cmt_or_str_cursor = self.in_comment_or_string()

                # Check if the line start with a comment or string
                cursor = self.textCursor()
                cursor.setPosition(cursor.block().position(),
                                   QTextCursor.KeepAnchor)
                cmt_or_str_line_begin = self.in_comment_or_string(
                                            cursor=cursor)

                # Check if we are in a comment or a string
                cmt_or_str = cmt_or_str_cursor and cmt_or_str_line_begin

                self.textCursor().beginEditBlock()
                TextEditBaseWidget.keyPressEvent(self, event)
                self.fix_indent(comment_or_string=cmt_or_str)
                self.textCursor().endEditBlock()

But I'm not sure if/how I could apply this indentation to the new function in base.py (line 977+)

def go_to_new_line(self):
    """Go to the end of the current line and create a new line"""
    self.stdkey_end(False, False)
    self.insert_text(self.get_line_separator())

Appart from this the latest commit is working for me, the keybinding can be changed in settings.

@jnsebgosselin
Copy link
Member

jnsebgosselin commented Feb 10, 2018

Alright, tyvm, this is a great addition!

I've edited your OP by adding "Fixes #5466", so that this PR closes Issue #5466 when it is merged.

@ccordoba12 ccordoba12 changed the title keybinding ctr+shift+enter to enter a new line PR: Add Ctrl+Shift+Enter to enter a new line in the Editor Feb 10, 2018
@ccordoba12
Copy link
Member

Great job so far @Sarbot! The only thing missing is a test, as @CAM-Gerlach pointed out.

Don't worry about the failing test, it's unrelated to your work.

@ccordoba12
Copy link
Member

am I right in assuming that something will break in my setup, either this functionality or the ability to use the proper shortcut for "Run Cell"?

I don't know, probably. Could you test it locally and report back?

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Feb 10, 2018

Nvm @ccordoba12 , he added the configurable shortcut—my bad, didn't see that when I was writing my original comment. Thanks @Sarbot ! I'll test it anyway just to make sure.

@CAM-Gerlach
Copy link
Member

Okay, tested it out. Starting out, neither function works; Ctrl-Shift-Enter does nothing. However, after I go to prefs, where I'm warned about the conflicting shortcut, and change it to something unique, this function works just fine as expected (and so does run cell). Thanks!

@lukruh
Copy link
Contributor Author

lukruh commented Feb 11, 2018

Is ctrl+shift+Return a spyder default, or common keybinding to run the cell or just you personal configuration? The default setting for the newline function can be what ever you want.
I've never written tests in python before but I'm going to check the tests for other functions and try to understand it.

@CAM-Gerlach
Copy link
Member

BTW, if you rebase on the latest 3.x and then push -f, it should fix the test failure.

Is ctrl+shift+Return a spyder default, or common keybinding to run the cell or just you personal configuration?

@Sarbot Nope, its not a default, though it is for Rstudio (and possibly others) which we plan to implement as a built-in hotkey preset in the upcoming Spyder 4. Once it is reset to something unique (which is possible thanks to you setting up the configurable shortcut), everything works perfectly, though if other users employ it, it could cause some confusion at first However, this is really more of a broader UX question of whether and how to better handle shortcuts that are added in a new release that conflict with existing added ones, and isn't really the fault of this PR at all. Thanks for checking!

I've never written tests in python before but I'm going to check the tests for other functions and try to understand it.

It shouldn't be too tough especially since we're using pytest; I never had either not to long ago and it wasn't too hard to learn at all. There should be plenty of good examples to base it off, but my first guess at the overall approach would be just creating a new editor with a few lines of example text, and then testing that the shortcut has the desired effect (e.g. triggering it via qtbot.keyClick, and/or by directly calling the function).

@ccordoba12
Copy link
Member

This is nice and a test for it seems too much of a burden. Let's merge it and see how it goes.

@ccordoba12 ccordoba12 merged commit 6f4b158 into spyder-ide:3.x Mar 3, 2018
ccordoba12 added a commit that referenced this pull request Mar 3, 2018
@lukruh lukruh deleted the 3.x branch March 10, 2018 23:47
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.

5 participants