Make r_minor_grid() behavior consistent with {xyz}_minor_grid

[j0hnnybash]

Without this adjustment setting r_minor_grid(true) (or r_minor_grid(false)) would non-intuitively disable the grid entirely on polar plots since the major grid would no longer be enabled by default. An alternative, potentially better solution would be to make {xyzr}_minor_grid(true) to implicitly also set {xyzr}_grid(true).

Note also that at least using gnuplot 5.4 minor grid lines will not show up on polar and non-polar plots unless 'ax->r_axis().tick_length(1.f)' (or some other non-zero tics scale value) is set (non-polar plots default to a non-zero value). This is most likely a gnuplot bug, since setting tics scale 0 is described by the "Internet(tm)" as the correct way to obtain grid lines without tics, apparently this only works for major grid lines.

[alandefreitas]

Without this adjustment setting r_minor_grid(true) (or r_minor_grid(false)) would non-intuitively disable the grid entirely on polar plots since the major grid would no longer be enabled by default

r_minor_grid(true) would disable the grid?

An alternative, potentially better solution would be to make {xyzr}_minor_grid(true) to implicitly also set {xyzr}_grid(true).

That or none of them should impact the grid. Disabling it is bad.

least using gnuplot 5.4 minor

Yes. These conflicts are awful and sometimes unfixable after a certain point. I would rewrite the whole thing with SDL if I had the time or whenever I have the time.

[j0hnnybash]

r_minor_grid(true) would disable the grid?

Exactly, because this would set r_user_grid_=true; and subsequently would prevent execution of the following block in axes_type::run_grid_command() which is responsible for default enabling the major grid for polar plots:

void axes_type::run_grid_command() {
        //...
        if (!r_user_grid_) {
            r_grid_ =
                r_axis().scale() == axis_type::axis_scale::log || is_polar();
            r_minor_grid_ = r_axis().scale() == axis_type::axis_scale::log;
        }
        //...
}

As a result r_grid_ would be false and the grid was disabled entirely.

[alandefreitas]

I noticed the PR changed to just removing

        r_user_grid_ = true;
        touch();

Not calling touch() seems problematic.

So we never set r_user_grid_ = true; anymore? Even when it's true?

What new behavior did we choose from the options above?

[j0hnnybash]

I noticed the PR changed to just removing

This PR always just consisted of this single commit, removing these two lines ;).

Not calling touch() seems problematic.

The removal of the above mentioned two lines makes axes_type::r_minor_grid(...) identical to the corresponding {xyz}_minor_grid(...) functions, which also do not call touch() nor do they set {xyz}_user_grid_ = true;, e.g.:

    void axes_type::x_minor_grid(bool x_minor_grid) {
        x_minor_grid_ = x_minor_grid;
    }

However, calling touch() should not hurt with regards to the grid being surprisingly disabled entirely, only the setting of r_user_grid_ without also setting r_grid_=true; is problematic.

So we never set r_user_grid_ = true; anymore? Even when it's true?

r_user_grid_ = true; will still be set by axes_type::r_grid(...) which enables or disables the major grid.

What new behavior did we choose from the options above?

This PR/commit changes r_minor_grid(...) s.t. it no longer sets r_user_grid_ nor call touch(), this fixes the issue of ax->r_minor_grid(true); by itself disabling the grid entirely. At the same time this makes the code for r_minor_grid(...) identical to the existing {xyz}_minor_grid(...) code. This is probably not the optimal solution, since the current implementations of {xyz}_minor_grid(...) is probably problematic as well (e.g. by not calling touch()), but it is a very small change that fixes the grid disappearing issue while making r_minor_grid(...) as broken (or non-broken) as its siblings. And as such I deemed it uncontroversial ;)

The mentioned alternative is not implemented but to outline it further: r_minor_grid(true) could be changed to imply r_grid(true) in that case it would be safe to set r_user_grid_ = true;, however r_minor_grid(false) would still be tricky. The only all-compassing solution that comes to my mind would be to have separate r_user_grid_ and r_user_minor_grid_ variables (this applies to the x,y and z axes as well).

P.S.: In hindsight this PR (and the containing commit) should have been titled: fix: don't hide the grid when setting r_minor_grid(true), with the subtitle Make r_minor_grid() _implementation_ consistent with {xyz}_minor_grid().

[alandefreitas]

Great. Thanks!

P.S.: In hindsight this PR (and the containing commit) should have been titled: fix: don't hide the grid when setting r_minor_grid(true), with the subtitle Make r_minor_grid() implementation consistent with {xyz}_minor_grid().

I was going to tell you to rename the commit but I could do that through the github interface. It only lets you do that when squashing though.