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

add justify= to .string() and deprecate right= #469

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

drtonyr
Copy link
Contributor

@drtonyr drtonyr commented Oct 8, 2023

Unfortunately I got in a right mess with #459 and no longer had the confidence I was doing the right thing.

This PR addresses the known issues with #459, namely it makes minimal changes to the documentation and is a single commit.

@drtonyr
Copy link
Contributor Author

drtonyr commented Nov 9, 2023

bump

Yes, I know I screwed up with the last PR (#459) getting everything into one merge. However, I don't think this is a complex issue and I do think I addressed all concerns.

Let me try to make a strong case for this PR. Screen drawing is slow, if we call fill() and start again it's visible. If we want to update just part of the screen with text then we really don't want to blank it and write an unknown length, we really want to make one call with a width and it simply overwrites what's there. So all calls to .string() really should have width= set and we can't do that at the moment if we want left justification. So we need this PR.

I have a watch face which also displays the tide times for holiday use. When I switch location it used to blank the screen and redrew at a new location which was very noticeable. I've just updated it so that it redraws on the old screen with the new location data, it looks so much nicer. Unfortunately I don't have left justification - it's waiting on this PR (not that I'd be offended if nobody wanted my tide times - it is niche).

@GaryM0101
Copy link

GaryM0101 commented Nov 9, 2023 via email

@fgaz
Copy link
Member

fgaz commented Nov 9, 2023

I thought Daniel passed you the torch to merge this and other PR's into the current source, or did I miss something?

Conditional on one approval, which is missing. Let me have a look...

@drtonyr
Copy link
Contributor Author

drtonyr commented Nov 9, 2023

@GaryM0101 Tides has been a lot of fun. I started in early September after returning from a holiday house that had a broken tide clock. I thought it would only take a couple of weeks to do something much better. Well, it is much better, just it took me a couple of months. I'll write it up in the PR, there's a fair amount to talk about.

@fgaz Thanks!

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.

None yet

3 participants