Mutable height and width

FoamyGuy commented 3 years ago

height and width properties that can be set to update the size of the button.

These changes allow the button to be used with the GridLayout from: https://github.com/adafruit/Adafruit_CircuitPython_DisplayIO_Layout/pull/2

in order to create a grid with different sized cells like this:

kmatch98 commented 3 years ago

I reviewed the updated code and makes sense to me. Did not verify on hardware.

Thanks @foamyguy for all this work on buttons!

kmatch98 commented 3 years ago

I did some hardware testing on the PyPortal and ran into a couple of things for clarification. I did a few checks of this latest button along with the grid_layout tool.

The only major issue I found is the first one, the second are more topics for discussion or minor documentation things.

I didn't check the reaction of the buttons, will add that to my todo list.

Issue 1 - Bug

I ran this code and received an error.

button_launch = Button(x=0, y=0, width=1, height=1, label="LAUNCH", label_font=terminalio.FONT)

Error message:

Traceback (most recent call last):
  File "code.py", line 24, in <module>
  File "/lib/adafruit_button.py", line 173, in __init__
ValueError: Layer already in a group.

I think the issue is caused in _create_body on line 120 where it appends self.append(self.body). I commented out line 120 and it worked fine after that. I also see that the width and height setters also call _create_body and append the body to the self group. With Line 120 commented out, the width and height setters worked fine for me.

Issue 2 - Handling text that doesn't fit inside the button

If the text won't fit into the button, the code crashes with RuntimeError: Button not large enough for label. I'd prefer a softer landing, either truncating or wrapping the text and giving a warning.

code.py output:
Traceback (most recent call last):
  File "code.py", line 28, in <module>
  File "/lib/adafruit_button.py", line 175, in __init__
  File "/lib/adafruit_button.py", line 196, in label
RuntimeError: Button not large enough for label

Issue 3 - Usage: I couldn't figure out how to use `style`

It took me a while to realize to use Button.ROUNDRECT to change the style. I tried style='RECT' and nothing showed up on the screen. It would help to add this clarification to the docs under the style parameter. Also, if the style doesn't match any existing styles, I would prefer to get an error rather than just a blank screen.

Issue 4 - Usage: `selected` versus `value` parameter

In the definition of Widget and Control classes, I envisioned a selected function that responds to touch_down events, and a separate value that is a state variable. The selected function could modify the value depending upon the Widget's definition.

Do you think this makes sense as a generic definition for how Widgets should respond? Or do you think there is a better way to define functions for generic response of Widgets and Control for "touch_down", "still_touch" and "touch_up/released" actions?

Idea #5 - Is a button just like a resizable label?

Just some open-ended questions about buttons vs. labels: In using the button, I realized that a button looks like a resizable text label. Can you reuse the button code for your resizable label? Alternately, should a button just be a label that has response to touch added?

FoamyGuy commented 3 years ago

I've updated to include a resize(new_width, new_height) function which can be used instead of the width and height properties. I made a new commit over in the Layout repo PR that makes use of this new function.

1: good catch thank you! fixed in latest commits

2: I've updated it to to attempt to truncate the text to fit. Another potential option could be making use of the new wrap by pixels once it's merged.

3: I agree with both suggestions, but I think both the updated doc string, and more graceful error handling could come in another PR.

4: I think the ideal end goal would be "touch_down", "still_touch" and "touch_up/released" but I'm not sure that we can get there with out more of an event system fleshed out that will can "watch" the touch screen in the background and give us new data when we check. I think as it is now we could miss these states if the user doesn't call the "checking" function quick enough. I do like what you've laid out for selected and value I think. Though I'm not sure if we'll ultimately want to bring that into this button or not, it may not be backward compatible with the current way this works. I think it's definitely worth more discussion and brainstorming.

5: Button and Label (especially if it were made "clickable" via Widget) are indeed very similar concepts. I think the main specific difference at the moment is buttons ability to have different styles, and to have border color effects. In the future I see opportunity for an ImageButton or similar that makes the same kind of shapes as the button but allows Bitmap image instead of text.

kmatch98 commented 3 years ago

I verified the change and it works fine for me! Thanks for the updates on this button, now along with grid_layout and the resize function, this is a great combination. Thanks for making these new features!

Looks good to me.

FoamyGuy commented 3 years ago

The new PyLint check for duplicate lines caused this to fail when merged. I will hold off on the release until we have the fix for that.

adafruit / Adafruit_CircuitPython_Display_Button