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 a Shortcuts Summary window #3464

Merged
merged 9 commits into from
May 10, 2017

Conversation

rlaverde
Copy link
Member

@rlaverde rlaverde commented Sep 27, 2016

Fixes: #3275

Same as #3331 but to the 3.x branch, so It could be merged early

@ccordoba12 ccordoba12 added this to the v3.1 milestone Sep 27, 2016
@ccordoba12
Copy link
Member

@rlaverde, let's have first the work to remove fixed_shortcut before merging this PR :-)

@ccordoba12
Copy link
Member

@rlaverde, could you rebase or merge this with the 3.x branch? It seems you started this work before 3.0 was released, and that's making Travis to fail :-)

@goanpeca
Copy link
Member

@rlaverde could you paste a screenshot just for reference?

Is it finished otherwise?

@goanpeca goanpeca changed the title Add Shortcuts summary window PR: Add Shortcuts summary window Oct 26, 2016
@rlaverde
Copy link
Member Author

@goanpeca This isn't ready, I'm planning to rewrite some of the logic to calculate the maximum amount of lines for column

@jitseniesen
Copy link
Member

Sorry, I have been rather picky 😊

@ccordoba12
Copy link
Member

Besides, the removal of fixed shortcuts have to be merged first :-)

MAX_FONT_SIZE = 16
MIN_FONT_SIZE = 8

class ShortCutsSummaryDialog(QDialog):
Copy link
Member

@ccordoba12 ccordoba12 Nov 30, 2016

Choose a reason for hiding this comment

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

Please name this as ShortcutsSummaryDialog

@ccordoba12
Copy link
Member

@rlaverde, please finish this one this week. After PR #3498, all our shortcuts should appear in this dialog :-)

@@ -0,0 +1,147 @@
# -*- coding: utf-8 -*-
Copy link
Member

Choose a reason for hiding this comment

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

Please name this file as shortcutssummary.py because just shortcuts is a bit vague :-)

@ccordoba12
Copy link
Member

Tests are failing when they run this line

python spyder/widgets/shortcutssummary.py

@ccordoba12 ccordoba12 modified the milestones: v3.2, v3.1 Jan 5, 2017
@ccordoba12
Copy link
Member

@rlaverde, please solve conflicts for this one and finish it by the end of the week.

@ccordoba12 ccordoba12 modified the milestones: v3.2, v3.3 Feb 14, 2017
@rlaverde rlaverde force-pushed the shortcuts_summary branch from 464de6b to fb7a256 Compare May 9, 2017 22:12
@rlaverde
Copy link
Member Author

rlaverde commented May 9, 2017

I tried to dynamically update the dialog to avoid it to be bigger of the screen, but it didn't work, because the size of the layout isn't updated when adding elements :/

I leave the "smart" logic that try to calculate the best amount of elements by column, and added an QScrollArea to prevent that the window will be bigger than the screen

@rlaverde rlaverde requested a review from jitseniesen May 9, 2017 22:19
@ccordoba12
Copy link
Member

Please rebase on top of 3.x and post a screenshot of how things look now.

@ccordoba12
Copy link
Member

The rebase is needed to fix the failures in Circle and Travis.

@rlaverde rlaverde force-pushed the shortcuts_summary branch from fb7a256 to eda8cfc Compare May 10, 2017 13:46
@goanpeca
Copy link
Member

Screenshot???? @rlaverde

@rlaverde
Copy link
Member Author

shortcuts summary

@ccordoba12 ccordoba12 closed this May 10, 2017
@ccordoba12 ccordoba12 reopened this May 10, 2017
@ccordoba12
Copy link
Member

I like it, a lot!! But I don't know if showing this dialog to the full width of the screen is the best option.

@goanpeca, @jitseniesen, what do you think?

Copy link
Member

@jitseniesen jitseniesen left a comment

Choose a reason for hiding this comment

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

Looks good to me. I say we merge this and then we refine how it looks later if necessary.

@jitseniesen
Copy link
Member

But I don't know if showing this dialog to the full width of the screen is the best option.

Full width seems okay to me because the shortcuts won't fit otherwise. Unless we select and display only the "important" shortcuts.

I guess I would prefer it if it scrolled vertically instead of horizontally.

@ccordoba12
Copy link
Member

I guess I would prefer it if it scrolled vertically instead of horizontally.

Yep, me too, but I don't know how we could achieve this.

Ok, let's merge this because it's a great improvement. We can improve it later, as you said :-)

@ccordoba12 ccordoba12 changed the title PR: Add Shortcuts summary window PR: Add a Shortcuts Summary window May 10, 2017
@ccordoba12 ccordoba12 merged commit 8ced72a into spyder-ide:3.x May 10, 2017
ccordoba12 added a commit that referenced this pull request May 10, 2017
@goanpeca
Copy link
Member

@rlaverde its Shortcut not ShortCut, why the camel casing?

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.

4 participants