Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Add improved modification of QuiverKey properties#18794
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| self._initialized = False | ||
| self.zorder = Q.zorder + 0.1 | ||
| def get_x(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Perhaps we can do without {get,set}_{x,y} and just stick to get_position (as for Text)? the backcompat X and Y would be kept for backcompat but can use straight lambdas (X = property(lambda self: self._text.get_position()[0], self.text.set_x) -- settingstale shouldn't really matter here as Text will propagate staleness to the parent Axes anyways)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Agree. We only have get_x on Rectangle and FancyBboxPatch. Let's keep the API size down and only provide get/set_position.
| self._update_text_transform() | ||
| self.stale = True | ||
| @property |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
consider deprecating this, given the potential source of confusion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Agree.
| - ``QuiverKey.text`` | ||
| - ``QuiverKey.vector`` | ||
| - ``QuiverKey.verts`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Perhaps someone may want e.g. to set usetex or math_fontfamily or whatnot on the Text; do you really want to deprecate access to it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Keeping text public is the pragmatic thing to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
None of the suggestions above are blocking, but please consider them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it. |
Additionally, add getters/setters for these properties, and use an`offset_copy` Transform to implement `labelsep`. This makes one lessthing depending on the DPI change signal.
I finally got around to rebasing this, though I don't really remember why I started this PR in the first place. @timhoffm what do you think about the API changes here, mainly the comments from@anntzer? I also had to use |
Commented on@anntzer‘s suggestions. label: Unfortunately, Artist.set_label is tied to the specific meaning: The label an artist gets in a legend. I‘m against repurposing that function for another usecase. See also#27971 (comment). Can‘t we leave the |
Yes, that does seem possible; should we deprecate the |
Let's deprecate all the |
PR Summary
QuiverKeypreviously had several attributes that were saved from the constructor, but these were never linked to the internal Artists that were actually drawn.This adds getters and setters for said properties, and instead of a separate copy of the information, just grabs/sets it on the internal Text child. Additionally, it deprecates access to these internal children; other than
.text, they were always overwritten indraw, so there was no way to make real use of them.PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsand runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).