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

Revert "Revert "2024.1.0: Advanced clock - draw date"" #199

Conversation

andrewjswan
Copy link

Reverts #198

Are you sure you haven't forgotten about advanced_clock: true ?

@andrewjswan
Copy link
Author

But when I try to make a PR with such a configuration file, I get an error. https://github.com/lubeda/EspHoMaTriXv2/actions/runs/7501946835/job/20423611396
[date_format_big] is an invalid option for [ehmtxv2]. Did you mean [time_format_big], [date_format], [time_format]?
Although this is specified in the Python. https://github.com/lubeda/EspHoMaTriXv2/pull/199/files#diff-b95092b415899adcf850b1e4115261c02d0ae5a352deedf4801b5ca740b1f6cb
Maybe the problem is in Github Action?

@andrewjswan
Copy link
Author

andrewjswan commented Jan 12, 2024

I did an experiment in my repository, made a similar PR (Revert, Revert) accepted it in 2024.1.0,
made a PR with a test change in the ulanzi-test.yaml configuration,
https://github.com/andrewjswan/EspHoMaTriXv2/commits/2024.1.0-prerelease/
andrewjswan@4c1fc95
https://github.com/andrewjswan/EspHoMaTriXv2/blob/4c1fc95bb73025611e2739cdbdf8fc4b54e16c39/tests/ulanzi-test.yaml#L272-L282
and everything build without errors.
https://github.com/andrewjswan/EspHoMaTriXv2/actions/runs/7503152730/job/20427316577

@andrewjswan
Copy link
Author

If we want to make advanced_clock be the default and always work, then we can enable it by replacing the default value with true
https://github.com/lubeda/EspHoMaTriXv2/blob/2024.1.0-prerelease/components/ehmtxv2/__init__.py#L168-L170

@lubeda
Copy link
Owner

lubeda commented Jan 19, 2024

Is true as default value a breaking change?
I mean, do I have to use set_clock_infotext_color or set_date_infotext_color or does the "normal" clock work as before?

If it's not breaking, fine, and perhaps this could lead to a little code clean up. e.g. set_clock_color internally calls set_clock_infotext_color and set_date_infotext_color. So only advanced users need to care and normal users (like me) have to change nothing.

Sorry, I'm low on spare time.

@andrewjswan
Copy link
Author

Is true as default value a breaking change?

No, everything should work as before without problems.

I mean, do I have to use set_clock_infotext_color or set_date_infotext_color or does the "normal" clock work as before?

When calling set_infotext_color, the color will be set to the same color for set_clock_infotext_color and set_date_infotext_color i.e. you can still have one method setting the color on all screens or do it individually
https://github.com/andrewjswan/EspHoMaTriXv2/blob/2024.1.0-prerelease/components/ehmtxv2/EHMTX.cpp#L887-L898

So only advanced users need to care and normal users (like me) have to change nothing.

When this mode is enabled, everything should continue to work as before, i.e. There should be no changes in the rendering or filling of regular methods.

But we can leave this mode disabled by default (as it is now). And then any user will be able to read the documentation, enable it and customize it to their taste.

@andrewjswan
Copy link
Author

Sorry, I'm low on spare time.

It's okay, I use this mode every day, so far there have not been a single error or incorrect operation. And we are in no hurry, unless of course there are plans to release the 2024.1.0 release right now.
But in my opinion, in its current state the firmware is quite stable and functional.

@lubeda lubeda merged commit 32f6395 into lubeda:2024.1.0-prerelease Jan 27, 2024
1 check passed
@andrewjswan andrewjswan deleted the revert-198-revert-197-2024.1.0-Advanced_clock_draw_date branch January 29, 2024 09:41
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

2 participants