Does this solve#10939?

No, for that you want to look at#11692.

Are the changes todoc/users/whats_new.rst something that belongs into this PR?

Technically this simplifies things.
One drawback of this is that it makes the option to turn the axes background off hard to find for a user. Any idea how to still make sure people would find that option? I think somehow it needs to become clear that the rcParamrcParam["figure.frameon"]=False is the same as settingfig.patch.set_visible(False) and the same asplt.figure(frameon=False).

Got rid of the (bookkeeping) change to whatsnew.
I don't think figure.frameon was particularly well documented either unless you were a fan of going through the shipped example matplotlibrc... in any case feel free to push doc changes.

Right now we have, for instance get_facecolor/set_facecolor (and edgecolor, but not linewidth :eyeroll:)

That's essentially the problem: we have a top layer (Figure) partially duplicating a bottom layer (Figure.patch), but not completely and some things under different names (set_visible -> set_frameon, although naming it set_visible wouldn't work either as that'd imply not drawing the child artists either). I actually think pointing the user to do everything on the .patch would make the API simpler for the end user, not less.

We should decide if that is the way to go, and do it all then and thoroughly update the docs. I guess Personally I'm 50/50 on going this way, versus just addingfigure.set_linewidth andfigure.set_frameon.

I'm -0.5 on adding additional figure methods to control the patch. It wouldn't end withset_linewidth (which by the way is not a good name. What is the figure line anyway? It may easily be confused with other lines, such as the axes.). You would needFigure.set_edgecolor and others. It's reasonable to control the background via a dedicated object. We should make this more clear in the docs.

Once this is done, there is no need forFigure.frameon anymore. We don't need two ways here.

I'm not sure about the namepatch. It's a bit unspecific.frame orbackground might be better. Though I'm not 100% sure which one I like. Essentially, we need to check which code looks most reasonable:

figure.patch.set_linewidth()figure.patch.set_facecolor()figure.patch.set_edgecolor()figure.patch.set_visible()

or

figure.frame.set_linewidth()figure.frame.set_facecolor()figure.frame.set_edgecolor()figure.frame.set_visible()

or

figure.background.set_linewidth()figure.background.set_facecolor()figure.background.set_edgecolor()figure.background.set_visible()

Another aspect is theframeon kwarg and thefigure.frameon rcparam. They get more obscure if there is no corresponding property. Might still be ok, if we would renamepatch toframe. Otherwise, we should do something about them as well.

Wealready haveFigure.set_facecolor andFigure.set_edgecolor, so addingFigure.set_linewidth isn't such a stretch. If we are keeping those, I don't see why we are making the user drill down to thepatch to manipulate visibility. So either

1. deprecate facecolor, egdecolor, frameon in favour of manipulating the patch (I'm not so worried about the name), or:
2. addset_linewidth and keepframeon.

I don't think it makes sense to deprecate existing/widespread API (frameon) unless there is a true problem which needs to be solved.

plt.rcParams["figure.frameon"] = Falsefig = plt.figure(frameon=True)fig.frameon = False

is a perfectly valid API. Anything that requires the user to think more about this, cannot be seen as improvement.

Having two or more layers in the code can be problematic, but it isn't in this case, whenframeon is a transparent wrapper for thevisibility of thepatch. So the improvement is to makeframeon a realproperty here.

Hence:
👍 on makingframeon a property withget_/set_frameon available.
👎 on changing any API and deprecating or removing existent functionality.

Would you consider that by the same argument#10088 was going in the wrong direction?

In my opinion, the problem of such additional layers is that they both make it harder to write correct code (the actual visibility state isfigure.frameon and figure.patch.get_visible(), not just either of them), and users tend to write ad-hoc code against them instead of using more general, widely usable APIs (e.g. consider the SkewT implementation change in#10088: yes, the "new" implementation was also possible previously, but whoever implemented it didn't think about it because label1On, etc. were "more apparent"; even without using ExitStack I think it's clear that manipulating the visibility state in draw() is simpler than overriding a bunch of properties).

I'm totally in favour of makingframeon a completely transparent wrapper topatch.visible. I do see the problem of the different layers, but if introducingframeon as property as you do here gets rid of that problematic completely, hence the state offigure.frameon andfigure.patch.get_visible() cannot disparate.

The same could have been done in#10088. There is no benefit of not having this kind of thin-wrapping properties, other than less code lines. But removing them might break people's code. Because they were not implemented as properties, code breaks in an intransparent way now. There is no indication thattick.label2On = False should suddenly not turn the label off any more. This is pretty mean for an inexperienced user.

There is no benefit of not having this kind of thin-wrapping properties, other than less code lines.

Again, I disagree, although even more strongly for#10088, where the names (label1On, etc.) clearly point to the corresponding attributes (label1, etc.): I want to encourage users to use generic utilities (set_visible()) rather than specific ones (label1On), as that knowledge is more widely applicable. The new implementation of SkewT, for example, demonstrates a general, reusable method of manipulating sub-artist visibility in draw(), rather than being completely specific to SkewT.

Because they were not implemented as properties, code breaks in an intransparent way now. There is no indication that tick.label2On = False should suddenly not turn the label off any more. This is pretty mean for an inexperienced user.

Actually I implemented them as properties for the deprecation period, so that a deprecation warning does get emitted...

See also discussion surroundinghttps://github.com/matplotlib/matplotlib/pull/12241/files#r223905198 for code that would become simpler if visibility just meant, well, {get,set}_visible(), instead of having to check on other attributes.

You'd be totally free to use.frameon or.patch.get_visible(), whichever you like more, in a case where you need to check visibility of the patch. If there are then people using constructs likeif fig.frameon and fig.pach.get_visible(): that would mostly be because of unclear docstrings.

I undeprecated frameon to move this PR forward (it is now just an alias for the visibility state of the underlying patch).

👍 Do you want to update any docstrings? Something like "This is a convenience wrapper forFigure.patch.set_visible kept for compatibility and convenience." ?

Please push the changes as desired.

@ImportanceOfBeingErnest Note, by the way, that having#10088 would have avoided the issue around#10981, namely that we were using one way, internally, to control visibility, whereas the user may be using another knob (although I guess this doesn't argue against having two synonyms for the same setting, but heh).

I consider this a major difference. Just leveling up some properties is totally different from having two (or more) separate code paths.
I do share the concerns about the latter, as seen from the discussion you link to. There is also#10882 for which there isn't even a solution as of now.
The main problem is mostly that there seem to be different concepts around and no general strategy being agreed upon, such that everyone just pushes a bit to either side. At the end, the user is the one who's conned.

I will look at the documentation for this latest on Saturday. If someone wants to merge in between, I can also open a new PR.

Ok, so how do I push to this PR? Isthis the right strategy? It seems that I would need to clone your complete fork to a local folder, which seems a bit strange/overkill.

You don't need a new clone; just some remotes pointing to the fork, or use the fork's URL directly.

To pull a PR's contents (ifupstream is your remote name for matplotlib):

git fetch upstream pull/11691/head:local-branch-name

You could also pull directly from someone's fork:

git fetch https://github.com/anntzer/matplotlib figureframeon:local-branch-name

(or use thegit@github.com SSH address instead of HTTPS.)

To push to@anntzer's fork:

git push https://github.com/anntzer/matplotlib local-branch-name:figureframeon

(or use thegit@github.com SSH address instead of HTTPS.)

Thanks@QuLogic, I had the feeling that it must be easier.

@anntzer are you ok with those docstrings? Do I now need to remove my approval due to me commiting something here?

Do I now need to remove my approval due to me commiting something here?

Technically I guess so, although I'd like to suggest a change of policy wrt these "collaborative" PRs (namely that we can just have separate approval by enough devs for each part of the PR).

Not sure if this has enough reviewers overall. If@anntzer and@ImportanceOfBeingErnest each approve their collaborative code, that would be 1 plus 1 by me.

IMO,@anntzer if you approve the changes of@ImportanceOfBeingErnest you can self-merge.

I approve@ImportanceOfBeingErnest's patch.