PR: Add Shortcuts Summary Window #3331

rlaverde · 2016-07-28T20:42:17Z

Summary shortcuts implementation

I'm still working on it, there is some things missing:

register shortcut
calculate font size and amount of elements in a column using screen size

goanpeca · 2016-07-30T00:38:08Z

@spyder-ide/core-developers should we create a 3.x branch so we don't leave opened PRs without supervision for weeks or months until 3.0 is released?

goanpeca · 2016-08-01T22:00:05Z

@Nodd comments?

ccordoba12 · 2016-08-02T18:45:33Z

@jitseniesen, it would be nice to hear from you here too :-)

jitseniesen · 2016-08-03T20:16:47Z

Looks quite decent to me. Well done @rlaverde ! But I do have a couple of points ...

I might have missed something, but it is not clear from the code how the user is going to get this window.
How does this work on a small screen? The window looks rather big and I think it does not always fit.
Can we get rid of the underscore in "Switch to variable_explorer" etc?
Probably not in the scope of this PR, but I find the inconsistency between "Shift+Ctrl+F4" and "Ctrl+Shift+F12" a bit annoying.
What about fixed (not configurable) shortcuts?
Looking at the code, is it necessary to define LABELS? What if a new context is added, e.g. in a plugin (once we get shortcuts in plugins working properly)? I would special case '_' and otherwise just capitalize.

rlaverde · 2016-08-23T21:00:32Z

I might have missed something, but it is not clear from the code how the user is going to get this window.

I haven't implemented this, I'm not sure where I should register this shortcut (gui? editor?), and which should be the shortcut (Ctrl+H maybe?)

How does this work on a small screen? The window looks rather big and I think it does not always fit.

hmm yes, font and windows size should be calculated from screen size (I'm working on this)

Can we get rid of the underscore in "Switch to variable_explorer" etc?

Yes, I added a commit that replace underscores by spaces

Probably not in the scope of this PR, but I find the inconsistency between "Shift+Ctrl+F4" and "Ctrl+Shift+F12" a bit annoying.

Yes ,I also think this should be in another PR, maybe modify iter_shourtcuts() function in spyderlib/config/gui.py

What about fixed (not configurable) shortcuts?

Fixed shortcuts aren't save anywhere, I think this will be another PR too, that add a function iter_fixed_shortcuts() and modify fixed_shortcut to save it in a global variable maybe.

Looking at the code, is it necessary to define LABELS? What if a new context is added, e.g. in a plugin (once we get shortcuts in plugins working properly)? I would special case '_' and otherwise just capitalize.

I just add another commit, I deleted LABELS and special case '_' as you advised

jitseniesen · 2016-08-27T12:47:39Z

I haven't implemented this, I'm not sure where I should register this shortcut (gui? editor?), and which should be the shortcut (Ctrl+H maybe?)

I would make it a global shortcut because the info is about the whole application. Ctrl+H sounds good (I don't think it conflicts with anything), but maybe we want to prefer double modifiers for global shortcuts and keep the single modifier shortcuts as much as possible available for local use. In which case, what about Alt-Shift-H or Ctrl-Shift-K (for keyboard shortcuts)? Ctrl-Shift-H is already taken.

I would also add an item in the help menu, otherwise people will never discover this new feature.

ccordoba12 · 2016-08-27T17:51:04Z

@rlaverde, please merge with master to update your work :-)

…umn using screen size

rlaverde · 2016-09-22T14:54:06Z

@rlaverde, please merge with master to update your work :-)

I rebase it and also squash some commits

I would also add an item in the help menu, otherwise people will never discover this new feature.

I added a new element as you suggested, and define Meta+F1 as the shortcut

ccordoba12 · 2016-09-22T15:51:47Z

@rlaverde, thanks a lot! The error on Appveyor is cause by errors with conda, don't worry about it :-)

jitseniesen

I tried out the code and looked at it in more detail and left some more comments. The one about size is the most important.

jitseniesen · 2016-09-27T11:49:35Z

spyder/app/mainwindow.py

@@ -931,6 +931,10 @@ def create_edit_action(text, tr_text, icon):
        else:
            tut_action = None

+        shortcuts_action = create_action(self, _("Shortcuts Sumary"),


fix spelling of 'Summary'

jitseniesen · 2016-09-27T11:50:34Z

spyder/app/mainwindow.py

+    def show_shortcuts_dialog(self):
+        from spyder.widgets.shortcuts import ShortCutsSummaryDialog
+        dlg = ShortCutsSummaryDialog(None)
+        dlg.show()


I think dlg.show() is not necessary because dlg.exec_() already shows the dialog, no?

jitseniesen · 2016-09-27T12:25:42Z

spyder/widgets/shortcuts.py

+MAX_FONT_SIZE = 16
+MIN_FONT_SIZE = 8
+
+class ShortCutsSummaryDialog(QDialog):


Please add a docstring with some information about the dialog, what it contains and what it looks like.

jitseniesen · 2016-09-27T12:26:27Z

spyder/widgets/shortcuts.py

+MIN_FONT_SIZE = 8
+
+class ShortCutsSummaryDialog(QDialog):
+    def __init__(self, parent=None, offset=0, force_float=False):


The last two arguments are not used; can they be deleted?

jitseniesen · 2016-09-27T12:30:28Z

spyder/widgets/shortcuts.py

+        width, height = self.get_screen_resolution()
+        font_size = height / 80
+        font_size = max(min(font_size, MAX_FONT_SIZE), MIN_FONT_SIZE)
+        shortcuts_column = (height - 8 * font_size) / (font_size +16)


This computation results in a window that is too tall on my laptop screen, which has:

width = 1366 height = 737 font_size = 9.2125 shortcuts_column = 26.30837878036688

The resulting window has height 771 (and width 1157).

I don't know how I could fix this, my laptop have similar dimensions, and the resulting height is 703, there's another way to add elements and avoid oversizing the window?

width = 1366
height = 738
font_size = 9.225
shortcuts_column = 26.3310

I don't know. I don't understand where you got the formula for shortcuts_column from. Maybe I have different fonts installed and the font I end up with has a different size?

Perhaps it's best not to be too smart. Pack all items in four columns of (rougly) equal size, and add a scroll bar if it ends up bigger than the screen.

Alternatively, add the items to the column one-by-one and measure the height of the resulting column; hopefully there is a field .height in Qt, but I don't know whether this is possible without displaying it on the screen, and it may be too much effort for the first iteration.

I'll try the second approach, if that doesn't work I'll implement the other one

jitseniesen · 2016-09-27T12:34:34Z

spyder/widgets/shortcuts.py

+"""Shortcut Summary dialog"""
+
+# TODO:
+# calculate windows size from screen size


Is this TODO comment out of date?

jitseniesen · 2016-09-27T12:35:02Z

spyder/widgets/shortcuts.py

+#
+# Copyright © 2009- The Spyder Development Team
+# Licensed under the terms of the MIT License
+# (see spyderlib/__init__.py for details)


Replace spyderlib by spyder

jitseniesen · 2016-09-27T12:42:05Z

I forgot to say the most important thing: It does work, and it seems almost ready.

By the way, I wonder whether we need to include a close button or something like that. At the moment, the user needs to press Esc to close the window, which is not totally obvious.

rlaverde · 2016-09-27T17:31:07Z

@jitseniesen For advise of @ccordoba12 I open a new PR to the 3.x branch, I will fix the things that you commented in the new PR

goanpeca changed the title ~~[WIP] Add Shortcuts Summary Window~~ PR: [WIP] Add Shortcuts Summary Window Jul 28, 2016

ccordoba12 added this to the v3.1 milestone Jul 28, 2016

goanpeca assigned Nodd Aug 1, 2016

goanpeca assigned jitseniesen Aug 2, 2016

rlaverde added 2 commits September 19, 2016 16:13

Add shortcuts Summary Window

7c5fe15

Add menu item and shortcut for shortcuts summary

7e76ab9

rlaverde force-pushed the shortcuts_summary branch from 28db7af to 7e76ab9 Compare September 19, 2016 21:14

Shortcuts Summary: Calculate font_size and amount of elements per col…

a377e24

…umn using screen size

rlaverde changed the title ~~PR: [WIP] Add Shortcuts Summary Window~~ PR: Add Shortcuts Summary Window Sep 22, 2016

jitseniesen reviewed Sep 27, 2016

View reviewed changes

rlaverde mentioned this pull request Sep 27, 2016

PR: Add a Shortcuts Summary window #3464

Merged

rlaverde closed this Sep 27, 2016

ccordoba12 removed this from the v3.1 milestone Sep 27, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PR: Add Shortcuts Summary Window #3331

PR: Add Shortcuts Summary Window #3331

rlaverde commented Jul 28, 2016 •

edited

Loading

goanpeca commented Jul 30, 2016

goanpeca commented Aug 1, 2016

ccordoba12 commented Aug 2, 2016

jitseniesen commented Aug 3, 2016

rlaverde commented Aug 23, 2016 •

edited

Loading

jitseniesen commented Aug 27, 2016

ccordoba12 commented Aug 27, 2016

rlaverde commented Sep 22, 2016

ccordoba12 commented Sep 22, 2016

jitseniesen left a comment

jitseniesen Sep 27, 2016

jitseniesen Sep 27, 2016

jitseniesen Sep 27, 2016

jitseniesen Sep 27, 2016

jitseniesen Sep 27, 2016

rlaverde Sep 27, 2016

jitseniesen Sep 27, 2016

rlaverde Sep 30, 2016 •

edited

Loading

jitseniesen Sep 27, 2016

jitseniesen Sep 27, 2016

jitseniesen commented Sep 27, 2016

rlaverde commented Sep 27, 2016

PR: Add Shortcuts Summary Window #3331

PR: Add Shortcuts Summary Window #3331

Conversation

rlaverde commented Jul 28, 2016 • edited Loading

goanpeca commented Jul 30, 2016

goanpeca commented Aug 1, 2016

ccordoba12 commented Aug 2, 2016

jitseniesen commented Aug 3, 2016

rlaverde commented Aug 23, 2016 • edited Loading

jitseniesen commented Aug 27, 2016

ccordoba12 commented Aug 27, 2016

rlaverde commented Sep 22, 2016

ccordoba12 commented Sep 22, 2016

jitseniesen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rlaverde Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jitseniesen commented Sep 27, 2016

rlaverde commented Sep 27, 2016

rlaverde commented Jul 28, 2016 •

edited

Loading

rlaverde commented Aug 23, 2016 •

edited

Loading

rlaverde Sep 30, 2016 •

edited

Loading