Allow label to be scaled #42
Conversation
| self._label = Label(self._label_font, text=newtext, scale=self._label_scale) | ||
| dims = list(self._label.bounding_box) | ||
| dims[2] *= self._label.scale | ||
| dims[3] *= self._label.scale |
There was a problem hiding this comment.
I'm not in love with having to do this with the bounding box dimensions. As I mentioned in the description, I'm not sure if it was intentional or not that the bounding box for Label isn't scaled, but since that's a separate repository, I thought I'd just work with what we have. I'm open to suggestions here.
There was a problem hiding this comment.
I do think the approach you've gone for here is fine. It may be worth considering changing in Label in the long term, but there could be reasons in there why it's like this, I don't know for certain.
| @@ -72,6 +73,7 @@ def __init__( | |||
| self._label_color = label_color | |||
| self._label_font = label_font | |||
| self._selected_label = _check_color(selected_label) | |||
| self._label_scale = label_scale or 1 | |||
There was a problem hiding this comment.
This is passed in to Label which wouldn't like a None value for scale, so this defaults the value to 1. I prefer for optional parameters to default to None, that way callers can pass in a None and get default behavior. But if it's desirable to set the default on the parameter to 1 as Label does it, I can change it.
There was a problem hiding this comment.
@lcmcninch Thanks for adding this functionality!
I think it's best if we can keep the different buttons having as similar of arguments as possible. Can you add the new label_scale argument over here inside of SpriteButton as well: https://github.com/adafruit/Adafruit_CircuitPython_Display_Button/blob/main/adafruit_button/sprite_button.py#L64 that way if folks swap from one type of button to the other this part of the functionality can remain consistent.
Absolutely, that's a good call, I'll make the change. |
|
@FoamyGuy I added the |
There was a problem hiding this comment.
Looks good to me. Thanks @lcmcninch!
I tested this successfully on a PyPortal 9.0.0alpha.2 with the simpletest and spritebutton simpletest slightly modified to use the new parameter.
Really nice functionality to have especially with lots of larger resolution displays gaining support.
| self._label = Label(self._label_font, text=newtext, scale=self._label_scale) | ||
| dims = list(self._label.bounding_box) | ||
| dims[2] *= self._label.scale | ||
| dims[3] *= self._label.scale |
There was a problem hiding this comment.
I do think the approach you've gone for here is fine. It may be worth considering changing in Label in the long term, but there could be reasons in there why it's like this, I don't know for certain.
Updating https://github.com/adafruit/Adafruit_CircuitPython_Display_Button to 1.9.0 from 1.8.1: > Merge pull request adafruit/Adafruit_CircuitPython_Display_Button#42 from lcmcninch/button_label_scale Updating https://github.com/adafruit/Adafruit_CircuitPython_PyCamera to 0.0.3 from 0.0.1: > Merge pull request adafruit/Adafruit_CircuitPython_PyCamera#5 from adafruit/qrio-demo-fix Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA: > Updated download stats for the libraries
Problem
When creating a
Button, there isn't a way to set the scale for the label font, so the label for bigger buttons can be tiny. See #41.Changes
__init__forButtonandButtonBasenow take an optionallabel_scaleparameter, which ultimately defaults to 1label, when theLabelis instantiated, it is passed the label scale. Then, to ensure the label is centered, the bounding box is modified by the scale factor as necessary.Testing
I don't have a ton of experience with this library or its dependencies, so I admittedly didn't do much testing. My application uses some buttons with the font scaled to 2 and 4 and everything seems to work okay. I'm open to doing more testing if there are suggestions.
Notes
Label's bounding box isn't scaled, and I considered submitting a pull request in that repository to add ascaled_bounding_boxproperty, but I figured the way the dependencies are for these libraries, it would be best for this change to be compatible with the current and previous Adafruit_CircuitPython_Display_Text versions.