Add keymap (default: G) to toggle minor grid. #6875

anntzer · Aug 1, 2016

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.

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

anntzer · Aug 1, 2016

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 · Aug 1, 2016

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 · Aug 1, 2016

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 · Aug 1, 2016

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 · Aug 2, 2016

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.

change this to draw_idle?

done

tacaswell · Aug 2, 2016

LGTM modulo an entry in whats_new.

fariza · Aug 2, 2016

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 · Aug 2, 2016

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

anntzer · Aug 2, 2016

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

fariza · Aug 2, 2016

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

anntzer · Aug 2, 2016

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 · Aug 2, 2016

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

PRs are welcome!

fariza · Aug 8, 2016

@anntzer can you correct the PEP8 problems?

anntzer · Aug 9, 2016

done

jenshnielsen · Aug 9, 2016

Cycled to rerun with current master and fix travis pyparsing issue

fariza · Aug 9, 2016

👍 from ToolManager side @tacaswell @WeatherGod any comments?

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

I find this form of docstring formatting to be annoying. Have either both of the triple quotes on the same line, or each on their own line.

Sure, fixed.

This comment seems misplaced since no "bailing out" happens here

There is, it'll raise a ValueError that's caught outside. Added a comment to that effect.

could use a little bit more distinguishing from keymap.grid.

WeatherGod · Aug 9, 2016

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

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?

Exactly.

anntzer · Aug 25, 2016

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 · Aug 28, 2016

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 · Aug 28, 2016

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

tacaswell · Aug 28, 2016

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

mdboom added the needs_review label Aug 1, 2016

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

jenshnielsen closed this Aug 9, 2016

jenshnielsen reopened this Aug 9, 2016

jenshnielsen added needs_review and removed needs_review labels Aug 9, 2016

tacaswell removed the needs_review label Aug 21, 2016

anntzer deleted the anntzer:keymap-grid-minor branch Aug 21, 2016

This was referenced Aug 28, 2016

Merged

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

Open

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

@@ -1313,6 +1313,7 @@ def validate_animation_writer_path(p):
          '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],

+    +            # Exclude major grids not in a uniform state.
+    +            and None not in [_get_uniform_gridstate(ax.xaxis.majorTicks),
+    +                             _get_uniform_gridstate(ax.yaxis.majorTicks)]):
+    +        x_state = _get_uniform_gridstate(ax.xaxis.minorTicks)
+    +        y_state = _get_uniform_gridstate(ax.yaxis.minorTicks)
+    +        cycle = [(False, False), (True, False), (True, True), (False, True)]
+    +        try:
+    +            x_state, y_state = (
+    +                cycle[(cycle.index((x_state, y_state)) + 1) % len(cycle)])
+    +        except ValueError:
+    +            # Exclude minor grids not in a uniform state.
+    +            pass
+    +        else:
+    +            ax.grid(x_state, which="both", axis="x")
+    +            ax.grid(y_state, which="both", axis="y")
+    +            canvas.draw()

@@ -99,7 +99,8 @@ Close all plots                    **shift** + **w**
      Constrain pan/zoom to x axis       hold **x** when panning/zooming with mouse
      Constrain pan/zoom to y axis       hold **y** when panning/zooming with mouse
      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

@@ -399,24 +399,74 @@ def trigger(self, sender, event, data=None):
                      a.set_navigate(i == n)
-    -class ToolGrid(ToolToggleBase):
-    -    """Tool to toggle the grid of the figure"""
+    +class _ToolGridBase(ToolBase):
+    +    """Common functionality between ToolGrid and ToolMinorGrid.

+    +            return False
+    +        else:
+    +            return None
+    +
+    +
+    +class ToolGrid(_ToolGridBase):
+    +    """Tool to toggle the major grids of the figure"""
+    +
+    +    description = 'Toogle major grids'
+    +    default_keymap = rcParams['keymap.grid']
+    +
+    +    def _get_next_grid_states(self, ax):
+    +        x_state, y_state = map(self._get_uniform_grid_state,
+    +                               [ax.xaxis.majorTicks, ax.yaxis.majorTicks])
+    +        cycle = self._cycle
+    +        # Bail out if major grids are not in a uniform state.

matplotlib/matplotlib

Join GitHub today

Add keymap (default: G) to toggle minor grid. #6875

Conversation

anntzer commented Aug 1, 2016

mdboom added the needs_review label Aug 1, 2016

Show 2 comments Hide comments afvincent and 1 other commented on an outdated diff Aug 1, 2016

Some checks were not successful

anntzer commented Aug 1, 2016

tacaswell commented Aug 1, 2016

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

anntzer commented Aug 1, 2016

WeatherGod commented Aug 1, 2016

anntzer commented Aug 2, 2016

Show 2 comments Hide comments tacaswell and 1 other commented on an outdated diff Aug 2, 2016

tacaswell commented Aug 2, 2016

Some checks were not successful

fariza commented Aug 2, 2016

fariza commented Aug 2, 2016

anntzer commented Aug 2, 2016

fariza commented Aug 2, 2016

anntzer commented Aug 2, 2016

fariza commented Aug 2, 2016

fariza commented Aug 8, 2016

All checks have passed

anntzer commented Aug 9, 2016

jenshnielsen closed this Aug 9, 2016

jenshnielsen reopened this Aug 9, 2016

jenshnielsen added needs_review and removed needs_review labels Aug 9, 2016

jenshnielsen commented Aug 9, 2016

fariza commented Aug 9, 2016

Show 1 comment Hide comment WeatherGod commented on an outdated diff Aug 9, 2016

Show 2 comments Hide comments WeatherGod and 1 other commented on an outdated diff Aug 9, 2016

Show 2 comments Hide comments WeatherGod and 1 other commented on an outdated diff Aug 9, 2016

Show 1 comment Hide comment WeatherGod commented on an outdated diff Aug 9, 2016

WeatherGod commented Aug 9, 2016

All checks have passed

WeatherGod commented on the diff Aug 16, 2016

Hide details View details tacaswell merged commit 22aa800 into matplotlib:master Aug 21, 2016 3 checks passed

3 checks passed

tacaswell removed the needs_review label Aug 21, 2016

anntzer deleted the anntzer:keymap-grid-minor branch Aug 21, 2016

anntzer commented Aug 25, 2016

fariza commented Aug 28, 2016

anntzer commented Aug 28, 2016

tacaswell commented Aug 28, 2016

This was referenced Aug 28, 2016

afvincent and 1 other commented on an outdated diff Aug 1, 2016

tacaswell and 1 other commented on an outdated diff Aug 2, 2016

WeatherGod commented on an outdated diff Aug 9, 2016

WeatherGod and 1 other commented on an outdated diff Aug 9, 2016

WeatherGod and 1 other commented on an outdated diff Aug 9, 2016

WeatherGod commented on an outdated diff Aug 9, 2016

tacaswell merged commit `22aa800` into matplotlib:master Aug 21, 2016
3 checks passed