Skip to content

Prefer to the GraphicsContext public API when possible. #8848

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
Jul 10, 2017

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Jul 8, 2017

This allows independent backends to just implement the corresponding
methods, rather than also having to provide the private attributes.

Also simplify the signature/implementation of set_sketch_params.

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/whats_new.rst if major new feature
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@tacaswell
Copy link
Member

The appveyor failures are un-related (see #8849 )

@@ -1164,7 +1165,7 @@ def get_sketch_params(self):
"""
return self._sketch

def set_sketch_params(self, scale=None, length=None, randomness=None):
def set_sketch_params(self, scale=None, length=128, randomness=16):
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 a slight change of behavior in that anything 'falsy' would give the default behavior. My guess is that this was not intended.

This came in via #1329 which has lots of discussion around it.

Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

The simplification to set_sktech_params should be reverted please.

@@ -1164,7 +1165,7 @@ def get_sketch_params(self):
"""
return self._sketch

def set_sketch_params(self, scale=None, length=None, randomness=None):
def set_sketch_params(self, scale=None, length=128, randomness=16):
Copy link
Member

Choose a reason for hiding this comment

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

On a bit more configuration, this breaks passing in None for length and randomness (as this falls through (eventually) to Py::Float(sketch_params[0]); with no extra validation. This should stay consistent with the convention of 'passing None means do the default'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Member

Choose a reason for hiding this comment

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

and by 'configuration' I meant 'consideration'....

This allows independent backends to just implement the corresponding
methods, rather than also having to provide the private attributes.
@anntzer anntzer force-pushed the graphicscontext-cleanup branch from 5859153 to 34aa69a Compare July 8, 2017 23:28
self.patch.set_clip_box(gc._cliprect)
self.patch.set_clip_path(gc._clippath)
self.patch.set_clip_box(gc.get_clip_rectangle())
clip_path = gc.get_clip_path()
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be more correct and possibly fixes bugs? Is it worth tracking down what those might be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's whatever bugs writing mpl_cairo has unearthed. I have a ton of them :-)

@dopplershift dopplershift merged commit 9e2acb6 into matplotlib:master Jul 10, 2017
@anntzer anntzer deleted the graphicscontext-cleanup branch July 10, 2017 05:21
@QuLogic QuLogic added this to the 2.1 (next point release) milestone Jul 10, 2017
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.

4 participants