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

Fix backend_svg.RendererSVG.draw_text to render urls #2521

Merged
merged 1 commit into from Jan 5, 2014

Conversation

mdengler
Copy link
Contributor

The SVG backend does nothing with Text object's URLs. In particular, the rendering path below
backend_svg.RendererSVG.draw_text does nothing with the provided GraphicsContext's _url field.

This commit fixes that issue, causing Text objects' URLs to become <a> tags around the rendered text in the rendered SVG.

@mdboom
Copy link
Member

mdboom commented Oct 15, 2013

Looks good. Can you add a test and a what's new entry?

@mdengler
Copy link
Contributor Author

Thanks, and sure -- added a test and what's new entry. I'm not familiar with the what's new entries' tone and levels, so I can re-do that in a different format / tone if you let me know an example to follow or any other specifics you'd like.

@@ -1128,11 +1128,17 @@ def draw_text(self, gc, x, y, s, prop, angle, ismath=False, mtext=None):
self.writer.start(
'g', attrib={'clip-path': 'url(#%s)' % clipid})

if gc.get_url() is not None:
self.writer.start('a', {'xlink:href': gc.get_url()})
Copy link
Member

Choose a reason for hiding this comment

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

This is crying out for a context manager. Not in scope for this PR though.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah -- the original commit that introduced the XML writer used a context manager, until I remembered we were still supporting Python 2.5 at the time. Now that we're not, I agree all of backend_svg should be refactored in that style, but not for this PR.

@pelson
Copy link
Member

pelson commented Oct 18, 2013

👍 - LGTM

The `svg` backend will now render :class:`~matplotlib.text.Text` objects'
url as a link in output SVGs. This allows one to make clickable text in
saved figures using the url kwarg of the :class:`~matplotlib.text.Text`
class.
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 fix the pep8 violation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I don't immediately see the PEP 8 violation in the .rst file; did you mean the two newlines after the block of text (lines 62-65)? Happy to format the whats_new text in whatever is the preferred way. As this is my first time and there appear to be different styles for items of differing importance, I had to wing it a bit.

Copy link
Member

Choose a reason for hiding this comment

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

blah, sorry right line wrong file.

@mdboom
Copy link
Member

mdboom commented Oct 30, 2013

👍 from me, once the other concerns are addressed.

@cleanup
def test_text_urls():
fig = plt.figure()
ax = fig.suptitle("test_text_urls", url="http://test_text_urls.matplotlib.org")
Copy link
Member

Choose a reason for hiding this comment

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

The pep8 violation is that this line is too long (83 char).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks - fixed and DRYed. I omitted the return value assignment and tweaked the whitespace a bit for clarity (I hope). Any other feedback very welcome.

@mdengler
Copy link
Contributor Author

The PEP8 issue has been addressed.

@tacaswell
Copy link
Member

Can you re-base? This will no longer merge cleanly.

@mdengler
Copy link
Contributor Author

Sure, rebased against matplotlib master.

@tacaswell
Copy link
Member

Travis says there is still issues in ERROR: matplotlib.tests.test_backend_svg.test_text_urls.test.

@tacaswell
Copy link
Member

needs another rebase (probably conflicts in 'whats_new.rst')

@tacaswell
Copy link
Member

@mdengler Can you rebase this onto current master again?

@mdengler
Copy link
Contributor Author

mdengler commented Jan 5, 2014

Rebased and addressed the Travis issues (hopefully).

The SVG backend does nothing with Text object's URLs.  In particular, the rendering path below
backend_svg.RendererSVG.draw_text does nothing with the provided GraphicsContext's _url field.
@mdengler
Copy link
Contributor Author

mdengler commented Jan 5, 2014

Travis looks good.

tacaswell added a commit that referenced this pull request Jan 5, 2014
Fix backend_svg.RendererSVG.draw_text to render urls
@tacaswell tacaswell merged commit 2d72977 into matplotlib:master Jan 5, 2014
@mdengler
Copy link
Contributor Author

mdengler commented Mar 7, 2014

Thanks very much for the merge.

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.

None yet

4 participants