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 key binding Ctrl+[ #89

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ryohei22
Copy link
Contributor

@ryohei22 ryohei22 commented Jul 12, 2023

Description of the change

Added key binding ctrl + [ to enter normal mode instead of esc key.

Issue(s) Resolved

Part of #85

Comments

I didn't check how it works with Windows.

@ryohei22 ryohei22 changed the title Add key binding 'ctrl+[' PR: Add key binding 'ctrl+[' Jul 12, 2023
@ccordoba12
Copy link
Member

@ryohei22, please rebase on top of our latest master to get the fix to our tests.

@ccordoba12 ccordoba12 added this to the v0.2.0 milestone Jul 12, 2023
@ccordoba12 ccordoba12 changed the title PR: Add key binding 'ctrl+[' PR: Add key binding Ctrl+[ Jul 12, 2023
Copy link
Collaborator

@ok97465 ok97465 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 the PR and sorry for the late review.
I'm only available for reviews on weekends.

I have a simple code suggestion for you.

# Standard library imports
import sys

Please add the above sentence above the "# Third party imports" syntax.

Comment on lines +71 to +75
sc2 = QShortcut(
QKeySequence(Qt.META + Qt.Key_BracketLeft),
editor.editorsplitter,
self.vim_cmd.commandline.setFocus)
sc2.setContext(Qt.WidgetWithChildrenShortcut)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
sc2 = QShortcut(
QKeySequence(Qt.META + Qt.Key_BracketLeft),
editor.editorsplitter,
self.vim_cmd.commandline.setFocus)
sc2.setContext(Qt.WidgetWithChildrenShortcut)
modifier = "Meta" if sys.platform == "darwin" else "Ctrl"
sc2 = QShortcut(
QKeySequence(f"{modifier}+["),
vim_cmd.editor_widget.editorsplitter,
vim_cmd.commandline.setFocus)
sc2.setContext(Qt.WidgetWithChildrenShortcut)

@ok97465
Copy link
Collaborator

ok97465 commented Jul 15, 2023

@ccordoba12

However, the above code does not work in the window and linux environments. I think Spyder's Codeeditor skips the CTRL key.

        if key in [Qt.Key_Control, Qt.Key_Shift, Qt.Key_Alt,
                   Qt.Key_Meta, Qt.KeypadModifier]:
            # The user pressed only a modifier key.
            if ctrl:
                pos = self.mapFromGlobal(QCursor.pos())
                pos = self.calculate_real_position_from_global(pos)
                if self._handle_goto_uri_event(pos):
                    event.accept()
                    return


                if self._handle_goto_definition_event(pos):
                    event.accept()
                    return
            return

Removing the duplicate CTRL+[ shortcut and commenting out the code above will make Ctrl+[ work in spyder-vim.

@ccordoba12
Copy link
Member

Removing the duplicate CTRL+[ shortcut

Perhaps the best solution to deal with this would be to show a dialog the first time this plugin is loaded that asks users if they want to unset all conflicting shortcuts so that Spyder-Vim can work as expected (I'm sure this won't be the only one).

and commenting out the code above will make Ctrl+[ work in spyder-vim.

That's not a proper solution. Perhaps we need to add an editor extension here to deal with keyPressEvent's (which would override what's done in CodeEditor), or find a way to propagate the Ctrl event in the code you mentioned.

@ok97465
Copy link
Collaborator

ok97465 commented Jul 16, 2023

@ccordoba12 I think this PR is to add CTRL+[on Mac only, and CTRL+[for windows and linux should be considered in another PR.

@ryohei22
Copy link
Contributor Author

So, in the end, is it okay to just include @ok97465 's suggestion?

@ccordoba12
Copy link
Member

So, in the end, is it okay to just include @ok97465 's #89 (comment)?

The thing is these changes wouldn't work on Windows and Linux, as @ok97465 pointed out. So perhaps it'd be better to investigate how to port this plugin to use an Editor extension, so that it has full control over the key presses on it. Otherwise, you'd keep finding conflicts between the Spyder default keyboard shortcuts and the Vim ones.

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.

3 participants