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

Numeral watchface #298

Merged

Conversation

jessp01
Copy link
Contributor

@jessp01 jessp01 commented Sep 27, 2022

Hello all,

Well done on this excellent project.

I've created this watch face:

numerals

Would be glad to see it merged.

Cheers,

@RuffaloLavoisier RuffaloLavoisier changed the base branch from master to develop September 27, 2022 22:06
Copy link
Member

@RuffaloLavoisier RuffaloLavoisier left a comment

Choose a reason for hiding this comment

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

Since we have a similar series watch, why don't we add the choose whether to display numbers in the web interface?

@RuffaloLavoisier RuffaloLavoisier added the ⭐ enhancement New feature or request label Sep 28, 2022
@uvwxy
Copy link
Member

uvwxy commented Sep 28, 2022

Since we have a similar series watch, why don't we add the choose whether to display numbers in the web interface?

I would be in favour of merging this as a separate watchface. As this uses additional fonts these would be added to the binary size of the OS even if the setting is not used.

By not including the watchfaces at compile time we can exclude additional, watchface specific dependencies at compile time, too.

@RuffaloLavoisier
Copy link
Member

Since we have a similar series watch, why don't we add the choose whether to display numbers in the web interface?

I would be in favour of merging this as a separate watchface. As this uses additional fonts these would be added to the binary size of the OS even if the setting is not used.

By not including the watchfaces at compile time we can exclude additional, watchface specific dependencies at compile time, too.

As the Watchface increases, it takes a long time to find or manage the design you want, so you need to find a way to solve it.

Copy link
Member

@RuffaloLavoisier RuffaloLavoisier left a comment

Choose a reason for hiding this comment

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

LGTM, Additionally, you must add this clock to the Web interface index.

src/apps/watchfaces/OswAppWatchfaceNumerals.cpp Outdated Show resolved Hide resolved
@RuffaloLavoisier
Copy link
Member

Corrected commit log.

@RuffaloLavoisier RuffaloLavoisier self-assigned this Sep 28, 2022
@jessp01
Copy link
Contributor Author

jessp01 commented Sep 29, 2022

Hi all,

I didn't want to make changes to other watchface files without discussing it with you (the maintainers) but generally speaking, I'd be all for the below modifications and am happy to submit them if everyone agrees:

  • One function to draw the numerals; this can accept the font to use (of course, we have to limit the selection to suitable fonts, can't use a 24pt one for example), coordinates, etc as parameters
  • Expose a configuration that draws the numerals conditionally, which can then be checked in applicable watchfaces

If that sounds reasonable, which file should we include the numeral drawing function in? perhaps under include/apps/watchfaces/utils?

Additionally, I thought of adding the day of the month like so:
image

Lastly, @RuffaloLavoisier,

Sometimes this step-counter will not be readable by the user by the second hand of the clock.

Indeed, but I reckon that's true to many different designs, including the one in OswAppWatchfaceMonotimer (although far less frequently) and other smart watch projects as well. I can use a smaller font size for it but personally, I don't think it's too bad as is.
The same statement applies to displaying the day of the month, of course.

LGTM, Additionally, you must add this clock to the Web interface index.
Just so I'm sure, do you mean I should add it to https://github.com/Open-Smartwatch/open-smartwatch.github.io/blob/source/docs/index.md?

I'd also be open to contributing documentation on how to add watchfaces, which could help newcomers.

Thanks,

@RuffaloLavoisier
Copy link
Member

LGTM, I'm happy to merge.

I'd also be open to contributing documentation on how to add watchfaces, which could help newcomers.

Thanks,

https://github.com/Open-Smartwatch/open-smartwatch-os/pull/288/files#diff-1eb7e6ee52c2add1ca7bafdf476d09de534e15be005de55cd51f235cc9f48c75R47

Please refer to this part And please write a brief introduction to this watch because we need to add it to the document (how does the number of steps look, the number of times of the week, etc.).

Copy link
Member

@RuffaloLavoisier RuffaloLavoisier left a comment

Choose a reason for hiding this comment

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

Most of the time, the Watchface uses a variety of similar features and another watchfaces. Therefore, it is better to choose how to reuse the code rather than duplicate it.

And please update display date part :)

include/apps/watchfaces/OswAppWatchfaceNumerals.h Outdated Show resolved Hide resolved
src/apps/watchfaces/OswAppWatchfaceNumerals.cpp Outdated Show resolved Hide resolved
src/apps/watchfaces/OswAppWatchfaceNumerals.cpp Outdated Show resolved Hide resolved
@jessp01
Copy link
Contributor Author

jessp01 commented Sep 29, 2022

Hi @RuffaloLavoisier,

Sorry, I've neglected to commit the changes I made to src/osw_config_keys.cpp before. Did so now.
Shall I also submit a pull to update https://github.com/Open-Smartwatch/open-smartwatch.github.io/blob/source/docs/index.md?

Most of the time, the Watchface uses a variety of similar features and another watchfaces. Therefore, it is better to choose how to reuse the code rather than duplicate it.

I agree but to me, it feels less desirable to rely on code implemented in other watchfaces which is why I made the suggestions in my last comment (#298 (comment)). Please let me know what you think.

And please update display date part :)

Updated. This too can be placed in a util function that can be then used in the different watch faces as per my last comment. Happy to refactor if you find this acceptable.

Thanks,

@RuffaloLavoisier
Copy link
Member

Your commit is based on the master.
This merge builds up unnecessary commit logs.
I got it corrected commit log, but you force-push... 😆
Please use, if you need..

$ git fetch && git pull

@RuffaloLavoisier
Copy link
Member

RuffaloLavoisier commented Sep 30, 2022

Sometimes this step-counter will not be readable by the user by the second hand of the clock.

which is why I made the suggestions in my last comment (#298 (comment)).

That comment is about the overlap between the number of steps and the hands of the clock. This is a (#298 (comment)) problem that makes code reuse, or repetitive code, difficult to maintain later

@RuffaloLavoisier RuffaloLavoisier added the ⏳ pending for discussion This object is queued for internal discussion... label Oct 1, 2022
Jesse Portnoy added 3 commits October 1, 2022 18:31
- Added day of month to drawing function
- Commit missing changes to src/osw_config_keys.cpp
@RuffaloLavoisier
Copy link
Member

Corrected the commit log again...

$ git pull

- Re-use monotimer draw 12 letters
- Re-use print weekday
- Remove unused code
Copy link
Member

@RuffaloLavoisier RuffaloLavoisier left a comment

Choose a reason for hiding this comment

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

LGTM - I've modified and organized some codes. There are more things to organize, but I think it would be okay to do it when I organize the watchface later.

@RuffaloLavoisier RuffaloLavoisier requested review from simonmicro and removed request for simonmicro October 1, 2022 14:13
@RuffaloLavoisier RuffaloLavoisier removed the ⏳ pending for discussion This object is queued for internal discussion... label Oct 1, 2022
@RuffaloLavoisier
Copy link
Member

@jessp01
First, I'm sorry and thank you. It is sometimes good to modify internally like this.
As a result, thanks to your idea, we are able to merge the new watchface.
Thank you for your contribution.

@RuffaloLavoisier RuffaloLavoisier merged commit 61e85bf into Open-Smartwatch:develop Oct 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants