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

Simple GUI interface enhancements #851

Merged
merged 12 commits into from Jul 1, 2012
Merged

Conversation

pelson
Copy link
Member

@pelson pelson commented Apr 23, 2012

In this pull request, I have:

  • Added a close figure key ("q" by default)
  • Fixed full screen toggle key ("f" by default)
  • Added full screen toggle capability to Tk backend
  • Added tooltips to the Tk backend, and factored other GUI backends to use the same definition.

I am unable to test on all backends (and operating systems), so there is the possibility that I have missed something very obvious, hence I have based this against master rather than 1.1.x.

('Pan', 'Pan axes with left mouse, zoom with right', 'move.ppm', 'pan'),
('Zoom', 'Zoom to rectangle','zoom_to_rect.ppm', 'zoom'),
(None, None, None, None),
('Subplots', 'Configure subplots','subplots.png', 'configure_subplots'),
Copy link
Member Author

Choose a reason for hiding this comment

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

The inconsistency with the image file has not been carried through to this change request. Does anyone know of a good reason why it might have had a different extension?

Copy link
Member

Choose a reason for hiding this comment

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

This might have actually been a recurring issue. There might be some mailing list threads about it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Couldn't find anything. I tried searching for:

The changeset which introduced it cee76c6 seems to suggest it was just an extension rather than a bug fix. Since I was able to test this on my machine & it is consistent with the other image files, I suggest that this was worth highlighting, but not likely to cause any issues.

@pelson
Copy link
Member Author

pelson commented Apr 23, 2012

Closes #830.

@pelson
Copy link
Member Author

pelson commented Apr 23, 2012

I have now been able to test these changes with qt4agg, gtkagg and tkagg backends on a Linux (Ubuntu) install.

@efiring
Copy link
Member

efiring commented Apr 23, 2012

On Mon, Apr 23, 2012 at 5:38 PM, Phil Elson
reply@reply.github.com
wrote:

In this pull request, I have:

 * Added a close figure key ("q" by default)
 * Fixed full screen toggle key ("f" by default)
 * Added full screen toggle capability to Tk backend

I really detest having these hot keys enabled by default, and I think
that having the close key enabled by default is especially bad UI
design. It is right up there with the idiotic "pending delete" that I
think Microsoft foisted on the world, in which any highlighted text is
deleted on the next keypress.

So my preference would be to have all hot keys be opt-in, not opt out.
If I am overruled by popular opinion, then I would still insist that
the "q" key be opt-in.

 * Added tooltips to the Tk backend, and factored other GUI backends to use the same definition.

I am unable to test on all backends (and operating systems), so there is the possibility that I have missed something very obvious, hence I have based this against master rather than 1.1.x.

It would not be appropriate for 1.1.x under any circumstances; it is a
major change in behavior, not a bugfix.

Eric

You can merge this Pull Request by running:

 git pull https://github.com/pelson/matplotlib quitkey

Or you can view, comment on it, or merge it online at:

 #851

-- Commit Summary --

  • added a quit shortcut key for gui backends and fixed "toggle full screen" shortcut key which was broken.
  • Backend factorisation for tooltip sharing.
  • removed todo

-- File Changes --

M lib/matplotlib/backend_bases.py (36)
M lib/matplotlib/backends/backend_gtk.py (17)
M lib/matplotlib/backends/backend_gtk3.py (15)
M lib/matplotlib/backends/backend_qt.py (18)
M lib/matplotlib/backends/backend_qt4.py (24)
M lib/matplotlib/backends/backend_tkagg.py (124)
M lib/matplotlib/rcsetup.py (1)
M matplotlibrc.template (1)

-- Patch Links --

 https://github.com/matplotlib/matplotlib/pull/851.patch
 https://github.com/matplotlib/matplotlib/pull/851.diff


Reply to this email directly or view it on GitHub:
#851

@WeatherGod
Copy link
Member

Was 'q' already active on some backends? if not, then I think what we can do is keep this code, but set the default rcParam for quit to an empty list. This way, the mechanism is in-place rather than in some commented-out, hard-coded spots in some backends.

Otherwise, I think just about everything else makes great sense.

@pelson
Copy link
Member Author

pelson commented Apr 27, 2012

Was 'q' already active on some backends? if not, then I think what we can do is keep this code, but set the default rcParam for quit to an empty list

No its not active in any backend as far as I can see. I personally will be enabling this feature (as I suspect will the original requester @birkenfield ), so maybe its worth taking a straw poll on the mpl-users mailing-list?

It would seem strange to me that all the other keyboard shortcuts are enabled by default, so maybe we could ask:

  • Who uses the keyboard shortcuts?
  • Would you re-enable them if we disabled them all by default?
  • Which shortcuts do you actually use? (for instance, the 'f' key hasn't worked on master for 2 months since @jdh2358's 0082466 changeset and no one has reported it)
  • If it were available would you use a 'q' keyboard short-cut to close a figure?

I don't really mind either way which way it goes, but I do think its worth thinking about consistency.

@pelson
Copy link
Member Author

pelson commented May 1, 2012

Just as an update, I will be away from a computer for a couple of weeks and wont have access to contribute and/or make adjustments to the pull.

Did anyone have any feeling as to whether asking the user community is the right thing to do? If it is not then I will remove the default "q" straight away so that this can be integrated onto master.

@twmr
Copy link
Contributor

twmr commented May 4, 2012

+1 especially for the "q" short-cut. I would prefer if it is turned on by default as I'm used to gnuplots behavior and the missing short-cut bothers me most in mpl.

Actually I tried to implement this a few weeks ago but I was not able to figure out how to destroy the figure. It's great that there even exists a PR for this!

@bilderbuchi
Copy link

To contribute a (fresh) user's perspective here, I was pretty suprised/alarmed that I could not close the figure window with Ctrl-W on Ubuntu, and subsequently searched for a relevant bug report. Ctrl-W is the default shortcut for closing windows here, afaik. Ctrl-Q didn't work either. Alt-F4 worked, though, but this may be some Windows-compatibility thing recognized by Ubuntu?

I think the window should respond to platform-default shortcuts, but I don't know how complicated it would be to implement this. Failing that, just 'q' would be fine, too.

@WeatherGod
Copy link
Member

I think you touched on something important. FWIW, on my CentOS6 system with gnome 2 installed, Alt-F4 is the keyboard shortcut for closing a window (Ctrl-W and Ctrl-Q do nothing). The "close" event is largely a system-level idiom. 'q' is not a system default anywhere for closing a window. My opinion is now that 'q' should not be turned on as default. Maybe add a comment to the matplotlibrc.template that 'Alt-F4' should always work anyway?

@pelson
Copy link
Member Author

pelson commented May 21, 2012

I have made some changes which might make the behaviour more palatable. I have removed the q key and replaced it with both ctrl+w (as is standard in Ubuntu) and escape. The changes are currently not cross backend as I have only implemented the modifier keys in Tk. In summary:

  • keys pressed with a modifier will be combined in the KeyPress event. i.e. pressing shift + ctrl + q would trigger an event with a key value of of "ctrl+Q".
  • quit has bindings of ctrl+w and escape (perhaps a ctrl+q should be implemented and escape removed)
  • save has another binding (the one you would expect) of ctrl+s (the original "s" is not removed)
  • fullscreen has another binding of ctrl+f
  • I propose that all default key bindings become modifier triggered (however I have done nothing to progress this proposal as that would be a far bigger interface change)
  • modifier keys will require implementing in qt, gtk and wx

At this point I would like us to find a common ground since clearly the "quick close keyboard shortcut that isn't alt+F4" is desirable by several users (myself included), whilst simply implementing a "q" is seen as bad interface design by others (myself included). My proposed middle ground therefore is ctrl+w (and possibly escape and/or ctrl+q) for closing a figure.

@efiring & @WeatherGod does this go some way to improving the original proposal?

@efiring
Copy link
Member

efiring commented May 27, 2012

http://en.wikipedia.org/wiki/Table_of_keyboard_shortcuts
There seem to be two questions here. One is what shortcuts should be enabled by default, and the other is what key combinations should mpl be able to recognize for potential use as shortcuts.
Starting with the second question, I don't think there will be much controversy: so long as the complexity doesn't get out of hand, and it doesn't introduce bugs or other maintenance problems, enabling the recognition of key combinations in all backends is a Good Thing. It seems like the gui toolkits should make this trivial, but evidently they don't.
For the first question, moving away from the use of simple letters by default to trigger major changes is definitely good.
The first few times I ran into "f", it was pretty irritating; I was minding my own business, typing, and suddenly a plot window filled the screen. (I use focus follows mouse, so probably I had bumped the trackpad such that the cursor moved into a plot window while I was typing.) I didn't even know there were shortcuts, much less that they had been activated by default, so I did not know what had gone wrong, or how to undo it!
My preference would be to have all shortcuts be opt-in via editing of the matplotlibrc file, but I have learned to live with having some on by default, so as long as the situation is not made worse than it is at present, I may grumble occasionally but I won't yell and scream. I even occasionally use "g" to turn on a grid; a minor convenience.

@efiring
Copy link
Member

efiring commented May 27, 2012

Regarding key specifics: I strongly prefer that "escape" not be included as a default window closer.

@pelson
Copy link
Member Author

pelson commented May 28, 2012

Thanks Eric, I found your example of why single keystrokes were irritating very useful.

I have added a commit which removes the escape key shortcut, and works on improving some of the relevant documentation. Additionally, I have put in some TODOs on the backends that I am unable to run/test.

My intentions are to implement the complex keystroke changes (i.e. deprecating p in favour of ctrl+p) in another pull request once this has been put back on the master (as I think there is a lot of room for debate about sensible defaults).

I will need to get this, and another pull (#897), tested with a combination of backends and OSes and therefore will put a post on the devel mailing list asking for some help testing (or implementing the missing backend's) new features. Other than that, is the pull heading in the right direction? Are there code review type actions to be done before sending out the mpl-devel mailing list message?

@@ -2262,8 +2264,13 @@ def key_press_handler(event, canvas, toolbar=None):

# toggle fullscreen mode (default key 'f')
if event.key in fullscreen_keys:
self.full_screen_toggle()
fig_manager = Gcf.get_active()
Copy link
Member Author

Choose a reason for hiding this comment

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

Use canvas.manager as per #919.

@pelson
Copy link
Member Author

pelson commented Jun 18, 2012

Note to self: Consider addressing #215 in this PR.

@pelson
Copy link
Member Author

pelson commented Jun 20, 2012

@jkseppan or other OSX developer: Would you mind having a look at this PR and running a couple of checks?

import matplotlib
matplotlib.use('TkAgg')
matplotlib.use('Gtk3')
matplotlib.use('Qt4')

import matplotlib.pyplot as plt

plt.plot(range(10))

def onkeyboard(event):
    print event.key

cid = plt.gcf().canvas.mpl_connect('key_press_event', onkeyboard)
plt.show()

And check that when pressing ctrl+w the window closes, pressing f the window full screens, and that the toolbar buttons are displaying correctly (other backends should all work as previously, but the ctrl+w shortcut wont be working.
For the Tk backend, please also hover over a toolbar icon and see if a tooltip appears.

Thanks,


@efiring and/or @WeatherGod, are you happy with this change?

@pelson
Copy link
Member Author

pelson commented Jun 24, 2012

I have been able to test this on both Linux (Gtk, Gtk3, wx, tk, pyqt4, pyside) and OSX (tk, gtk, pyside) and on each of the backends I get at least the desired ctrl+w functionality (on OSX, ctrl really is the control button, and not the cmd button).

@pelson
Copy link
Member Author

pelson commented Jun 24, 2012

Capability wise, this PR is now up to speed. Are there any further review actions/comments?

# to make a tkagg window pop up on top on osx, osascript can be used
# this came from http://sourceforge.net/mailarchive/message.php?msg_id=23718545
cmd = ("""/usr/bin/osascript -e 'tell app "Finder" to set """ + \
"""frontmost of process "%s" to true'""") % \
Copy link
Member

Choose a reason for hiding this comment

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

Here and below, a couple matters of style:
First, please avoid backslash line continuations. Parentheses can be used instead.
Second, and less importantly, python has automatic string concatenation, so there is no need for the explicit addition when breaking a string across lines.

@jkseppan
Copy link
Member

Tested on OS X 10.6.8, Python 2.7.2, TkAgg: window closes on ctrl+w (and on cmd+w, which is the more natural Mac binding), fullscreens on f, toolbar icons look fine and have tooltips.

@jkseppan
Copy link
Member

OS X 10.6.8, Python 2.7.3 installed via MacPorts:

TkAgg: crashes (likely not related to this change - I suspect the MacPorts Tkinter installation, since it works fine with my python.org Python)
Qt4Agg: fullscreens on f, closes on ctrl+w (not on cmd+w), icons look fine, no tooltips
Gtk3Agg: can't get pygobject installed properly, with some trying

OS X package management being what it is, I can't be sure that something I happen to have installed isn't interfering with some of the backends (though I'm using virtualenv to install packages under the MacPorts python). For proper testing, we should probably have a virtual machine image with all the correct dependencies installed, and a script that installs a git version of matplotlib from scratch.

@pelson
Copy link
Member Author

pelson commented Jul 1, 2012

@efiring: I've removed some the two blocks that you highlighted as they don't belong in this PR. I will re-consider them for another pull request (once I have figured out how to implement it properly).

I think this PR is in a state to merge now. Obviously, there is no way that we can test every combination of OS, Python version, backend etc., but any further issues that crop up we can deal with in future issues/PRs. Anyone disagree?

efiring added a commit that referenced this pull request Jul 1, 2012
Simple GUI interface enhancements
@efiring efiring merged commit 5b90a27 into matplotlib:master Jul 1, 2012
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

6 participants