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

Qtconsole menu #887

Merged
merged 52 commits into from Oct 20, 2011
Merged

Qtconsole menu #887

merged 52 commits into from Oct 20, 2011

Conversation

fperez
Copy link
Member

@fperez fperez commented Oct 17, 2011

This PR is meant to continue the work started by @Carreau in #813, which was sadly interrupted by a robbery. Since we don't know when @Carreau will be able to resume regular work, I've rebased his branch on top of master (at his last commit it had a small conflict), and I'm putting it up here on a common branch that several of us could in principle work on.

Unfortunately, when this happened the branch was actually in a broken state, because the qtconsole doesn't start, failing with:

/home/fperez/usr/lib/python2.7/site-packages/IPython/frontend/qt/console/qtconsoleapp.py in init_window_shortcut()
   1134                 statusTip="Toggle menubar betwin visible and not",
   1135                 triggered=self.toggle_menu_bar)
-> 1136             self.window_menu.addAction(self.toggle_menu_bar_act)
   1137 
   1138     def toggle_menu_bar(self):
AttributeError: 'IPythonQtConsoleApp' object has no attribute 'window_menu'

I'm not sure what happened, since a few days ago I'd played with the branch and it was broken. I haven't had a chance to bisect it yet.

Basically, the point of putting this PR here is to see whether we can find the resources to finish up @Carreau's work to include it in 0.12. It's a large PR that had very nice functionality, and I would love to have his contribution in 0.12. But it's a lot of complex code that we can't just dump in there unless we're reasonably confident it's in good shape.

@epatters had given @Carreau already a ton of feedback, so you're the best positioned to gauge whether it's just missing a tiny bit of (doable) work or major effort that is not realistic in a short timeframe. I certainly won't take any action without your input.

In summary, before this can be merged, it needs:

  • fixing the current brokenness, since the qt console won't even start.
  • finishing up the remaining comments that were in-flight in the discussion for Create menu bar for qtconsole #813.
  • any other checks/fixes that @epatters might deem necessary.

We'll see if it can be done before 0.12 or not...

	console_widget:
		modifie/create some QAction to be able to put it in menu bar
			-made print_action and export_action public
			-transform increase/decrease/rest front_size
			 to QAction
			-create qaction for undo/redo

	MainWindow
		Create the menubar with the following structure
		And link each menu with its action in console Widget
		File
		|__Print
		|__Save as XML
		|__Select ALL
		Edit
		|__Undo
		|__Redo
		Font
		|__Increase font size
		|__Decresae font size
		|__Restet Font size
		Window
		|__Minimize
		|__Maximize
		|__Full Screen
		Magic
		|__Reset
		|__History
		|__Export History
		|__Who
		|__Whos
		|__Who_ls
		Help
		|_Open online Help

		The 'Magic' Menu, execute %magic after 'exit_keyboard' and pasting
		'%magic'.  The 'Export History' menu prompt the user for a file name
		and a range before pasting `%save filename range`.

	This this also fix a bug in toggle full screen where
	Full Screen -> (try to) Minimizing -> Out Of Full Screen
	is broken (grey background for all screen), at least on OSX
	It's clearly not definitive, as most of the file are not in the right
	location and icon should be graphically improved, so il will certainly be rebased later.
	But as it goes with UI improvemetn in the menubar I add it to this branch.

	Note: About the svg Icon, I've done it from scratch in inkscape, inspired by
	iTerm2 icon, which is under GPL
	the .icns file is OSX Specific.
		Did export the wrong area with inkscape, making the Icons to Have the
		wrong aspect ratio once loaded in qt. Use this excuse to refine
		shadow/lightnig and general color
	Real file chooser menu, get the current session history range by default.
	If the file already exist, the user will be prompted twice if he/she really
	want to be override the existing file ( by the gui, and the magic afterward)
	One might also to add the (-r) %save option into the gui.
	not all haves benn moves as i'm not sure yet how to handle some of them
	working 2 kinds of tab
	with Ctrl+Shift+T -> tab attached on same kernel
	with Ctrl+T -> tab attached on new kernel

	closing event management is way far from the one of the mainWindow
	and "first tab" is still handeled differently as the other ones as some action
	are still attaches to it directly
	tabs are now names "kernel i"

	starting the application with --existing ... use to attached all new tabs
	to the same kernel. New tab attached to new kernel now start a local kernel
	on random port and connect to it.

	rename some variable to be more explicit.
	Note that svg doesn't render the shadow casting in QT, Maybe it's an
	inkscape specific feature
@minrk
Copy link
Member

minrk commented Oct 19, 2011

Okay, I pushed with those changes, and (in a separate commit) gave MainWindow its own file as @epatters suggested.

@fperez
Copy link
Member Author

fperez commented Oct 19, 2011

Great! A few comments:

  • I'd change the menu shortcut to Ctrl-Shift-M instead of Meta. It's the only one using meta, and Ctrl-Shift-M is already used for menubar toggle in apps like konsole.
  • The tab nav keybindings are backwards from the behavior on chrome and firefox: pgup should go left, pgdown should go right.
  • There's a strange focus bug: open several tabs, then close one with C-w. The focus doesn't return to the window so it's impossible to type. I have to switch focus to a different window and back to the console to recover focus. No amount of clicking on the qtconsole helps. A visual cue of the problem is that the blinking cursor disappears.
  • I was wondering if we should change the close action on the whole window to ask about closing all tabs, and shutting them all down if affirmative. Right now if you have 10 tabs open, there's no way to close them all without answering 'yes' 10 times.

Awesome job, Min. This is almost ready to go, I'm excited. It makes the qt console a really appealing environment.

@minrk
Copy link
Member

minrk commented Oct 19, 2011

Sure, sorry - I misread your Ctrl-S-M before for some reason. I'll fix pgup/down as well.

Closing everything with Quit should get a single-confirm, and then close everything or do nothing. I'll look into that, as well as the weird focus bug.

@fperez
Copy link
Member Author

fperez commented Oct 20, 2011

On Wed, Oct 19, 2011 at 3:42 PM, Min RK
reply@reply.github.com
wrote:

Sure, sorry - I misread your Ctrl-S-M before for some reason. I'll fix pgup/down as well.

Great, thx.

Closing everything with Quit should get a single-confirm, and then close everything or do nothing. I'll look into that,

Agreed, that should be the behavior

as well as the weird focus bug.

Thanks. No idea where that came from!

* Single prompt on quit event
* Disabled leaving the kernel running on window close, as it doesn't make as much sense
  now that there are multiple kernel windows
* Tabs respect confirm-exit setting
* Removed some obsolete kwargs from MainWindow.__init__, that are no longer used

Closing logic is still a bit wonky, depending on the ordering of tabs closed (masters, slaves, etc.).

also fix some spelling errors, and remove errant semicolons
@minrk
Copy link
Member

minrk commented Oct 20, 2011

Should be single dialog on app quit / window close, now, and the confirm-exit setting is respected.

Definitely not 100% sure the closing logic is right, as now with possibly many tabs closing out of order, there are a large number of possible cases, and the little mess that I had made with the close dialog when there could be only one window is now pretty tangled.

@fperez
Copy link
Member Author

fperez commented Oct 20, 2011

Looking good...

On Wed, Oct 19, 2011 at 5:44 PM, Min RK
reply@reply.github.com
wrote:

Should be single dialog on app quit / window close, now, and the confirm-exit setting is respected.

Definitely not 100% sure the closing logic is right, as now with possibly many tabs closing out of order, there are a large number of possible cases, and the little mess that I had made with the close dialog when there could be only one window is now pretty tangled.

Looks correct to me so far, great job! Final points:

  • Ctrl/Cmd-Q for quit? And listing it at the bottom of the file menu?
    I think people will expect that now that it's an app.
  • C-S-p for print not working on linux.
  • focus bug remains, I don't know if you'd taken a stab at that...

@minrk
Copy link
Member

minrk commented Oct 20, 2011

Ctrl/Cmd-Q for quit? And listing it at the bottom of the file menu?
I think people will expect that now that it's an app.

I used QKeySequence.Quit, which I figure we should use unless there is conflict. I assume that's Ctrl-Q on Linux.

Of course, it is supposed to already be at the bottom of File, but I inverted the sys.platform != darwin check so I could test on my mac, but never put it back. That should be fixed now.

C-S-p for print not working on linux.
focus bug remains, I don't know if you'd taken a stab at that...

I haven't checked on this, as I've been mostly testing on my Mac. I'll try on Linux after dinner. I haven't seen the focus bug in OSX, and printing works fine.

@minrk
Copy link
Member

minrk commented Oct 20, 2011

The problem with print (and save, select-all, etc.) is that the widget events are still bound to key-combos via the context-menu, and ambiguous events don't resolve (helpful printed error messages, though). I'll investigate.

Re: focus - it looks like the tab itself is highlighted, rather than the IPython area, after closing a tab without confirmation (either slave, or any with --no-confirm-exit). For me, hitting TAB moves focus back to the text area, but I'll look into why it's different.

set some menu bar contexts to Widget, preventing conflict.

Essentially, these events will never be called by shortcut, as the events
for which they are a proxy are already bound to the shortcut, and get it instead.

This is a bit ugly, because the menu shortcuts aren't really active - they depend
on the widget shortcuts being the same.
@minrk
Copy link
Member

minrk commented Oct 20, 2011

Fixed the conflict with a slightly dirty trick of setting the menu bar shortcut context (for those that conflict) to widget-only. This means that they won't ever get invoked by shortcut (but they will display their shortcut keys), and the widget shortcuts actually do all the work.

@minrk
Copy link
Member

minrk commented Oct 20, 2011

disallowing focus in the tab-bar itself seems to have fixed the focus issue.

stop_channels() *must* be called, to prevent fd count rising, but
stop_channels waits up to 0.2s up to 4 times, and it shouldn't get in the
way of the UI.  This just calls stop_channels in a thread, to protect
UI responsiveness.
Problem case: flags and aliases with the same name (e.g. existing)

would attempt removal twice, raising ValueError
@Carreau
Copy link
Member

Carreau commented Oct 20, 2011

So much things in one night...
I've got a new (time-shared-with-my-girlfriend) computer, so I might be able to soon help. just need to reinstall everything

Can I ask why the non-OSX fullscreen shortcut is ctrl-meta-space? I see F11 on gnome-terminal.

There is no 'full screen' shortcut (in my knowledge ) on Mac, so I put one I use in a terminal, I may have missed a Qt.Shortcut.Fullscreen. Fxx Keys are not dirrecty accessible on newest mac, bind to volumes control for example

I was wondering if we should change the close action on the whole window to ask about closing all tabs, and shutting them all down if affirmative. Right now if you have 10 tabs open, there's no way to close them all without answering 'yes' 10 times.

I was thinking of a 3 way option:
-cancel
-close all
-ask each time

a more complexe option might be to add a "yes for all" or "no for all" button

Definitely not 100% sure the closing logic is right, as now with possibly many tabs closing out of order, there are a large number of possible cases, and the little mess that I had made with the close dialog when there could be only one window is now pretty tangled.

I had some issues with it too :-)

BTW, i just though of a 'bug' : tab names are based on number of open tabs.
I might mess up names if you closes tab without closing kernels. We might prefer counting the number of opened kernel...

@minrk
Copy link
Member

minrk commented Oct 20, 2011

Adding as a side note, that my somewhat stale install of Qt 4.7.1 did not support svg icons (who knows why). I updated Qt to current (4.7.4) and the icon works now.

@minrk
Copy link
Member

minrk commented Oct 20, 2011

Can I ask why the non-OSX fullscreen shortcut is ctrl-meta-space? I see F11 on gnome-terminal.

There is no 'full screen' shortcut (in my knowledge ) on Mac, so I put one I use in a terminal, I may have missed a
Qt.Shortcut.Fullscreen. Fxx Keys are not directly accessible on newest mac, bind to volumes control for example

OSX may not have a perfectly uniform standard, but cmd-ctrl-F is very common (Preview, Chrome, Safari, GitX), and approximately 100% of the exceptions to this are apps that use cmd-opt-F or cmd-shift-F, so I think there's a well established reference.

BTW, i just though of a 'bug' : tab names are based on number of open tabs.
It might mess up names if you closes tab without closing kernels. We might prefer counting the number of opened kernel...

right you are - open two tabs, close the first, open a third, and you have two tabs labeled 'kernel 1'. easy fix is to just use a monotonic counter.

previous method of using tab count allowed collisions when closing and reopening tabs.
@fperez
Copy link
Member Author

fperez commented Oct 20, 2011

OK, as far as I'm concerned, this is good to go. Anyone object? Otherwise I say we merge it. There's a huge amount of work in here, and the resulting functionality is very, very nice. I don't want it to go stale after you guys having put so much high quality work in.

Thanks a lot to @Carreau, @minrk and @epatters for this awesome contribution!!

@Carreau
Copy link
Member

Carreau commented Oct 20, 2011

I'm ok for it... will work on improving again in a new branch.
Thank to all of you for continuing without me !

pressing tab inserts four spaces, so there should never be any '\t' characters
inserted by typing.

closes gh-900
closes gh-513
@minrk
Copy link
Member

minrk commented Oct 20, 2011

I've been using it myself, and it seems reliable. I would bet on there being some problems if you close the right combination of tabs in a particular order, but I keep trying a variety of combos, and it seems well behaved.

@fperez
Copy link
Member Author

fperez commented Oct 20, 2011

OK, I'm pulling it in, then... We can always keep fine-tuning afterwards, but there's no question this is high-quality work we want in. Thanks again everyone!

fperez added a commit that referenced this pull request Oct 20, 2011
Major improvements to the Qt console with menubar and tabs.

- Menu support for all actions previously available only via keybindings or right-click menus.
- Help menu giving quick access to information magics and online docs.
- Magic menu with top-level access to common magics and full access to all.
- Edit menu with common editing actions.
- View menu for font size, full-screen, etc.
- tabs: you can run multiple tabs either pointing to the same kernel or to separate kernels.
@fperez fperez merged commit 78f4eff into master Oct 20, 2011
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
Major improvements to the Qt console with menubar and tabs.

- Menu support for all actions previously available only via keybindings or right-click menus.
- Help menu giving quick access to information magics and online docs.
- Magic menu with top-level access to common magics and full access to all.
- Edit menu with common editing actions.
- View menu for font size, full-screen, etc.
- tabs: you can run multiple tabs either pointing to the same kernel or to separate kernels.
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.

None yet

3 participants