Skip to content

FIX Preserve title case when saving through GUI (issue #7824) #7865

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

Merged
merged 1 commit into from
Jan 18, 2017

Conversation

afvincent
Copy link
Contributor

A quick fix to preserve the title case when saving a figure through the GUI (before issue #7824 slips through the cracks).

Currently, it addresses the initial problem stated in #7824 but not the one raised in the discussion by @jkseppan about adding some way of dealing with forbidden characters. My viewpoint is that this could be done in another PR if we define a list of the forbidden characters in file names on the different OSes and which valid character to replace them with.

@afvincent afvincent added this to the 2.0.1 (next bug fix release) milestone Jan 18, 2017
@anntzer
Copy link
Contributor

anntzer commented Jan 18, 2017

You can reject invalid filenames by passing them to the OS.

On Linux:

Path('\0').is_file() --> ValueError

On Windows:

Path('n!ul:').is_file() --> OSError
Path('nul').is_file() --> OSError

(too lazy to remember the os.path equivalents but you get the idea).

Invalid inputs could either trigger an exception (I guess there's some value in erroring early... perhaps?) or be ignored (i.e. the default filename remains). Or we could just let the error happen later and ignore all this... which may be just fine. (I would suggest NOT trying to sanitize the filename, that's a can of worms I don't want to open... see e.g. https://bugs.python.org/issue27827)

Edit: Upon further thought, given that the only point of the default filename is to initialize a GUI dialog, we should just let the dialog do its own normalization of it, and not worry at all about invalid names.

In fact I would suggest even keeping the spaces in. It's really not that bad.

@codecov-io
Copy link

Current coverage is 62.12% (diff: 100%)

Merging #7865 into master will not change coverage

@@             master      #7865   diff @@
==========================================
  Files           174        174          
  Lines         56072      56072          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits          34833      34833          
  Misses        21239      21239          
  Partials          0          0          

Powered by Codecov. Last update 2374c9b...e942e29

@tacaswell
Copy link
Member

I think we should err on the side of letting users shoot them selves in the foot here.

This will change the default filename from 'figure_1.png' to 'Figure_1.png'. Not sure how big of a deal this is.

@anntzer
Copy link
Contributor

anntzer commented Jan 18, 2017

Or 'Figure 1.png'?

@NelleV NelleV changed the title FIX Preserve title case when saving through GUI (issue #7824) [MRG+1] FIX Preserve title case when saving through GUI (issue #7824) Jan 18, 2017
@efiring efiring changed the title [MRG+1] FIX Preserve title case when saving through GUI (issue #7824) FIX Preserve title case when saving through GUI (issue #7824) Jan 18, 2017
@efiring efiring merged commit 9e6bee6 into matplotlib:master Jan 18, 2017
@efiring
Copy link
Member

efiring commented Jan 18, 2017

@anntzer, the code is still replacing spaces with underscores, as it should. I would strongly oppose supplying a default filename with spaces in it. Regarding the switch to starting the default with upper case, I would say "no big deal", so I merged it.

@NelleV
Copy link
Member

NelleV commented Jan 18, 2017

I agree with @efiring.
I think it would be sensible to raise a warning if the filename is changed though.

@tacaswell
Copy link
Member

If we warned an changing the file name we would warn by default as the default figure name in 'Figure N', I think silently mapping ' ' -> '_' is an opinionated, but reasonable, thing to do. We do not muck with the names that come in via fig.save_fig, this is only the default that pops up in the save UI.

@QuLogic
Copy link
Member

QuLogic commented Jan 30, 2017

Backported to v2.0.x as 2262d53.

QuLogic pushed a commit that referenced this pull request Jan 30, 2017
FIX Preserve title case when saving through GUI (issue #7824)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants