Skip to content
This repository

Reworking qtconsole shortcut, add fullscreen #776

Merged
merged 1 commit into from over 2 years ago

4 participants

Matthias Bussonnier Fernando Perez Evan Patterson Min RK
Matthias Bussonnier
Collaborator

Hi,
As I really like sometime to work in fullScreen (not Maximized), i add this option to the qtconsole through the shortcut ctrl+alt+space.

I also Find that the if-elsif-..-else way to deal with shortcut is a bit complex to read, so i'm rearranging it with {Qt.Key_X: function to call} dictionnary

i'll do the same for ctrl+alt+shift, ctrl+shift ... etc

I conclude with a question about those same shortcut:
the "pressing return/Enter" event intercept most of the action and prevent futur treatment by ctrl_down, or alt_down cases.
Do you mind if I change this beaviour ?

Fernando Perez
Owner

This looks fine to me, but the Qt code is mostly @epatters authorship, so I'd like to get his input on this PR before taking any action.

Evan Patterson
Collaborator

Thanks, @fperez. I meant to comment on this earlier, but it slipped through the cracks...

@Carreau, Thanks for the contribution! This is a nice feature, but I have a couple comments about the implementation:

  1. Full-screen support is an application-level feature, not a widget-level one. As such, this should be implemented in IPython.frontend.qt.console.qtconsoleapp, not in the ConsoleWidget itself. Using a QShortcut is the simplest way to do this.

  2. I know that the if-then-else logic in the event handler is a bit convoluted. It's certainly the worst part of the Qt console code base and I hope to eventually replace it with a flexible and user-customizable keybinding system. For the time being, though, I'd prefer to keep it as is. It's quite well tested, but not unit-tested at all, so the cost of refactoring is high.

  3. You have merged the master branch into your feature branch several times. Unless there is a really compelling reason to do this, please avoid it, since it clutters up the commit history.

Fernando Perez
Owner

@epatters, many thanks for pitching in! @Carreau, feel free to simply rebase and do a forced push on this branch to remove the master merges (I didn't read the commit log carefully enough, I should have pointed that issue right away!).

Once you address the issues raised by @epatters, we should be able to move forward with this.

Matthias Bussonnier
Collaborator

Hi,
Sorry if the implementation is not in the right place, i'll try to move where it belongs.
I'll rebase and force push when possible.

I'll try to find how to write unit-testing for the shortcut in qtconsole.

Evan Patterson
Collaborator

@Carreau, generally you shouldn't have to worry about rebasing or merging in the master branch, unless you actually need some new change from the master branch. For a small change like this one, a single merge once the PR is ready is usually sufficient, and GitHub handles this automatically.

Also, don't worry about trying to unit test this. I know we are deficient in this regard, but it's a larger project that will require some thought about how to effectively test a complex widget like this one.

Matthias Bussonnier
Collaborator

So I unmerged the master branch, and put the code in IPython.frontend.qt.console.qtconsoleapp and connect it to Ctrl+Meta+Space.

I've done the same with minimized (Ctrl+m) and maximized (Ctrl+Shift+M)

I forced-pushed the branch

Evan Patterson
Collaborator

Great, this is almost ready for merge. Thank you for reworking this. A few minor comments:

  1. I'm on the fence about providing keybindings for maximize and minimize. Shouldn't this be the responsibility of the user's window manager? Many window managers will already have global keybindings for this. @fperez, what do you think about this?

  2. I hate to nitpick, but I will nonetheless: there are spaces before your colons, as well as some semicolons. Please try to avoid this.

Also, you should consider adding a note to the %guiref about the keybinding, so that other users have a hope of discovering your new feature. See:

https://github.com/ipython/ipython/blob/master/IPython/core/usage.py#L375

Matthias Bussonnier
Collaborator

Sory for 2. , i'll fix it.
For the keybinding for maximization/minimization, we can do it conditionnaly depending on the platform. But at least on Mac Os, the windows manager doesn't bind any shortcut to minimizing/maximizing.
i'll add a note into the %guiref

Fernando Perez
Owner

re. min/maximizing: I think that should be left alone and for the OS to handle. Sorry if some OSes don't expose keybindings for it, but I don't think individual applications should start messing with the job of the window manager. It's bad enough that 'full screen' is already spelled in a million different ways from application to application, we shouldn't compound that problem by also making application-level bindings for other window management tasks.

In Gnome, it's easy enough to configure keybindings for those (I've done it). There may be OSX extensions/apps that do it there too, but that's not our job.

Matthias Bussonnier add fullscreen support for QtConsole throught shortcut
	add a toggleFullscreen action to qtconsoleapp and connect it to the
	'Ctrl+Meta+Space' shortcut (which is Command+Ctrl+Space on Mac)
	add shortcut description in %guiref

move maximize and minimize shortcut/fonction to another branch
677c8ce
Matthias Bussonnier
Collaborator

I agree that it should be left to the os to handle.

I'll remove all reference to Max/Min imizing and rebase to a have a clean commit.

I'll put this Maximizing/Minimising function in a separate branch, but I think that we still should support it.
IMHO having an application that works out-of-the-box the way you'r expecting it to do is a great deal for many user. Even if it is possible to configure keybinding for the windows manager

We can still have the same discussion about full-screen arguing that with most tilling manager you should be able to get full-screen application even if they doesn't support it.

Evan Patterson
Collaborator

This looks great, @Carreau. Thank you for your contribution!

Regarding the minimize/maximize shortcuts, I'm not sure your comparison is fair. I'm aware of many applications that, for better or worse, include a shortcut for full screen mode, but none that include shortcuts for minimization and maximization. I think we should be trying to conform to UI conventions rather an inventing our own.

Evan Patterson epatters merged commit 08bae62 into from September 15, 2011
Evan Patterson epatters closed this September 15, 2011
Matthias Bussonnier
Collaborator

I totally agree that an application should conform to UI convention for every system it can be installed on.
I just don't agree with the fact that we shouldn't do it on a application level if it's not handled by the window manager.
I do think we have to try to do it in a way so that it conforms to the rest of the system shortcut, of course.
I may have misunderstood the previous discusion, and might have not correctly explain my thought in english.

So speeking about UI,

For now, on OSX, IPython, with the Qt console look like any other python app, ie generic python icon on the task switcher and dock, the 'menu bar' on top of the screen doesn't have any action like closing and else, and the name of the app is just 'Python'

If I implement a conditional 'darwin' only UI modification to the qt console which for example map some shortcut to minimizing/maximizing and put an 'IP[y]:' icon on the task-switcher..etc. does it have a chance to be accepted ?

I'm really happy to give a small contribution to IPython, and I hope i'll be able able to convert more people to it.

Min RK
Owner

The qtconsole is not really an application - it's a widget with the thinnest possible QApplication wrapper to get it running. A real app would add things like menu items, sensible platform icons/controls, etc. (e.g. a nice settings interface for configuration).

We certainly do want a real QtConsole application, and this kind of work could be the beginning of that.

Brian E. Granger ellisonbg referenced this pull request from a commit January 10, 2012
Commit has since been removed from the repository and is no longer available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 1 unique commit by 1 author.

Sep 15, 2011
Matthias Bussonnier add fullscreen support for QtConsole throught shortcut
	add a toggleFullscreen action to qtconsoleapp and connect it to the
	'Ctrl+Meta+Space' shortcut (which is Command+Ctrl+Space on Mac)
	add shortcut description in %guiref

move maximize and minimize shortcut/fonction to another branch
677c8ce
This page is out of date. Refresh to see the latest.
1  IPython/core/usage.py
@@ -416,6 +416,7 @@
416 416
 - ``C-.``: force a kernel restart (a confirmation dialog appears).
417 417
 - ``C-+``: increase font size.
418 418
 - ``C--``: decrease font size.
  419
+- ``C-M-Space``: toggle full screen. (Command-Control-Space on Mac OS X)
419 420
 
420 421
 The IPython pager
421 422
 =================
15  IPython/frontend/qt/console/qtconsoleapp.py
@@ -98,7 +98,7 @@ def __init__(self, app, frontend, existing=False, may_close=True,
98 98
         self._frontend.exit_requested.connect(self.close)
99 99
         self._confirm_exit = confirm_exit
100 100
         self.setCentralWidget(frontend)
101  
-    
  101
+
102 102
     #---------------------------------------------------------------------------
103 103
     # QWidget interface
104 104
     #---------------------------------------------------------------------------
@@ -461,6 +461,19 @@ def initialize(self, argv=None):
461 461
         self.init_kernel_manager()
462 462
         self.init_qt_elements()
463 463
         self.init_colors()
  464
+        self.init_window_shortcut()
  465
+
  466
+    def init_window_shortcut(self):
  467
+        fullScreenAction = QtGui.QAction('Toggle Full Screen', self.window)
  468
+        fullScreenAction.setShortcut('Ctrl+Meta+Space')
  469
+        fullScreenAction.triggered.connect(self.toggleFullScreen)
  470
+        self.window.addAction(fullScreenAction)
  471
+
  472
+    def toggleFullScreen(self):
  473
+        if not self.window.isFullScreen():
  474
+            self.window.showFullScreen()
  475
+        else:
  476
+            self.window.showNormal()
464 477
 
465 478
     def start(self):
466 479
 
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.