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

Improvements to oled.centre_text() #329

Merged

Conversation

roryjamesallen
Copy link
Collaborator

Reviewing #328 I noticed some improvements that should have been made long ago to centre_text to allow it to work with larger displays where the 3 line rule is not applicable, and with other character sizes where some numbers were still hard coded for 8x8

@roryjamesallen roryjamesallen added the firmware Software related issue label Feb 6, 2024
@mjaskula
Copy link
Collaborator

mjaskula commented Feb 7, 2024

This reminds me that we also have an implementation of centre_text() in experimental/custom_font.py. I don't think that it should influence the changes in this PR, but we should be aware of it when we finally resolve the three implementations.

Copy link
Collaborator

@mjaskula mjaskula left a comment

Choose a reason for hiding this comment

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

These changes make sense to me.

@roryjamesallen
Copy link
Collaborator Author

This reminds me that we also have an implementation of centre_text() in experimental/custom_font.py. I don't think that it should influence the changes in this PR, but we should be aware of it when we finally resolve the three implementations.

Good point! I looked over that implementation and there isn't anything crucial that's different to the changes in this PR, I think for the X it'll be a bigger overhaul anyway given that larger fonts are more likely to be used on the larger display, so more consideration is probably gonna be needed when it comes to it

@roryjamesallen roryjamesallen merged commit 129476e into Allen-Synthesis:main Feb 8, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
firmware Software related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants