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

Show alarm controller state in status icons #1884

Merged
merged 1 commit into from
Jan 5, 2025
Merged

Conversation

jmlich
Copy link
Contributor

@jmlich jmlich commented Oct 8, 2023

I would like to know if the alarm is active on Watch Face Digital screen.

I made an prototype with InfiniSim and it looks that it works as expected:

screenshot-alarm-indicator InfiniSim_2024-12-22_141530

However, I have only sealed version of PineTime and this is my first attempt to make something for embedded device. I will be happy if someone can check if this actually works on real device. I am looking forward for your feedback.

@github-actions
Copy link

github-actions bot commented Oct 8, 2023

Build size and comparison to main:

Section Size Difference
text 372944B 128B
data 948B 0B
bss 22536B 0B

Copy link
Contributor

@minacode minacode left a comment

Choose a reason for hiding this comment

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

I like this feature! :)

You should be very safe to test this on a sealed device (speaking from experience).
If something goes wrong, which shouldn't happen, you can always:

  • reboot to get back to your last firmware (don't verify your debug flash of course)
  • hold the button pressed until the color changed twice during boot to get to the fallback firmware and update your watch from there.

src/displayapp/widgets/StatusIcons.cpp Outdated Show resolved Hide resolved
@jmlich
Copy link
Contributor Author

jmlich commented Oct 21, 2023

I have also tested the feature on hardware.

@LinuxinaBit
Copy link

I like this, but I think the icon has a little too much filled space and the simple circle isn’t as defined as a more standard alarm icon:
image
Of course these might not work with a resolution that small, but I’m sure something similar could…

@jmlich
Copy link
Contributor Author

jmlich commented Nov 9, 2023

I wasn't trying to replace clock icon. I was trying to provide an information about alarm status with existing icon.

I agree that rework of icons and design overall may improve look and feel, but this wasn't my intention in this pull request.

@LinuxinaBit
Copy link

LinuxinaBit commented Nov 9, 2023 via email

@vkareh
Copy link
Contributor

vkareh commented Feb 12, 2024

This should be rebased against main and updated to reflect the new icon from #1996

@rnwgnr
Copy link

rnwgnr commented Aug 15, 2024

@jmlich I'd like to give this change a try, would you mind rebasing this changeset against the current state of main?

@jmlich jmlich force-pushed the main branch 2 times, most recently from 54190ac to f0e01cb Compare August 24, 2024 08:24
@jmlich
Copy link
Contributor Author

jmlich commented Aug 24, 2024

Rebased to recent main

@jmlich jmlich force-pushed the main branch 5 times, most recently from 15fb52c to e2ff1b2 Compare December 22, 2024 13:05
@jmlich
Copy link
Contributor Author

jmlich commented Dec 22, 2024

Rebased to recent main. The screenshot and screencast in pull request description was updated as well.

@mark9064 mark9064 added the enhancement Enhancement to an existing app/feature label Dec 22, 2024
Copy link
Member

@mark9064 mark9064 left a comment

Choose a reason for hiding this comment

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

Looking great!

src/displayapp/widgets/StatusIcons.cpp Outdated Show resolved Hide resolved
Copy link
Member

@mark9064 mark9064 left a comment

Choose a reason for hiding this comment

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

Code looks great! I still need to test on hardware, other than that all good to go from me :)

@mark9064 mark9064 added this to the 1.16.0 milestone Dec 22, 2024
Copy link
Contributor

@vkareh vkareh 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 been using this on hardware in my daily driver branch for many months

Copy link
Collaborator

@JF002 JF002 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@JF002 JF002 merged commit 3e23ee7 into InfiniTimeOrg:main Jan 5, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement to an existing app/feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants