JohanDegraeve / xdripswift

xdrip for iOS, written in Swift
GNU General Public License v3.0
329 stars 328 forks source link

adding the "contact trick" #505

Closed yurique closed 5 months ago

yurique commented 6 months ago

Fixes #481.


Most of this PR is adding the new settings section to the iOS app. The contact image rendering itself is heavily inspired by the implementation in OpenGlück, but I modified it quite a bit to support more customizations (and removed things related to "episodes", etc).


This is my first time coding for iOS, as well as first time coding in swift or in xcode – please feel free to nitpick all the things :)


Does it work? - Yes! I kept the original G7 complication on my watch together with the "contact trick" for a while - feels like half of the time I was looking, the G7 complication was either not showing anything (--) or showing the outdated reading (the previous one), while the contact picture updates almost immediately and has been up-to-date all the time.


image IMG_3219

callms commented 6 months ago

Hello @yurique! OpenGlück maintainer here, nice to see you have been able to port this to xdripswift! 🥳 From the screen it looks great, well done! 🎉

yurique commented 6 months ago

@callms hey hey! Btw, a bit off-topic: I tried running the OpenGlück-server but never managed to get it to work (it doesn't seem to be creating the initial user and keeps sending me back to the create-user page) (I was trying to launch it inside k8s, so it could be a misconfiguration on my side - but I couldn't figure it out)

callms commented 6 months ago

@callms hey hey! Btw, a bit off-topic: I tried running the OpenGlück-server but never managed to get it to work (it doesn't seem to be creating the initial user and keeps sending me back to the create-user page) (I was trying to launch it inside k8s, so it could be a misconfiguration on my side - but I couldn't figure it out)

Thanks for pointing it out, I think it was somehow broken when I added multi-user. I think it should be fixed in https://github.com/open-gluck/opengluck-server/pull/28

bjornoleh commented 6 months ago

What a cool addition! Now I just need to get it to work :-) Any special requirements for the contact that is used? I am not seeing any picture being added to this contact, so still just seeing the x. And should it work both as NS follower and Dexcom master (receiver)? Thanks!

yurique commented 6 months ago

@bjornoleh I don't know enough about the internals of xdrip, but the contact trick is just subscribing to receive updates whenever there is a new BG reading available and then renders it into the contact picture - I'm assuming it doesn't matter if it's in follower or master mode (though I could be wrong about this).

I have only tested it on my phone, and in the simulator - and for me it "just works".

Is there a chance you could get the logs from the app? (if you're running it from xcode - the logs should show up there, in the debugger - otherwise you can send the logs from within the app - "Send Issue Report"). In case of errors it should log messages starting with in updateContact, ... (for example, in updateContact, failed to update the contact).

bjornoleh commented 6 months ago

Thanks for your reply. I am trying to get this to work in a S3, which only gets to watchOS 8. Any limitations here?

Regarding the contact added, can any contact (with a fake email address?) be used, or is a specific one like in your screenshot needed?

callms commented 6 months ago

(Original OG implementor here.) It should work with any watchOS version compatible with xdripswift, just like when using the calendar trick. watchOS doesn't have limitations on Apple widgets as far as refresh goes.

bjornoleh commented 6 months ago

(Original OG implementor here.) It should work with any watchOS version compatible with xdripswift, just like when using the calendar trick. watchOS doesn't have limitations on Apple widgets as far as refresh goes.

Thanks!

Can any contact be used, or only a specific email (there is a specific one menton the OG readme)? I also tested adding a picture to the contact to see if that was displayed, and it was (but rather small on the 38 mm S3). The picture was not updated though.

yurique commented 6 months ago

Any contact should work - it stores the ID of the contact you select in the settings, and then finds the contact by that ID and updates it.

bjornoleh commented 6 months ago

Ok, thanks. I did find in updateContact, failed to update the contact every five minutes in the logs, but I guess that doesn’t tell us much

yurique commented 6 months ago

@bjornoleh that's a very good start! There is an error, but its message is not logged. I just pushed an update that should log a bit more details about why it's failing.

callms commented 6 months ago

@bjornoleh by any chance, can you double-check if permissions allow xdripswift to update contacts?

bjornoleh commented 6 months ago

Thank @callms for looking into this! I rebuilt with the additional logging now. Then I verified that the contacts permission is given: Disabled, noted the warning in Xdrip when toggling on the contacts trick, then gave permission and started testing again.

I get Code 207: Container is read-only

Did fail to save (1740 µs): Error Domain=CNErrorDomain Code=207 "Container is read-only" UserInfo={NSUnderlyingError=0x280866280 {Error Domain=ABAddressBookErrorDomain Code=0 "(null)" UserInfo={PolicyRejectionReason=SourceNotWritable}}, CNInvalidRecords=(
    "<CNMutableContact: 0x117f05db0: identifier=AFB3B46E-876E-481F-A322-38D4489EA01A:ABPerson, givenName=(not fetched), familyName=(not fetched), organizationName=(not fetched), phoneNumbers=(not fetched), emailAddresses=(not fetched), postalAddresses=(not fetched)>"
), NSLocalizedFailureReason=The save request failed because it attempted to modify records in a container that is read-only., NSLocalizedDescription=Container is read-only}
in updateContact, failed to update the contact - Code 207: Container is read-only
yurique commented 6 months ago

It looks like the contact is read-only. I don't really know, why. Is it a regular contact on your phone? Is there anything "special" about it?

bjornoleh commented 6 months ago

It looks like the contact is read-only. I don't really know, why. Is it a regular contact on your phone? Is there anything "special" about it?

I don't think there is anything special about it. It is given a name (xd) and has the same email as in your readme (bg@xdrip.nowhere) I have tried various other contacts too.

The picture was displayed on the S3 watch when I set a contact picture manually.

The phone is a kids phone with some Screen Time restrictions. Not sure if any of that could interfere?

The phone also shared its iCloud with another phone used as a follower phone by other caregivers, so the account had two associated phone numbers. I thought this could matter, and signed the outer phone out of iCloud. I rebooted phone and watch afterwards for good measure. But no difference still.

callms commented 6 months ago

@bjornoleh by any chance, does it work when you update the contact from your phone? Does it work when you create a new contact?

bjornoleh commented 6 months ago

@bjornoleh by any chance, does it work when you update the contact from your phone? Does it work when you create a new contact?

Yes, I can change the contact picture manually, it updates on the watch

yurique commented 6 months ago

It seems like some kind of restriction on the phone - Screen Time, iCloud, something else? Unfortunately, I know next to nothing about all of this :(

paulplant commented 6 months ago

Once 5.2.0 is released, we'll look at getting this PR merged in @yurique , @callms . Nice work!

We'd need to try and limit the settings options if possible. Can the font size/type and also the dark mode option be removed? I mean can we use a fixed/system font and have SwiftUI scale it to the size needed to fit the circularAccessory widget?

paulplant commented 6 months ago

Another thought... although technically the name "Contact Trick" would work for devs because it accurately describes what it is, I don't think it would work well for public release. In fact translating this into other languages would probably create confusion that would need further explanation. The fact that it's a trick/workaround/exploit shouldn't be worded into the description itself.

Do you guys have any other ideas that could be used? For example, "Calendar events" works well because it is exactly what it is, an event in the calendar. But I'm struggling to think of a better name for this that doesn't use the work "trick".

If this is also only for Watch complications, then maybe this needs to be clearer too. Maybe something like "Watch contact complication" would work but it's maybe a bit long.

m-a-v commented 6 months ago

What about Contact Sync. At least in the docs there will be needed some kind of explanation. But agree, that trick is probably not the best term...

yurique commented 6 months ago

Can the font size/type and also the dark mode option be removed? I mean can we use a fixed/system font and have SwiftUI scale it to the size needed to fit the circularAccessory widget?

Definitely! The renderer is already trying to adjust the font size to fit it into the area.

Dark mode - this was an attempt to make it work on both dark and brighter watch backgrounds. But it looks like the watch is able to be somewhat smart about it. I tried playing with it just now: when I'm changing the color "scheme" - the watch is replacing the white text color on the contact pictures with the color I select. I've never used a non-black background, though.

Font name - I was trying to find a font that looks the most readable in a small complication, but system default looks decent as well.

callms commented 6 months ago

What about Contact Sync

I personally like it, “Contact Sync” and “Calendar Sync” would make it even to understand for non-developers if we were to talk about the two in the same sentence.

dnzxy commented 6 months ago

What about Contact Sync

I personally like it, “Contact Sync” and “Calendar Sync” would make it even to understand for non-developers if we were to talk about the two in the same sentence.

Chiming in as a Loop and iAPS contributor. I wouldn't call it Calendar sync for the fact that there's the calendar events "trick" (writing a new calendar entry and updating time upon every new CGM reading) in both LoopFollow and iAPS, as well as xDrip. This trick/workaround (calendar events) got broken with the release of watchOS 10, as you folks probably are aware. It's a limitation posed by Apple, and there have been a multitude of bug reports done for it.

I really like Contact Sync or may Contact Avatar Sync.

callms commented 6 months ago

This trick/workaround (calendar events) got broken with the release of watchOS 10, as you folks probably are aware

Thanks, I wasn't aware. I tried playing with the idea of a Contact photo as I wanted to use a circular complication for my BG.

yurique commented 6 months ago

@dnzxy you might want to chime in in iAPS as well :) - https://github.com/Artificial-Pancreas/iAPS/pull/595#issuecomment-2010935013

dnzxy commented 6 months ago

@dnzxy you might want to chime in in iAPS as well :) - Artificial-Pancreas/iAPS#595 (comment)

@yurique, the relationship between JBM and an extensive community of contributors/developers has experienced its fair share of challenges. It's noteworthy that a large portion contributors have chosen to move on, and others and I have found themselves unable to participate in the official Discord channel. Please feel free to connect with me directly if you're interested in more details. Should you be considering involvement with that project (your PR shows so), it might be worthwhile to carefully evaluate the situation.

bjornoleh commented 6 months ago

@yurique , I did some testing now in Xcode simulator. My previous builds are working fine there, the contacts are updated as expected.

For some reason, the contacts app is not installed on the simulator Watch S9, so I can't see it on the watch. But the contacts are updated correctly.

Question: what is the "Range Indicator"?

Also tested the iAPS version, and I like the option of displaying the glucose date, that is pretty crucial in this context. Also liked the possibility of configuring several contacts.

yurique commented 6 months ago

@bjornoleh range indicator (not the greatest label) - it's the little colored bar (green/yellow/red for normal, not-urgent, urgent)

image

Also tested the iAPS version, and I like the option of displaying the glucose date, that is pretty crucial in this context. Also liked the possibility of configuring several contacts.

Multiple contacts is nice indeed - but the way settings are organized in xdripswift (in the code) makes adding new options hard and I don't even know how one could add an "array" of things (to store a list of contacts) (though I might just not know how it is supposed to be done). And without multiple contacts there isn't really any room to put additional data.

bjornoleh commented 6 months ago

Thanks, got it!

Yes I understand about the limitations for xdrip settings, I messed a little with this once, and seem to remember it was complicated.

paulplant commented 5 months ago

@callms , @yurique , I'm going to start getting this pulled in over the next days.

Did you guys want to make any updates first?

I know in iAPS it makes sense to have multiple contacts to show different complications with fields, but I'd really like this to be as simple as possible here if that's OK... so use a single design and just show the BG value (+ trend if you like), but keep it very simple and remove as many extra settings as possible.

paulplant commented 5 months ago

Regarding the name, I was thinking a bit more about this and although we use "Calendar Event" because it is a generic title as the events can be used in many different places (Watch complications, iPhone homescreen/widgets, CarPlay) and even shared to other users via shared calendar, in this case, the "Contact Trick" is only used for the Apple Watch complications.

Can you guys confirm that this is true?

If so, then we could maybe be more specific in the name. Something like "Watch Contact"? (Or is it too confusing)?

yurique commented 5 months ago

@paulplant sounds good to me! I'll try to find time to resolve the conflicts later today and will remove some unnecessary settings.

But definitely - please feel free to adjust it as much as you find necessary :)

yurique commented 5 months ago

the "Contact Trick" is only used for the Apple Watch complications.

I have only used it on the watch. One can put a contact widget on the phone as well (or even on the desktop in macOS) but I'm not sure if it's useful.

yurique commented 5 months ago

Sorry, didn't get to this yesterday :/. Now aiming for today :)

yurique commented 5 months ago

Conflicts resolved, now going to remove some of the font settings.

paulplant commented 5 months ago

Great... let me know when ready and I'll probably cherry pick your commits into my staging branch and we can finish working on it there.

dnzxy commented 5 months ago

@yurique @paulplant is it worth verifying fastlane build-ability now, or wait for the RC in your staging branch, Paul? Just curious when to best set aside a bit of time to test.

yurique commented 5 months ago

@dnzxy I just pushed a commit removing some of the settings. It builds in xcode, but I haven't tested it yet. There is one thing that I'm not sure about - in iAPS I used the standard Contact Selector "component", and here I'm just building a drop-down (I didn't know better when I implemented it). I think it's worth using the standard component as well but I'm not sure how to integrate it into the way settings views are built.

dnzxy commented 5 months ago

I‘ll spin up a build tonight and see how it goes. Pretty confident that it should build fine. Doesn’t look like this should break the fastlane build 😊🤞

paulplant commented 5 months ago

@yurique @paulplant is it worth verifying fastlane build-ability now, or wait for the RC in your staging branch, Paul? Just curious when to best set aside a bit of time to test.

I would hang off for a bit... when I saw the changes you did 2 weeks ago I know understand what causes things to break in the fastlane build and there shouldn't be anything like that yet.

Last time we were up against the 90 day expiry so had to really push to get 5.1.0 out and then we were rushing hot-fixes out for a few days.

This time we've got lots of time so it's better to wait until everything hits develop in the next week or so and then we can calmly test fastlane before release. There's no rush this time and we'll make sure it's all ready before release.

paulplant commented 5 months ago

I added a staging branch: https://github.com/JohanDegraeve/xdripswift/commits/staging-5.2/

yurique commented 5 months ago

looks like I missed to stage one of the changes when when committing (removing the reference to FontTypeWeight in project.pbxproj), I fixed it and merged the upstream changes (last two commits here: https://github.com/yurique/xdripswift/commits/feature/contact-trick/)

paulplant commented 5 months ago

Please open a PR with them into this branch

yurique commented 5 months ago

@paulplant here it is: https://github.com/JohanDegraeve/xdripswift/pull/527

dnzxy commented 5 months ago

FWIW: Staging 5.2 (with all things merged) is building fine 🚀 see successful run at https://github.com/dnzxy/xdripswift/actions/runs/8651372734/job/23721981359

paulplant commented 5 months ago

They'll be a lot more commits going to that branch over the next week or two so I'll tag you again when it's worth re-checking. Thanks!!

dnzxy commented 5 months ago

Anytime! 🤝 I just wanted to make sure that this feature is building without issue (as expected; now verified).

paulplant commented 5 months ago

@yurique , @callms , @m-a-v , I've been using it for a short while and thinking about it and I would like to propose "Contact Image" as the name... as this is exactly what it is/does without any confusion or making it sound like a workaround.

Does this sound ok?

yurique commented 5 months ago

Sounds perfectly fine! (I don't really have any preferences regarding the name :) )