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: Go to Next / Previous console #20521

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

impact27
Copy link
Contributor

Description of Changes

I couldn't find a keyboard shortcut to quickly switch consoles. So I added one.

  • Wrote at least one-line docstrings (for any new functions)
  • Added unit test(s) covering the changes (if testable)
  • Included a screenshot or animation (if affecting the UI, see Licecap)

Issue(s) Resolved

Fixes #

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct:

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Great idea @impact27! I left a couple of small suggestions for you, otherwise looks good to me.

spyder/config/main.py Show resolved Hide resolved
Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>
@ccordoba12 ccordoba12 modified the milestones: v6.0alpha1, v6.0alpha2 Jun 8, 2023
@ccordoba12
Copy link
Member

@impact27, this is not working for me on Linux. I think you need to add these new shortcuts to the create_shortcuts method of shell.py for them to take effect (at least that's what we do with the new tab shortcut and others).

But perhaps @dalthviz knows better about it since he handled this when migrating the console to the new API.

@dalthviz
Copy link
Member

So, if I remember correctly, yes, I think the shortcuts need to be defined at:

def create_shortcuts(self):

Also, checking the PR, I noticed a couple of things that maybe need a check:

  • I think the main_widget attribute that is a QTabWidget instance is defined as tabwidget instead of tabs
  • I think the icon strings need to be all lower case so next and previous instead of Next and Previous
  • And could it be worthy to add the new actions to the console context menu?

@ccordoba12
Copy link
Member

I think the icon strings need to be all lower case so next and previous instead of Next and Previous

I think there's no need for the translation strings to match the action and shortcut names. What's important is that names and shortcuts coincide, which already do.

@dalthviz
Copy link
Member

I think there's no need for the translation strings to match the action and shortcut names. What's important is that names and shortcuts coincide, which already do.

What I meant to say is that to create the action icons, for example with the next action, instead of:

icon=self.create_icon('Next'),

You need to do use:

icon=self.create_icon('next'), 

Note that the icon string is full lower case, otherwise you will not get an icon (the Next icon doesn't exits, it is identified as next). Something similar happens with the previous action.

As it is right now if you add the actions to, for example, the context menu you will get a red X as icon:

nav

Out of topic, but thinking about this, maybe having an enum or class with constants that track the Spyder valid icons identifiers could be worthy?

@ccordoba12
Copy link
Member

Ok, I see. Sorry for the confusion @dalthviz.

@ccordoba12 ccordoba12 modified the milestones: v6.0alpha2, v6.0alpha3 Aug 4, 2023
@ccordoba12 ccordoba12 modified the milestones: v6.0alpha3, v6.0beta1 Nov 17, 2023
@ccordoba12 ccordoba12 modified the milestones: v6.0alpha4, v6.0beta1 Feb 6, 2024
@ccordoba12 ccordoba12 modified the milestones: v6.0alpha5, v6.0beta1 Mar 12, 2024
@ccordoba12 ccordoba12 modified the milestones: v6.0beta1, v6.0beta2 May 10, 2024
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.

None yet

4 participants