Skip to content

Adding the wrap_by_pixels function #111

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

Merged
merged 12 commits into from
Feb 24, 2021
Merged

Conversation

FoamyGuy
Copy link
Contributor

Jeff wrote this nice helper function for wrapping text to a number of pixels instead of characters.

This PR adds that function as well as an example that illustrates it's usage.

One quirk worth consideration is that the new wrap_by_pixels function returns a string with newlines inserted. Whereas the older wrap_by_characters function returns a list of the lines, and joining with the newline is left up to the code using it. I think I like the way this one does it better with the string. But the other one came from the PyPortal libraries originally, perhaps there is something in them that is using the list. Maybe it would be worthwhile to add a parameter to both that controls this behavior?

@ladyada
Copy link
Member

ladyada commented Jan 31, 2021

id kinda prefer the list-return version - can you adapt it to match?

@ladyada ladyada requested a review from evaherrada January 31, 2021 19:57
@evaherrada
Copy link
Collaborator

@FoamyGuy I'll test this sometime in the next few days

@kmatch98
Copy link
Contributor

I ran several scenarios to sort out the behavior with miscellaneous white space. I found one bug that adds a leading newline. The other two things may be your planned behavior, so my only suggestion is to add a couple lines of documentation to clarify how newlines and leading/trailing whitespace is treated.

Bug?

Input string: text = "THISISasuperlongstringwithnospacesthatwillnotfit"
Output: ['', 'THISISasuperlongstring-', 'withnospacesthatwillno-', 'tfit']
Adds an extra newline at the beginning.

For Documentation

Input string: text = "\t A short \ntest"
Output: ['A short test']
Behavior: Newlines and tabs are obliterated.
(Unrelated to this, I found that label and bitmap_label both ignore tabs. I'll add an issue.)

For Documentation

Input string: text = " Leading and trailing spaces are deleted. "
Output: ['Leading and trailing', 'spaces are deleted.']
Behavior: Leading and trailing spaces are deleted.

@FoamyGuy
Copy link
Contributor Author

Thanks for looking into this @kmatch98!

Definitely a bug on the first one.

I lean toward bug on \ns getting removed as well. It's probably best not to use them if you want the function to do the wrapping, but if the user includes them we should try to keep them if we can. Tabs we can look at once they get fixed in label and bitmap_label.

Agreed about documenting the removal of leading and trailing spaces. For leading spaces the indend0 parameter can be used to add some spaces ( or anything) to the beginning

I'll work on making these changes to fix and document the above.

@evaherrada
Copy link
Collaborator

@FoamyGuy Currently pretty swamped with other stuff so I'm not sure when I'm gonna get to this. If you're fine waiting a bit (looking like a week at least) I'd still be down to review this, but otherwise I won't be offended if someone else reviews it

@FoamyGuy
Copy link
Contributor Author

Okay, I think this is ready for review again. The latest commits resolve the issues noted above in the following ways:

  • Very long word results in extra newline at the beginning. This was due to the longword logic always starting on the next line down instead of picking up in the current spot of the line. That coupled with the indent0 value was causing the extra line at the beginning. Newest version just starts right where it's at in the partial line instead of jumping down to the next. This avoids the extra newline in the single long word scenario.

  • tabs and newlines removed. Tabs we will need to deal with inside of label and bitmap label. So I didn't do anything specific here for them yet. Newlines no longer get removed, and are now properly respected. So the user can have newlines in the input string to get extra empty lines in the output.

  • leading and trailing whitespace removed. This behavior is now documented along with a note about the indent0 and indent1 parameters that can be used to control leading whitespace if desired.

@kmatch98
Copy link
Contributor

I tested and the this fixes the issues mentioned above. I also verified that the leading and trailing spaces are now retained.

I checked the operation of the indent0 and indent1. I noticed that using the indent1 option always leaves an extra line at the end. Not a big deal, but it's unexpected.

text = "CircuitPython is a programming language"
print(wrap_text_to_pixels(text, WRAP_WIDTH, font, indent0="*", indent1="-"))

Output:

['*CircuitPython is a', '-programming language', '-']

Thanks for these fixes!

@FoamyGuy
Copy link
Contributor Author

Nice catch @kmatch98, thank you! indent1 was getting added an extra time by some code adding partial pat the end an extra time.

The latest commit resolves it, the returned list no longer has an extra item in it.

@kmatch98
Copy link
Contributor

I verified this update, it definitely solves the problem. Thanks for getting this merged into the library and fixing all these details!

Looks good to me.

Copy link
Collaborator

@evaherrada evaherrada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Tested on a PyPortal Titano

@evaherrada evaherrada merged commit d33018a into adafruit:master Feb 24, 2021
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.

4 participants