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

Keyboard shortcut editor enhancements #2523

Merged
merged 18 commits into from
Jul 27, 2015
Merged

Keyboard shortcut editor enhancements #2523

merged 18 commits into from
Jul 27, 2015

Conversation

goanpeca
Copy link
Contributor

@goanpeca goanpeca commented Jul 9, 2015

Fix #2475
Fix some pending issues from #2367


"Fuzzy" string matching

image

image

image

Todo

  • Fix docstrings
  • Change toggle behavior in shortcut dialog
  • Add fuzzy sorting
  • Change information display for conflicting shortcuts
  • Handle sequences bigger than 4 keys
  • Relocate HTMLDelegate to widgets/helperwidgets.py

For another PR (when I have access to a Mac...)

  • Fix text color on selected text (in Mac)
  • Fix dead strokes input

@goanpeca
Copy link
Contributor Author

@SylvainCorlay can you give me an example of what happens with the dead stroke issues?

What type of keyboard layout you have in your pc... OS.. etc..

Cheers

@ccordoba12
Copy link
Member

@goanpeca: One example that @SylvainCorlay showed me in person: pressing Ctrl + <tilde> + e where <tilde> is the key to get é on the screen. That's a dead key :-)

@SylvainCorlay
Copy link
Member

@goanpeca if you use a US-international keyboard layout, the following key sequences do

``` then e -> `è`
`^` then `e` -> `ê`
`'` then `e` -> `é`

meaning that the first character is a dead key and does not do anything by itself. If you just want a quote, you need to press the space bar.

If you use a Spanish layout, I think that ^ is already a dead key.

I don't think that we really need to support dead keys for keyboard shortcuts. Although in the French layout, things like é are a single key stroke, and should not create any problem.

@goanpeca
Copy link
Contributor Author

I cannot reproduce that behavior.

On ubuntu with US-International, if I press Control+' and (while holding control) press "e", I get
Ctrl+'; Ctrl+e

@goanpeca goanpeca changed the title [WIP] Fix/add shortcut editor minor issues Fix/add shortcut editor minor issues Jul 11, 2015
@ccordoba12
Copy link
Member

@goanpeca, do you have a Latin American keyboard to test? This is what I'm getting with Ctrl+<tilde>+e:

Ctrl+ោ

@goanpeca
Copy link
Contributor Author

Not right now...

@ccordoba12
Copy link
Member

Ok, no prob. We can check it out week when we meet in Colombia :-)

@goanpeca
Copy link
Contributor Author

Any comments on the search order screenshots... ?

from spyderlib.utils.qthelpers import get_std_icon
from spyderlib.utils.stringmatching import get_search_regex, get_search_scores
from spyderlib.widgets.arraybuilder import HelperToolButton
Copy link
Member

Choose a reason for hiding this comment

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

This helper button should be moved to helperwidgets too. But this is just a reminder, let's leave it for another PR :-)

@ccordoba12
Copy link
Member

@goanpeca, let's leave the dead keystroke task for another PR.

@blink1073, would you mind to take a look at the fuzzy matching code introduced by @goanpeca in utils/stringmatching.py?

Parameters
----------
query : str
String with letters to seacrh in choice (in order of appearance).
Copy link
Contributor

Choose a reason for hiding this comment

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

search*

String with letters to seacrh in choice (in order of appearance).
choice : string
Sentences/words in which to search for the 'query' letters.
ignore_case : True
Copy link
Contributor

Choose a reason for hiding this comment

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

ignore_case : boolean, optional
       Perform a case insensitive search (True by default).

@ccordoba12
Copy link
Member

@goanpeca, please address @blink1073 and my comments so that we can merge this for beta1. I think we need it because it has very important improvements.

@ccordoba12 ccordoba12 changed the title Fix/add shortcut editor minor issues Keyboard shortcut editor enhancements Jul 23, 2015
@goanpeca
Copy link
Contributor Author

@ccordoba12 I made the corrections, can you test on your side?

Cheers

@ccordoba12
Copy link
Member

@goanpeca, I have a patch (against your latest commit in this branch) that solves all issues I have right now with the ShortcutEditor dialog:

@@ -139,7 +139,8 @@ class ShortcutEditor(QDialog):
         self.text_new_sequence = CustomLineEdit(self)
         self.text_new_sequence.setPlaceholderText(sequence)
         self.helper_button = HelperToolButton()
-        self.label_warning = QLabel('<br>')
+        self.label_warning = QLabel()
+        self.label_warning.hide()

         bbox = QDialogButtonBox(QDialogButtonBox.Ok | QDialogButtonBox.Cancel)
         self.button_ok = bbox.button(QDialogButtonBox.Ok)
@@ -164,7 +165,7 @@ class ShortcutEditor(QDialog):
         self.label_warning.setFocusPolicy(Qt.NoFocus)

         # Layout
-        spacing = 24
+        spacing = 5
         layout_sequence = QGridLayout()
         layout_sequence.addWidget(self.label_info, 0, 0, 1, 3)
         layout_sequence.addItem(QSpacerItem(spacing, spacing), 1, 0, 1, 2)
@@ -267,8 +268,8 @@ class ShortcutEditor(QDialog):

             if len(self.keys) == 1 and key == Qt.Key_Escape:
                 self.set_sequence('')
-                self.toggle_state()
-                return
+                self.label_warning.setText(_("Please introduce a different "
+                                             "shortcut"))

             if len(self.keys) == 1 and key in [Qt.Key_Return, Qt.Key_Enter]:
                 self.toggle_state()
@@ -278,7 +279,7 @@ class ShortcutEditor(QDialog):
                 self.nonedit_keyrelease(e)
             else:
                 debug_print('keys: {}'.format(self.keys))
-                if self.keys:
+                if self.keys and key != Qt.Key_Escape:
                     self.validate_sequence()
                 self.keys = set()
                 self.key_modifiers = set()
@@ -304,13 +305,14 @@ class ShortcutEditor(QDialog):

         if warning_type == NO_WARNING:
             warn = False
-            tip = '<br><br>'
+            tip = ''
         elif warning_type == SEQUENCE_CONFLICT:
             template = '<i>{0}<b>{1}</b></i>'
             tip_title = _('The new shorcut conflicts with:') + '<br>'
             tip_body = ''
             for s in conflicts:
                 tip_body += ' - {0}: {1}<br>'.format(s.context, s.name)
+            tip_body = tip_body[:-4]  # Removing last <br>
             tip = template.format(tip_title, tip_body)
             warn = True
         elif warning_type == SEQUENCE_LENGTH:
@@ -326,6 +328,7 @@ class ShortcutEditor(QDialog):
             warn = True

         if warn:
+            widget_message.show()
             widget.setIcon(get_std_icon('MessageBoxWarning'))
         else:
             widget.setIcon(get_std_icon('DialogApplyButton'))

This patch:

  1. Fixes the spacing issues in the dialog (i.e. a huge empty space between the shortcut line edit and the buttons).
  2. Uses Esc to reset a conflicting shortcut, and let the user introduce a new one at the same time.

I'm fine with hidding label_warning at first and then showing it when a conflict appears.

@ccordoba12
Copy link
Member

Forget about my last patch, I have other improvements so I'll do a PR against yours :-)

@goanpeca
Copy link
Contributor Author

Ok, please go ahead!

Should I make a new PR for the plugins (the pending @Nodd pr?)

@ccordoba12
Copy link
Member

Yes, please! ;-) We need to start moving on that front ASAP, but remember that that's work for beta2.

@goanpeca
Copy link
Contributor Author

So what is missing for beta1 at this point?

@ccordoba12
Copy link
Member

Take a look at our shared Google doc. There is the list of missing things.

@goanpeca
Copy link
Contributor Author

Got it...

@goanpeca
Copy link
Contributor Author

Ok, feel free to merge this one then :-)

ccordoba12 added a commit that referenced this pull request Jul 27, 2015
Keyboard shortcut editor enhancements
@ccordoba12 ccordoba12 merged commit 0a96c43 into spyder-ide:master Jul 27, 2015
@goanpeca goanpeca deleted the shortcut-editor branch August 3, 2015 04:35
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.

4 participants