Make Text._get_layout simpler to follow. #12951
Conversation
| baseline = (h - d) - thisy | ||
|
|
||
| if i == 0: |
There was a problem hiding this comment.
| if i == 0: | |
| if thisy == 0: # first line |
gets rid of the need for enumerate().
There was a problem hiding this comment.
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 the h = 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.
Just by common sense, the vertical position thisy 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.
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.
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.
Alterantively
xs = [0.] * len(ys)
outside of the loop.
There was a problem hiding this comment.
I think the symmetry of handling between xs and ys is nicer.
There was a problem hiding this comment.
IMO xs = [0.] * len(ys) is much clearer about xs 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. |
|
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