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

Fixed interactive save to use rcParams for facecolor and edgecolor. #4467

Closed
wants to merge 1 commit into from

Conversation

u55
Copy link
Contributor

@u55 u55 commented May 25, 2015

Fix for issue #3437. I implemented the rcParams for 'savefig.facecolor' and 'savefig.edgecolor', but I did not try to implement the 'savefig.transparent' option. This is only a temporary fix, as the savefig logic is currently duplicated in multiple places and should be refactored.

@tacaswell
Copy link
Member

Can you also change around L1504 to default to passing None in to print_figure so we only have the rcparam look-up logic once?

@tacaswell tacaswell modified the milestones: next major release, next point release May 25, 2015
@u55
Copy link
Contributor Author

u55 commented May 26, 2015

@tacaswell, you must mean approximately line 1504 of figure.py rather than backend_bases.py. I wonder if everything from figure.py/Figure.savefig should be moved to backend_bases.py/FigureCanvasBase.print_figure, with the exception of the call to self.canvas.print_figure(*args, **kwargs) of course. What do you think?

Also, according to the CHANGELOG from 2007-09-06, backend canvases are not supposed to override the print_figure method, so it does seem safe to move everything to this method.

Alternatively, it seems that everything could be moved from backend_bases.py/FigureCanvasBase.print_figure to figure.py/Figure.savefig if all of the interactive backends were to change their NagivationToolbar2 classes to use something like: self.canvas.figure.savefig(six.text_type(fname)) instead of self.canvas.print_figure(six.text_type(fname)), in their overridden save_figure method. However, there are a few documented examples that use the canvas.print_figure method explicitly, such as examples/pylab_examples/hyperlinks.py, and I wonder if these are just really old examples or if there is still a use case for fig.canvas.print_figure instead of fig.savefig?

@tacaswell
Copy link
Member

Yes, I did mean in figure.py, sorry for being unclear.

This does bring up a discussion of where the rcparam logic should be looked up and I do not have a clear answer for that off the top of my head.

The toolbar just got a major overhaul. attn @OceanWolf

I think I am on board with changing all of those calls to 'print_figure->self.canvas.fig.save_fig()`, but that is some what orthogonal to where the default logic goes.

@OceanWolf
Copy link
Contributor

@tacaswell do you mean MEP22? sounds like a good idea to me. I do have plans to do more savefig overhaul as part of MEP22 as and when I get around to it (thesis, job interviews, etcetera, hopefully by this weekend)

@u55
Copy link
Contributor Author

u55 commented May 27, 2015

Yes, I guess the question is: As a general rule, should rcParams be handled as early as possible or as late as possible? I would expect that earlier is better.

I could remove the kwarg-passing lines near L1504 of figure.py, for this pull request, as you suggested @tacaswell. But I currently do not have time right now to any significant code refactoring and testing. If someone else, such as @OceanWolf, wants to do a more extensive fix, then I am happy to abandon this PR.

@OceanWolf
Copy link
Contributor

To clarify, by overhaul, I just mean moving out the backend specific savefig to backend.tools, we only need a mechanism to get a filename from the backend which a generic savefig method would call (GTK3 already has this logic split, it just needs a copy-paste to backend-tools and then get expanded on with the other backends, probably with a tidy-up at the same time as it looks a bit messy 😉).

So yes, @u55 my concern only lies with a first pass to clear the specific backends of generic functionality, moving to backend_tools.

@tacaswell
Copy link
Member

@u55 Any progress on this?

I am re-milestoning this to 'proposed next point release'. If it is ready it can go in, but it will not be a blocker for the next point release.

@tacaswell tacaswell modified the milestones: proposed next point release, next point release Jun 16, 2015
@tacaswell tacaswell modified the milestones: 2.0 (style change major release), 2.1 (next point release) Mar 21, 2016
@tacaswell tacaswell self-assigned this Mar 21, 2016
@QuLogic
Copy link
Member

QuLogic commented Mar 26, 2016

Replaced by #6197.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants