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.

Sign up for GitHub

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

Jump to bottom

Live activity improvements #310

Merged
merged 4 commits into from
Jul 6, 2024
Merged

Live activity improvements #310

merged 4 commits into from
Jul 6, 2024

Conversation

10nas
Copy link
Contributor

@10nas 10nas commented Jun 15, 2024

  • Add back live previews for the simple live activity view.
  • Refactor LiveActivityAttributes.ContentState.
  • Change LiveActivity structure for readability.
  • Fix cases where text got clipped in the simple live activity. Live previews are really helpful to find these kinds of issues. Please don't delete the live previews again in the future.
  • Add a disclaimer "Live Activity Expired" when the live activity expires on the lock screen. Important improvement that has also been requested on Discord before.
  • Delete unused file in the live activity folder.

marionbarker
marionbarker previously approved these changes Jun 25, 2024
View reviewed changes
Copy link
Contributor

@marionbarker marionbarker left a comment

Choose a reason for hiding this comment

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

Other than the conversionFactor noted, this code built on an SE (no LA available) and an iPhone 15. Testing it without using Trio.

I did not see the clipping that was supposed to be fixed either before or after applying the patch.

@dnzxy
Copy link
Contributor

dnzxy commented Jun 26, 2024

I did not see the clipping that was supposed to be fixed either before or after applying the patch.

If you want to reproduce clipping, simulate a 3 digit glucose with a double arrow up, that should be clipped to something like 10… instead of 100 <arrow> (before this fix).

@Sjoerd-Bo3 Sjoerd-Bo3 deleted the branch nightscout:dev July 2, 2024 14:00
@Sjoerd-Bo3 Sjoerd-Bo3 closed this Jul 2, 2024
@Sjoerd-Bo3 Sjoerd-Bo3 reopened this Jul 2, 2024
@Sjoerd-Bo3 Sjoerd-Bo3 changed the base branch from alpha to dev July 2, 2024 14:03
@Sjoerd-Bo3 Sjoerd-Bo3 dismissed marionbarker’s stale review July 2, 2024 14:03

The base branch was changed.

marionbarker
marionbarker previously approved these changes Jul 3, 2024
View reviewed changes
Copy link
Contributor

@marionbarker marionbarker left a comment

Choose a reason for hiding this comment

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

Approving again after base branch was changed. (Same comments as before about conversion - but that is out of scope of this PR.)

@marionbarker
Copy link
Contributor

As requested. Lock screen graphics when CGM data is stale and when LA has expired.

IMG_6899
IMG_6900

dnzxy
dnzxy previously approved these changes Jul 4, 2024
View reviewed changes
Copy link
Contributor

@dnzxy dnzxy left a comment

Choose a reason for hiding this comment

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

Approval based on code review. LGTM.

@10nas
Copy link
Contributor Author

10nas commented Jul 4, 2024

As requested. Lock screen graphics when CGM data is stale and when LA has expired.

For future reference, the expired screen can be easily tested by changing this line to true: LiveActivityBridge.swift:105 The live activity will then always show the expired message (only the "simple" live activity).

@10nas 10nas dismissed stale reviews from dnzxy and marionbarker via 7587a2e July 4, 2024 19:27
@10nas
Copy link
Contributor Author

10nas commented Jul 4, 2024 •
edited
Loading

IMG_6900

I noticed the strange background here and fixed it. No other changes in my newest commit. Also synced up with latest dev branch while at it. Good to go from my side.

This is the fixed version:

la_expired_fixed

@dnzxy
Copy link
Contributor

dnzxy commented Jul 4, 2024

So this only removes the square background behind the text in the rounded shape, correct, @10nas ?

@10nas
Copy link
Contributor Author

10nas commented Jul 4, 2024

So this only removes the square background behind the text in the rounded shape, correct, @10nas ?

Correct

dnzxy
dnzxy approved these changes Jul 4, 2024
View reviewed changes
Copy link
Contributor

@dnzxy dnzxy left a comment

Choose a reason for hiding this comment

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

Re-approving after minor UI change around expired LAs.
Thanks for the update, nas!

@MikePlante1
Copy link
Contributor

I reproduced the clipping in Dynamic Island with an iPhone 15 simulator. The clipping occurred when the sgv was 3 digits and there was a double arrow. It only occurred when Trio was only using one Dynamic Island slot because the other was being used by a different app.

Interesting to note that none of the examples displayed 2 arrows like they should, but neither did the main screen of Trio. Two arrows were shown for both Simple and Detailed lockscreen widgets and the notifications, though.

Also interesting that the arrow is only red when using one Dynamic Island slot.

Notes Screenshot
dev 1 slot 1 slot dev
dev 2 slot 2 slot dev
PR 1 slot 1 slot with PR
PR 2 slot 2 slot with PR

MikePlante1
MikePlante1 approved these changes Jul 5, 2024
View reviewed changes
Copy link
Contributor

@MikePlante1 MikePlante1 left a comment

Choose a reason for hiding this comment

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

LGTM from testing and light code review.

Sjoerd-Bo3
Sjoerd-Bo3 approved these changes Jul 6, 2024
View reviewed changes
Copy link
Contributor

@Sjoerd-Bo3 Sjoerd-Bo3 left a comment

Choose a reason for hiding this comment

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

Tested in vivo and looked for the expire changes and such. They look and function great. Clipping didnt test (mmol, so never had any issue anyway)

Code review looks great!

@Sjoerd-Bo3
Copy link
Contributor

Merging with 3(+expired from marion) approvals

@Sjoerd-Bo3 Sjoerd-Bo3 merged commit b02b2a9 into nightscout:dev Jul 6, 2024
@MikePlante1 MikePlante1 mentioned this pull request Jul 16, 2024
Merged
mountrcg pushed a commit to mountrcg/Trio that referenced this pull request Aug 1, 2024
@Jon-b-m
New Meal view and bolus views (nightscout#310)
db99798 
* Chart for predictions Bolus view
* A bit cleaner Carbs View with better presets summary
* Back button and view meal entries in Bolus views
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mountrcg mountrcg mountrcg left review comments

@Sjoerd-Bo3 Sjoerd-Bo3 Sjoerd-Bo3 approved these changes

@dnzxy dnzxy dnzxy approved these changes

@MikePlante1 MikePlante1 MikePlante1 approved these changes

@marionbarker marionbarker marionbarker left review comments

@bjornoleh bjornoleh Awaiting requested review from bjornoleh bjornoleh is a code owner

@aug0211 aug0211 Awaiting requested review from aug0211 aug0211 is a code owner

@AndreasStokholm AndreasStokholm Awaiting requested review from AndreasStokholm AndreasStokholm is a code owner

@t1dude t1dude Awaiting requested review from t1dude t1dude is a code owner

Assignees
No one assigned
Labels
None yet
Projects
Trio
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@10nas @dnzxy @marionbarker @MikePlante1 @Sjoerd-Bo3 @mountrcg @bjornoleh