Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mutable height and width #27

Merged
merged 4 commits into from
Feb 23, 2021
Merged

Conversation

FoamyGuy
Copy link

@FoamyGuy FoamyGuy commented Feb 1, 2021

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: adafruit/Adafruit_CircuitPython_DisplayIO_Layout#2

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

@kmatch98
Copy link

kmatch98 commented Feb 1, 2021

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

Thanks @FoamyGuy for all this work on buttons!

@kmatch98
Copy link

kmatch98 commented Feb 8, 2021

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
Copy link
Author

FoamyGuy commented Feb 9, 2021

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
Copy link

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 FoamyGuy merged commit 0d30f69 into adafruit:master Feb 23, 2021
@FoamyGuy
Copy link
Author

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-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Feb 26, 2021
Updating https://github.com/adafruit/Adafruit_CircuitPython_DS18X20 to 1.3.4 from 1.3.3:
  > Merge pull request adafruit/Adafruit_CircuitPython_DS18X20#21 from adafruit/dherrada-patch-1

Updating https://github.com/adafruit/Adafruit_CircuitPython_EPD to 2.8.0 from 2.7.2:
  > Merge pull request adafruit/Adafruit_CircuitPython_EPD#45 from makermelissa/master

Updating https://github.com/adafruit/Adafruit_CircuitPython_GPS to 3.7.0 from 3.6.8:
  > Merge pull request adafruit/Adafruit_CircuitPython_GPS#54 from lesamouraipourpre/parse-gsa-and-gsv

Updating https://github.com/adafruit/Adafruit_CircuitPython_RGB_Display to 3.10.6 from 3.10.5:
  > Merge pull request adafruit/Adafruit_CircuitPython_RGB_Display#90 from makermelissa/master

Updating https://github.com/adafruit/Adafruit_CircuitPython_SSD1306 to 2.11.0 from 2.10.2:
  > Merge pull request adafruit/Adafruit_CircuitPython_SSD1306#57 from adamcandy/add-page-addressing-mode

Updating https://github.com/adafruit/Adafruit_CircuitPython_Display_Button to 1.5.2 from 1.5.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_Display_Button#27 from FoamyGuy/resizeable_button

Updating https://github.com/adafruit/Adafruit_CircuitPython_Display_Text to 2.15.0 from 2.14.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_Display_Text#111 from FoamyGuy/wrap_by_pixels

Updating https://github.com/adafruit/Adafruit_CircuitPython_LED_Animation to 2.5.3 from 2.5.2:
  > Merge pull request adafruit/Adafruit_CircuitPython_LED_Animation#75 from adafruit/REUSE
  > Hardcoded Black and REUSE versions
  > Added pre-commit-config file
  > Added pre-commit and SPDX copyright

Updating https://github.com/adafruit/Adafruit_CircuitPython_MiniMQTT to 5.0.3 from 5.0.2:
  > Merge pull request adafruit/Adafruit_CircuitPython_MiniMQTT#65 from brentru/update-cpython-example
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants