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

Implementation of Issue #3418 - Auto-wrapping text #4342

Merged
merged 6 commits into from May 22, 2015

Conversation

sptm
Copy link
Contributor

@sptm sptm commented Apr 15, 2015

Implementation of Issue #3418 - Auto-wrapping text.
This is done by adding a new parameter 'wrap' to Text.

When enabled, it incrementally builds a temporary line and re-estimates the dimensions of the line for each word added, inserting new lines when needed.

Currently works for any rotated text, different modes of alignment, and for text that are either labels or titles.

@tacaswell tacaswell added this to the next point release milestone Apr 16, 2015
@tacaswell
Copy link
Member

Nice. I don't really understand what is causing the failures though. I assume it is some issue with freetype versions...

@OceanWolf
Copy link
Contributor

@tacaswell Anyway to pull the image out of Travis and compare?

@sptm Do you have any rcParams set different from defaults? For example savefig.bbox

@andyniemi
Copy link

@tacaswell Freetype2 is the version I've been using to generate the test images on several computers. Is there a version to conform to?

@OceanWolf In development, rcParams remained the same. It's been recently updated to the MPL source code though when preparing for PR. Would that make a difference?

@sptm Remove the font test case and check?

@tacaswell
Copy link
Member

exactly which version of freetype2 matters. All of the current test images are generated with freetype 2.5.3-21 on Fedora 21 (per #4031 (comment)).

The failed images on non-PR builds are uploaded to AWS but there is no way to get the images from failed PR build.s

@tacaswell
Copy link
Member

Also see #4345

@OceanWolf
Copy link
Contributor

Oooh, brainwave, as part of the testing infrastructure we could catch the test failure email the graphic to the owner of the PR (or something similar) and then re-raise the error. See http://stackoverflow.com/questions/3362600/how-to-send-email-attachments-with-python for emailing attachments!

@mdboom
Copy link
Member

mdboom commented Apr 17, 2015

@OceanWolf: That's a good idea, and one I've certainly thought about. The problem is that to e-mail you need e-mail credentials to some mail transport server. And to use e-mail credentials in a pull request, you are basically exposing those credentials to anyone who wants to submit a pull request (i.e. anyone with a github account). It won't be long until someone uses those credentials to just send spam.

What we do do is upload the images to an Amazon S3 account, and those images are then publically downloadable. But we can only do it on the main fork (because the only people who can put code in master are "trusted" people), not in any PR. As a workaround, of course, an author of a PR can ask someone with developer priviledges to push the source branch on the PR to a temporary branch on the main fork. That causes Travis-CI to test the branch and upload the failed images to Amazon S3.

@OceanWolf
Copy link
Contributor

Hmm, re the security issues of email, I can think of two methods:

  1. Have github do the email (I don't know if any sort of api exists between travis and github to do that).
  2. A tweak to what happens at the moment, we could have travis send the failed images to a/the server we have access to (e.g. the s3 server) which then sends the image as an attachment to the PR submitter or something. As the server will process the incoming response from Travis you keep the credentials safely locked away on the server.

@mdboom
Copy link
Member

mdboom commented Apr 17, 2015

For (1) -- you'd still need Github credentials for that, even if that were possible.

For (2) -- where do you store the credentials to the S3 server? That server would cost real money if someone took it over to share files on or something.

See the problem? Any build of a PR by necessity has exposes the keys to the castle to anyone who can make a PR. Doing any sort of network operation (other than read-only downloading) requires those keys. The workaround involves a trusted person (i.e. a matplotlib developer) to take an affirmative action after reading the PR for anything nefarious. As long as PRs are open to anyone on the internet, I don't see a way around that...

@OceanWolf
Copy link
Contributor

@mdboom Actually no I don't see the problem.

Method 1

While probably not possible at the moment, I wondered how Travis communicates back to github to tell it that the build has succeeded/failed. For instance how it seems to work at the moment.

  1. When you push to github, github creates a webhook to Travis telling it about the new files.
  2. Travis then somehow replies to github telling it that it has started and providing a link details to the current build.
  3. When Travis has finished the build, it then sends another message back to github about whether the build succeeded or failed and github updates it's merge status appropriately.

I suggest here that we tap into the messaging stream which occurs with step 2. and/or 3. operates to have Travis on failure ask github to message the owner of the PR in the same way that github messages the owner of the PR whenever someone posts a message on the PR. The secure communication aspect for this seems to exist, I just don't know about the API. If not, then it seems logical that it should and thus could get implemented. Of course I may completely misunderstand how the communication occurs between Travis and Github, apologies if I have.

Method 2

You don't need to store the credentials for the S3 server, you send a normal request, http
or something, I think debian does it the same way with reportbug. the server just behaves as a relay.

  1. The S3 server receives for example an http POST request with the image and details of the PR.
  2. The S3 server validates that Travis sent the request, either from simple ip address checking, (as a more complicated addition it could talk to Travis using the Travis-API to confirm the request came from Travis).
  3. The S3 server sends the email with the image as an attachment (perhaps with a check to github to get the email address).
  4. Profit.

Because we keep all the credentials and perform all of the validation on the S3 server, we never expose any secure information, we only show publicly the information in the HTTP POST which contains only data that exists in the public domain anyway (and we can even encrypt using HTTPS, but I don't think we need to).

@sptm
Copy link
Contributor Author

sptm commented Apr 19, 2015

Sorry for the late reply, the freetype version of the most recent one was 2.4.0.
The next one is generated from 2.4.8, which should work in Travis.

@tacaswell
Copy link
Member

The problem (I think) is that you are using fonts that are not on travis. Running this on my system (which has other issues with text-related tests (10 of them fail 😞 ) ) I get the following images

fonttext_wrap
fonttext_wrap-expected
fonttext_wrap-failed-diff

@tacaswell
Copy link
Member

Can you also re-base so that the commits with the bad images are not in the history? We are trying to keep the size of the repo down as much as possible.

@sptm
Copy link
Contributor Author

sptm commented Apr 19, 2015

So it looks like they don't have Times New Roman... thanks for the info.
I'm going to re-work this test and will do a re-base.

@tacaswell
Copy link
Member

Iirc, there are licensing issues with it. We could probably install it, but
relying only on the fonts we ship with mpl is a better option.

On Sun, Apr 19, 2015, 00:12 sptm notifications@github.com wrote:

So it looks like they don't have Times New Roman... thanks for the info.
I'm going to re-work this test and will do a re-base.


Reply to this email directly or view it on GitHub
#4342 (comment)
.

@jenshnielsen
Copy link
Member

Looking into the Travis logs there are currently there are 3 missing fonts

/home/travis/virtualenv/python2.6.9/lib/python2.6/site-packages/matplotlib-1.5.dev1-py2.6-linux-x86_64.egg/matplotlib/font_manager.py:1285: UserWarning: findfont: Font family [u'Umpush'] not found. Falling back to Bitstream Vera Sans
  (prop.get_family(), self.defaultFamily[fontext]))
/home/travis/virtualenv/python2.6.9/lib/python2.6/site-packages/matplotlib-1.5.dev1-py2.6-linux-x86_64.egg/matplotlib/font_manager.py:1285: UserWarning: findfont: Font family [u'Times New Roman'] not found. Falling back to Bitstream Vera Sans
  (prop.get_family(), self.defaultFamily[fontext]))
/home/travis/virtualenv/python2.6.9/lib/python2.6/site-packages/matplotlib-1.5.dev1-py2.6-linux-x86_64.egg/matplotlib/font_manager.py:1285: UserWarning: findfont: Font family [u'Waree'] not found. Falling back to Bitstream Vera Sans
  (prop.get_family(), self.defaultFamily[fontext]))

- Auto-wrapping text enabled/disabled through new argument 'wrap'
- PEP8 changes
- Added test cases
@andyniemi
Copy link

It looks like it builds now. Any updates or suggestions regarding the PR?

@tacaswell
Copy link
Member

@ joferkington Any comments as this is based on your SO answer?

tmp, lp_h, lp_bl = renderer.get_text_width_height_descent('lp',
self._fontproperties,
ismath=False)
tmp, lp_h, lp_bl = renderer.get_text_width_height_descent(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you discard this change?

@sptm
Copy link
Contributor Author

sptm commented Apr 27, 2015

Newest commits uses a context manager to save the old text. Not sure if this is the correct way, as it's my first time using it.

gc.set_alpha(self.get_alpha())
gc.set_url(self._url)
self._set_gc_clip(gc)
with _wrap_text(self) as self:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not yield this as self, that is a bit terrifying.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can rename this to text? textobj?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

@tacaswell
Copy link
Member

@sptm This definitely needs an entry in https://github.com/matplotlib/matplotlib/tree/master/doc/users/whats_new This is a feature people have been wanting for a long time and it needs to be advertised!

Putting an example in the gallery would also be a good idea.

@sptm
Copy link
Contributor Author

sptm commented May 22, 2015

Documentation added. The example is the same one from the test case.

@tacaswell
Copy link
Member

Awesome! Thanks for your work.

tacaswell added a commit that referenced this pull request May 22, 2015
ENH : Implement auto-wrapping text

closes #3418
@tacaswell tacaswell merged commit b8ea343 into matplotlib:master May 22, 2015
@sptm
Copy link
Contributor Author

sptm commented May 22, 2015

No problem! Thanks for all the help.

@joferkington
Copy link
Contributor

@sptm and @tcaswell - I just wanted to say thanks for the effort in getting this in! (And with a much nicer implementation than my initial hack, as well.)

@mghods
Copy link

mghods commented Aug 19, 2015

Thank you for adding this.

@OceanWolf
Copy link
Contributor

Shouldn't the needs_review label have been removed from this when it got merged?

@jenshnielsen
Copy link
Member

Well there are 218 closed PRs with needs review so whats special about this one. I don't really think thats important since github always does is:open label:needs_review

@OceanWolf
Copy link
Contributor

Ahh, didn't realise, nothing special, just the only one I have seen, I feel so used to seeing the needs_review label disappearing once an issue was closed, so much so, I thought it an automatic standard practice thing...

@jenshnielsen
Copy link
Member

I think the automatic removal is a relatively new thing so it will be missing from older merges.

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

9 participants