Reworking qtconsole shortcut, add fullscreen

[Carreau]

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 ?

[fperez]

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.

[epatters]

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.

[fperez]

@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.

[Carreau]

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.

[epatters]

@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.

[Carreau]

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

[epatters]

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

[Carreau]

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

[fperez]

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.

[Carreau]

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.

[epatters]

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.

[Carreau]

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.

[minrk]

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.