Add keymap (default: G) to toggle minor grid. by anntzer · Pull Request #6875 · matplotlib/matplotlib

anntzer · 2016-08-01T03:55:48Z

If either minor grid is on, turn both of them off. Otherwise, turn all
major and minor grids on (it usually doesn't make sense to have the
minor grids only).

Also change the behavior of keymap.grid ("g") to synchronize the grid
state of both axes. Otherwise, if starting from only grids in one
direction (plt.plot(); plt.grid(axis="x")), "g" switches between only
x grid and only y grid, which is unlikely to be the expected
behavior.

See #6616.

afvincent · 2016-08-01T07:57:48Z

lib/matplotlib/rcsetup.py

    'keymap.quit':         [['ctrl+w', 'cmd+w', 'q'], validate_stringlist],
    'keymap.quit_all':     [['W', 'cmd+W', 'Q'], validate_stringlist],
    'keymap.grid':         [['g'], validate_stringlist],
+    'keymap.grid_both':    [['G'], validate_stringlist],


Is it normal that it is 'keymap.grid_both' here while it is 'keymap.grid_minor' anywhere else in the commit?

no, that's a typo...

If either minor grid is on, turn both of them off. Otherwise, turn all major and minor grids on (it usually doesn't make sense to have the minor grids only). Also change the behavior of `keymap.grid` ("g") to synchronize the grid state of both axes. Otherwise, if starting from only grids in one direction (`plt.plot(); plt.grid(axis="x")`), "g" switches between only `x` grid and only `y` grid, which is unlikely to be the expected behavior.

anntzer · 2016-08-01T08:18:41Z

Actually, after some more thought, I think I'll prefer another behavior for g (resp. G): if the major (resp. major and minor) grids are not set for all ticks on both axes, just do nothing. Basically, when this is the case, it means that the user did some specific customization of the grid that is not going to be undoable via g/G if we try to touch it -- and I believe interactive navigation should avoid as much as possible irreversible changes to the figure.
Thoughts?

tacaswell · 2016-08-01T15:46:35Z

How the grids cycle through (x,y): (off, off), (on, on), (on, off), (off, on) ? This could probably be generalized to 3d in a strait forward way.

anntzer · 2016-08-01T16:29:11Z

They switch both (which comes from the behavior of grid): you're in one of the cycles, (on,on)<->(off,off) or (on,off)<->(off,on). Actually I like your suggestion, but I think we should give it a try first... (UX testing done by the devs...).
(Generalization to 3D is a bit awkward because any cycle would become pretty long and painful to use, IMO.)

WeatherGod · 2016-08-01T16:31:09Z

Also... there are no minor grids/ticks in mplot3d (at this point, that is).

On Mon, Aug 1, 2016 at 12:29 PM, Antony Lee notifications@github.com
wrote:

They switch both (which comes from the behavior of grid): you're in one
of the cycles, (on,on)<->(off,off) or (on,off)<->(off,on). Actually I like
your suggestion, but I think we should give it a try first... (UX testing
done by the devs...).
(Generalization to 3D is a bit awkward because any cycle would become
pretty long and painful to use, IMO.)

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#6875 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AARy-HOE_WD22ZHinVUomfJt77ILEqIJks5qbh7YgaJpZM4JZOSZ
.

anntzer · 2016-08-02T03:16:07Z

Implemented @tacaswell's suggestion. I chose the order (none)->(x)->(xy)->(y), so that one can easily switch from both-on to both-off and vice versa by typing "g/G" twice, which I think may be more user friendly.

tacaswell · 2016-08-02T12:37:34Z

lib/matplotlib/backend_bases.py

+        else:
+            ax.grid(x_state, which="both", axis="x")
+            ax.grid(y_state, which="both", axis="y")
+            canvas.draw()


change this to draw_idle?

tacaswell · 2016-08-02T12:41:30Z

LGTM modulo an entry in whats_new.

fariza · 2016-08-02T17:54:21Z

Sorry I'm late to the party.
We are moving towards toolmanager and tools to replace (between other things) the backend_bases.key_press_handler.

I know it is a lot of work, but can you add a tool or modify backend_tools.ToolGrid to add this functionality there?
If this new functionality is not added there, it will be lost when deprecating old stuff.

fariza · 2016-08-02T17:55:18Z

@anntzer if you have any questions regarding how to do it i'm happy to help

anntzer · 2016-08-02T17:59:41Z

How can I test my (TBD) implementation in the new API?

fariza · 2016-08-02T18:07:40Z

take a look at examples/user_interfaces/toolmanager.py you can create an independent tool to test your functionality.

anntzer · 2016-08-02T18:35:35Z

Done. What's the status of MEP22? It's certainly a nicer API than the huge switch-based dispatch in key_press_handler...
BTW, is there a tracking issue for this MEP? I'd like to suggest that the data kwarg to trigger_tool seems redundant (only used to pass some info to the Rubberband tool, and that info could well (should?) probably be calculated by the Rubberband itself on the basis of the event it already gets).

fariza · 2016-08-02T18:47:16Z

MEP22 is already merged but not completed, we are waiting for MEP27 to land, to modify all the backends.

PRs are welcome!

fariza · 2016-08-08T21:47:06Z

@anntzer can you correct the PEP8 problems?

anntzer · 2016-08-09T02:33:06Z

done

jenshnielsen · 2016-08-09T11:17:12Z

Cycled to rerun with current master and fix travis pyparsing issue

fariza · 2016-08-09T13:43:17Z

👍 from ToolManager side @tacaswell @WeatherGod any comments?

WeatherGod · 2016-08-09T13:49:30Z

doc/users/navigation_toolbar.rst

 Preserve aspect ratio              hold **CONTROL** when panning/zooming with mouse
-Toggle grid                        **g** when mouse is over an axes
+Toggle major grid                  **g** when mouse is over an axes
+Toggle major and minor grid        **G** when mouse is over an axes


Here you say "major and minor grids", but in the api_changes, you say that "G" will toggle minor grids.

WeatherGod · 2016-08-09T14:00:53Z

Not totally sold on the naming because it is called "minor", but it impacts both minor and major.

WeatherGod · 2016-08-16T19:56:33Z

lib/matplotlib/backend_tools.py

+        # Bail out (via ValueError) if minor grids are not in a uniform state.
+        x_state, y_state = (
+            cycle[(cycle.index((x_state, y_state)) + 1) % len(cycle)])
+        return x_state, y_state, "both"


This is the only thing that is still confusing me. The documentation and descriptions all say that it would be minor grids, but it really is both minor and major, right? Not a reason to hold things up, really -- just want to make sure I understand and see if tweaks to the documentation is needed.

Basically it toggles the minor grids, but if it would turn on a minor grid while the corresponding major grid is off, then it also turns on the major grid, because it typically doesn't make much sense to have just the minor grid.
If you have a better suggestion for the docs I'm happy to take it...

Ok, I think I understand now. So it doesn't toggle both major and minor in lockstep. If the major grid is already on, then pressing 'G' won't do anything to the major grid, right? But if the major grid is off, then both of them are turned on/off?

anntzer · 2016-08-25T23:18:45Z

Note to self (or whoever wants to take over, should be easy enough): this implementation does not switch off the minor grids when the major grids are turned off, when it should probably do so.
Basically, the which argument to ax.xaxis.grid should be "both" if not state else "major" in that case ("if turning off the major grids, also turn off the minor grids at the same time.").

fariza · 2016-08-28T22:20:50Z

Small PR?

On Aug 25, 2016 7:18 PM, "Antony Lee" notifications@github.com wrote:

Note to self (or whoever wants to take over, should be easy enough): this
implementation does not switch off the minor grids when the major
grids are turned off, when it should probably do so.
Basically, the which argument to ax.xaxis.grid should be "both" if not
state else "major" in that case ("if turning off the major grids, also
turn off the minor grids at the same time.").

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#6875 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABa86W5WrDhwcnUaYynNAPykD7fDNNEqks5qjiLggaJpZM4JZOSZ
.

anntzer · 2016-08-28T22:27:48Z

Let's see whether playing the waiting game will get someone else to volunteer :)

tacaswell · 2016-08-28T22:38:05Z

@anntzer that is not a game I would bet on winning 😜

mdboom added the status: needs review label Aug 1, 2016

afvincent reviewed Aug 1, 2016
View reviewed changes

anntzer force-pushed the keymap-grid-minor branch from 6fa0fd1 to e8de04c Compare August 1, 2016 08:18

tacaswell added this to the 2.1 (next point release) milestone Aug 1, 2016

tacaswell reviewed Aug 2, 2016
View reviewed changes

Cycle the grid state (none->x->xy->y) for 'g'/'G'.

0c3937c

anntzer force-pushed the keymap-grid-minor branch from d7d940f to 0c3937c Compare August 2, 2016 17:18

anntzer force-pushed the keymap-grid-minor branch from 1fd07ae to a02b264 Compare August 2, 2016 18:32

Toolmanager implementation of minor grid toggler.

32187df

anntzer force-pushed the keymap-grid-minor branch from a02b264 to 32187df Compare August 9, 2016 02:33

jenshnielsen closed this Aug 9, 2016

jenshnielsen reopened this Aug 9, 2016

jenshnielsen added status: needs review and removed status: needs review labels Aug 9, 2016

WeatherGod reviewed Aug 9, 2016
View reviewed changes

Minor fixes re: grid_minor keybinding.

0658c2b

WeatherGod reviewed Aug 16, 2016
View reviewed changes

tacaswell merged commit 22aa800 into matplotlib:master Aug 21, 2016

tacaswell removed the status: needs review label Aug 21, 2016

anntzer deleted the keymap-grid-minor branch August 21, 2016 01:30

anntzer mentioned this pull request Aug 28, 2016

Turn off minor grids when interactively turning off major grids. #6995

Merged

anntzer mentioned this pull request Sep 29, 2016

Default locator for log-scale messes up minor ticks sometimes #7200

Closed

Uh oh!

Conversation

anntzer commented Aug 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anntzer commented Aug 1, 2016

Uh oh!

tacaswell commented Aug 1, 2016

Uh oh!

anntzer commented Aug 1, 2016

Uh oh!

WeatherGod commented Aug 1, 2016

Uh oh!

anntzer commented Aug 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tacaswell commented Aug 2, 2016

Uh oh!

fariza commented Aug 2, 2016

Uh oh!

fariza commented Aug 2, 2016

Uh oh!

anntzer commented Aug 2, 2016

Uh oh!

fariza commented Aug 2, 2016

Uh oh!

anntzer commented Aug 2, 2016

Uh oh!

fariza commented Aug 2, 2016

Uh oh!

fariza commented Aug 8, 2016

Uh oh!

anntzer commented Aug 9, 2016

Uh oh!

jenshnielsen commented Aug 9, 2016

Uh oh!

fariza commented Aug 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WeatherGod commented Aug 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anntzer commented Aug 25, 2016

Uh oh!

fariza commented Aug 28, 2016

Uh oh!

anntzer commented Aug 28, 2016

Uh oh!

tacaswell commented Aug 28, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants