Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Make Text._get_layout simpler to follow.#12951
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
Also would benefit from renaming some few-character variables, e.g.d -> descent
| baseline = (h - d) - thisy | ||
| if i == 0: |
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.
| ifi==0: | |
| ifthisy==0:# first line |
gets rid of the need forenumerate().
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.
It's not immediately obvious that thisy cannot be zero for the second line too even if the first line is empty (it's true, but only thanks to theh = max(h, lp_h)/d = max(d, lp_d) lines) so I think leaving it as it is is clearer.
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.
Just by common sense, the vertical positionthisy must increase also for empty lines. So it should only be zero before handling the first line.
But ok, if you want to leave 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.
I don't think common sense applies to this function (or to a lot of matplotlib, sadly). (e.g. if you look at the way center_baseline is defined a bit below, it looks weirdly different between anchor and non-anchor rotations...)
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.
Funnily that got fixed by#13029 :)
| horizLayout[i] = thisx, thisy, w, h | ||
| thisy -= max(min_dy, (h - d) * self._linespacing) | ||
| xs.append(thisx) # == 0. |
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.
Alterantively
xs = [0.] * len(ys)outside of the loop.
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.
I think the symmetry of handling between xs and ys is nicer.
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.
IMOxs = [0.] * len(ys) is much clearer aboutxs than secretly constructing a list of zeros in the loop. The loop should only need to bother about the quantities that actually depend on the lines. But not going to argue about this.
The single-character variables are "inside the loop", the longer ones are the one that outlive the loop. |
Which still doesn't make the single-character variables readable. |
timhoffm left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Accepting as an incremental improvement, even though I see room for further improvement.
The single-letter variables are defined as so their meaning is pretty clear IMO (in the context). |
That's fine in a narrow scope like a list comprehension or a few-lines |
Additionally factored out offsets computation, following#13029. |
- Instead of constructing and accessing multidimensional arrays whs and horizLayout by (opaque) index, construct separate lists with more meaningful names.- Define xmin/xmax/ymin/ymax/width/height closer to their place of use.- Make computation of offsetx and offsety more symmetric between rotation_mode == "anchor" and == "default".
horizLayout by (opaque) index, construct separate lists with more
meaningful names.
rotation_mode == "anchor" and == "default".
PR Summary
PR Checklist