-
-
Notifications
You must be signed in to change notification settings - Fork 938
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 watch face to enum #1339
Conversation
I think we need to add another array to the CheckboxList with the values of each option. Right now, the values default to |
I guess I will put this PR on hold until #1351 is figured out. |
ba13a1d
to
8161387
Compare
Testing results right now: pressing the settings button in the quick settings opens the quick settings again with an animation. The settings do not show at all. |
New testing result: it runs all well, except the background image of the Infineat watch face is missing. The Casio watch face looks as expected. Edit: My fault. I didn't remember that I turned off the background 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commits should be fixed according to the commit conventions https://github.com/InfiniTimeOrg/InfiniTime/blob/develop/CONTRIBUTING.md#commit-conventions
src/displayapp/WatchFaces.h
Outdated
|
||
namespace Pinetime { | ||
namespace Applications { | ||
enum class WatchFace : uint32_t { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By using uint8_t we should be able to avoid updating the settings version and avoid an unnecessary settings reset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, and to do that we would also need to explicitly give the enumerations values like with WakeUpMode, because otherwise the values are undefined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CheckboxList
uses uint32_t
. I still changed the enum to uint8_t
since it should be fine.
Can I just squash them? |
6c32e16
to
15e84e8
Compare
Build size and comparison to main:
|
I think you forgot to update the submodules in your local repo before committing. |
All the commits seem to be fixing issues from previous commits, so they can all be squashed. |
Given the current ideas regarding getting rid of the apps enum, I am not sure if this PR makes sense anymore. |
I'm sure this PR would be merged if the issues were fixed. |
Alright, here we go 😄 |
8fb2a41
to
0f676ed
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the issue relates to this PR. #1376
This can be squash merged.
Thank you. 😊 I wasn't aware of this issue anymore. |
Support for PR: InfiniTimeOrg/InfiniTime#1339
This PR introduces the same enum-implementation for watch faces that already exists for apps. I think it is cleaner than using
uint8_t
.The implementation of
CheckboxList uses
uint32_t` internally, so there is a conversion in between.