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

Refactor the classes #154

Merged
merged 27 commits into from
Jul 16, 2021
Merged

Refactor the classes #154

merged 27 commits into from
Jul 16, 2021

Conversation

lesamouraipourpre
Copy link
Contributor

The aim of this refactor is as follows:

  • Deduplicate any code in the subclasses and move it up to LabelBase
  • Where code only applies to one of the subclasses, have no knowledge of it in the other subclass or the base class.
  • Remove max_glyphs - the removal of max_size from displayio.Group makes it redundant. - Try to remove max_glyphs restriction from label #131
  • Make all variables private/protected (start with an _). Note: This may break any external code which relied on access to these.

These changes have been tested on a PyPortal. I can not see any problems with the examples but it's possible I'm missing a fault.

@jposada202020
Copy link
Contributor

Just to open the discussion here, for me I would keep some of the variables public:

  • anchor_position
  • anchored_position
  • base_alignment
  • label_direction
    As any change in these variables will change library behavior. However, this is just a personal(functional) preference. So no problem either way.
    Also, you mentioned that you test the examples, so I guess we would cover a lot of the example code in the learning guides.

@lesamouraipourpre
Copy link
Contributor Author

anchor_point, anchored_position & label_direction are all available as properties of LabelBase
base_alignment is currently only a constructor parameter but could easily be made into a property as well.

@jposada202020
Copy link
Contributor

good it is solved then could do PR for the basealignment after :)

@FoamyGuy
Copy link
Contributor

FoamyGuy commented Jul 14, 2021

The PyPortal example: https://github.com/lesamouraipourpre/Adafruit_CircuitPython_Display_Text/blob/refactor/examples/display_text_pyportal.py

crashes with this traceback on the branch from this PR:

Traceback (most recent call last):
  File "code.py", line 41, in <module>
AttributeError: 'Label' object has no attribute 'height'

it looks like height was previously set in the constructor here:

self.height = self._font.get_bounding_box()[1]

but was made private with the leading underscore in these changes.

Perhaps we should make an explicit height (and width) property that acts as a convenience accessor for bounding_box so that they actually get documented as properties rather than just happening to exist by virtue of being set in __init__()

I'm working on testing the rest of the examples as well.

@FoamyGuy
Copy link
Contributor

I tested all of the other examples and all are working as intended. I looked over these changes and everything looks good to me beyond the height property issue mentioned above.

The only other thought that I had is it might be worthwhile to temporarily (for a few months / release versions) check if user code passed in a max_glyphs and if so print a warning message that tells them it's no longer necessary and can be safely removed. I'm not 100% sure if we do want to do this though as it could lead to extra "spam" being printed in the serial console and there does not seem to be any harm in user code passing it in with this version, it just gets ignored. It may be worth asking some more of the team members to see if folks think it's a good idea or not.

Thanks for working on this @lesamouraipourpre!

@lesamouraipourpre
Copy link
Contributor Author

I've added height and width properties to LabelBase. The previously defined height and width were defined differently on Label and BitmapLabel. They are now defined on the root class based on the bounding_box.

max_glyphs - I've added a printed message "Please update your code: 'max_glyphs' is not needed anymore."

Copy link
Contributor

@FoamyGuy FoamyGuy 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. I Re-tested the display_text_pyportal.py example successfully after the latest commits and all is well.

Thanks again @lesamouraipourpre

@FoamyGuy FoamyGuy merged commit c31c0ef into adafruit:main Jul 16, 2021
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Jul 17, 2021
Updating https://github.com/adafruit/Adafruit_CircuitPython_DisplayIO_SH1107 to 1.3.0 from 1.2.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_DisplayIO_SH1107#7 from ladyada/main
  > Merge pull request adafruit/Adafruit_CircuitPython_DisplayIO_SH1107#6 from lesamouraipourpre/max-size
  > Moved CI to Python 3.7
  > Added help text and problem matcher
  > Added pull request template

Updating https://github.com/adafruit/Adafruit_CircuitPython_DisplayIO_SSD1305 to 1.2.0 from 1.1.4:
  > Merge pull request adafruit/Adafruit_CircuitPython_DisplayIO_SSD1305#12 from lesamouraipourpre/max-size
  > Moved default branch to main
  > Moved CI to Python 3.7
  > Added help text and problem matcher
  > Added pull request template
  > "Increase duplicate code check threshold "

Updating https://github.com/adafruit/Adafruit_CircuitPython_DisplayIO_SSD1306 to 1.4.0 from 1.3.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_DisplayIO_SSD1306#21 from lesamouraipourpre/max-size
  > Moved default branch to main
  > Moved CI to Python 3.7
  > Added help text and problem matcher
  > Added pull request template
  > "Increase duplicate code check threshold "

Updating https://github.com/adafruit/Adafruit_CircuitPython_HX8357 to 1.3.0 from 1.2.6:
  > Merge pull request adafruit/Adafruit_CircuitPython_HX8357#15 from lesamouraipourpre/max-size
  > Moved default branch to main
  > Moved CI to Python 3.7
  > Added help text and problem matcher
  > Added pull request template
  > "Increase duplicate code check threshold "

Updating https://github.com/adafruit/Adafruit_CircuitPython_IL0373 to 1.3.9 from 1.3.8:
  > Merge pull request adafruit/Adafruit_CircuitPython_IL0373#22 from lesamouraipourpre/ondiskbitmap-changes
  > Moved default branch to main
  > Moved CI to Python 3.7

Updating https://github.com/adafruit/Adafruit_CircuitPython_IL0398 to 1.1.7 from 1.1.6:
  > Merge pull request adafruit/Adafruit_CircuitPython_IL0398#11 from lesamouraipourpre/ondiskbitmap-changes
  > Moved default branch to main
  > Moved CI to Python 3.7

Updating https://github.com/adafruit/Adafruit_CircuitPython_ILI9341 to 1.3.0 from 1.2.6:
  > Merge pull request adafruit/Adafruit_CircuitPython_ILI9341#27 from lesamouraipourpre/max-size
  > Moved default branch to main
  > Moved CI to Python 3.7
  > Added help text and problem matcher
  > Added pull request template
  > "Increase duplicate code check threshold "

Updating https://github.com/adafruit/Adafruit_CircuitPython_SSD1327 to 1.3.0 from 1.2.2:
  > Merge pull request adafruit/Adafruit_CircuitPython_SSD1327#10 from lesamouraipourpre/max-size
  > Moved default branch to main
  > Moved CI to Python 3.7
  > Added help text and problem matcher
  > Added pull request template
  > "Increase duplicate code check threshold "

Updating https://github.com/adafruit/Adafruit_CircuitPython_SSD1331 to 1.3.0 from 1.2.5:
  > Merge pull request adafruit/Adafruit_CircuitPython_SSD1331#14 from lesamouraipourpre/max-size
  > Moved default branch to main
  > Moved CI to Python 3.7
  > Added help text and problem matcher
  > Added pull request template
  > "Increase duplicate code check threshold "

Updating https://github.com/adafruit/Adafruit_CircuitPython_SSD1351 to 1.3.0 from 1.2.5:
  > Merge pull request adafruit/Adafruit_CircuitPython_SSD1351#16 from lesamouraipourpre/max-size
  > Moved default branch to main
  > Moved CI to Python 3.7
  > Added help text and problem matcher
  > Added pull request template
  > "Increase duplicate code check threshold "

Updating https://github.com/adafruit/Adafruit_CircuitPython_SSD1608 to 1.2.8 from 1.2.7:
  > Merge pull request adafruit/Adafruit_CircuitPython_SSD1608#11 from lesamouraipourpre/ondiskbitmap-changes
  > Moved default branch to main
  > Moved CI to Python 3.7

Updating https://github.com/adafruit/Adafruit_CircuitPython_SSD1675 to 1.1.7 from 1.1.6:
  > Merge pull request adafruit/Adafruit_CircuitPython_SSD1675#10 from lesamouraipourpre/ondiskbitmap-changes
  > Moved default branch to main
  > Moved CI to Python 3.7

Updating https://github.com/adafruit/Adafruit_CircuitPython_SSD1680 to 1.0.2 from 1.0.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_SSD1680#2 from lesamouraipourpre/ondiskbitmap-changes
  > Moved CI to Python 3.7

Updating https://github.com/adafruit/Adafruit_CircuitPython_SSD1681 to 1.0.4 from 1.0.3:
  > Merge pull request adafruit/Adafruit_CircuitPython_SSD1681#5 from lesamouraipourpre/ondiskbitmap-changes
  > Moved CI to Python 3.7

Updating https://github.com/adafruit/Adafruit_CircuitPython_ST7735 to 1.2.0 from 1.1.5:
  > Merge pull request adafruit/Adafruit_CircuitPython_ST7735#15 from lesamouraipourpre/max-size
  > Moved default branch to main
  > Moved CI to Python 3.7
  > Added help text and problem matcher
  > Added pull request template
  > "Increase duplicate code check threshold "

Updating https://github.com/adafruit/Adafruit_CircuitPython_ST7789 to 1.5.0 from 1.4.5:
  > Merge pull request adafruit/Adafruit_CircuitPython_ST7789#23 from lesamouraipourpre/max-size
  > Moved default branch to main
  > Moved CI to Python 3.7
  > Added help text and problem matcher
  > Added pull request template
  > "Increase duplicate code check threshold "

Updating https://github.com/adafruit/Adafruit_CircuitPython_Display_Text to 2.20.0 from 2.18.6:
  > Merge pull request adafruit/Adafruit_CircuitPython_Display_Text#154 from lesamouraipourpre/refactor

Updating https://github.com/adafruit/Adafruit_CircuitPython_Gizmo to 1.3.3 from 1.3.2:
  > Merge pull request adafruit/Adafruit_CircuitPython_Gizmo#20 from lesamouraipourpre/ondiskbitmap-changes
  > Merge pull request adafruit/Adafruit_CircuitPython_Gizmo#19 from lesamouraipourpre/max-size
  > Moved default branch to main

Updating https://github.com/adafruit/Adafruit_CircuitPython_MIDI to 1.4.1 from 1.4.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_MIDI#36 from lesamouraipourpre/patch-1
  > Moved default branch to main
  > Merge pull request adafruit/Adafruit_CircuitPython_MIDI#35 from 4dcu-be/master
  > Moved CI to Python 3.7
  > Added help text and problem matcher
  > Added pull request template

Updating https://github.com/adafruit/Adafruit_CircuitPython_MiniMQTT to 5.1.0 from 5.0.7:
  > Merge pull request adafruit/Adafruit_CircuitPython_MiniMQTT#84 from PhearZero/patch-1
  > Moved default branch to main
  > Moved CI to Python 3.7
  > Added help text and problem matcher
  > Added pull request template

Updating https://github.com/adafruit/Adafruit_CircuitPython_PyBadger to 3.3.0 from 3.2.3:
  > Merge pull request adafruit/Adafruit_CircuitPython_PyBadger#45 from lesamouraipourpre/max-size
  > Moved default branch to main
  > Moved CI to Python 3.7
  > Added help text and problem matcher
  > Added pull request template
  > "Increase duplicate code check threshold "
  > Merge pull request adafruit/Adafruit_CircuitPython_PyBadger#43 from FoamyGuy/pylintrc

Updating https://github.com/adafruit/Adafruit_CircuitPython_PYOA to 2.3.0 from 2.2.4:
  > Merge pull request adafruit/Adafruit_CircuitPython_PYOA#23 from lesamouraipourpre/max_size
  > Moved default branch to main
  > Moved CI to Python 3.7
  > Added help text and problem matcher
  > Added pull request template
  > "Increase duplicate code check threshold "

Updating https://github.com/adafruit/Adafruit_CircuitPython_Pypixelbuf to 2.2.6 from 2.2.5:
  > Merge pull request adafruit/Adafruit_CircuitPython_Pypixelbuf#32 from dunkmann00/revert-31-slice-fix
  > Moved default branch to main
  > Moved CI to Python 3.7
  > Added help text and problem matcher
  > Added pull request template

Updating https://github.com/adafruit/Adafruit_CircuitPython_turtle to 2.2.0 from 2.1.5:
  > Merge pull request adafruit/Adafruit_CircuitPython_turtle#24 from lesamouraipourpre/max-size
  > Moved default branch to main
  > Moved CI to Python 3.7
  > Added help text and problem matcher
  > Added pull request template
  > "Increase duplicate code check threshold "

Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA:
  > Added the following libraries: Adafruit_CircuitPython_Pixelbuf
@lesamouraipourpre lesamouraipourpre deleted the refactor branch July 18, 2021 15:24
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.

3 participants