Refactor override management

[avouspierre]

The PR includes a large refactoring of the swift part of override/profile functions :

• Override is stored in override core data, including history
• Override preset is stored in overridepreset core data
• Add the display of the override in main graph
• allows to select a override preset (or a temp target) in AW
• allows to cancel a current override preset or specific override (idem for temp target) in AW
• add a color profil for override profil in AW
• Fix some minor code in AW
• add the upload of override as a exercice in Nightscout - Fix [feature request] Display profiles/overrides in Nightscout  #145
• improve the management of indefinate override / stop of indefinate override
• modify the code to respect the Ivan’s patterns of the app :

• Use of swiftInject (dependency injection) with the use of protocol class in the code
• Use of MVP principes, in particular not use of direct coredata in view class
• Use of a proxy model class between coredata and the app to manage changes of core data
• Use of the pattern of observe to refresh data/view/uploads

• add a core data unit tests allowing to add tests for coredata with a in-memory datastore for tests.
• test for overrideStorage available

This PR do NOT change the logic with oref and the interface of override informations in oref. This PR do NOT require a update of trio-oref code.

This PR is a part of #219 (split required by admin for review).

override display	override in NS

[avouspierre]

This version is now updated with the dev version including the new edit of override. Could be review

[mountrcg]

What happens if someone enacts at temp target on top of an override with a target and an insulin change? It used to be that this temp would override the override target (pun intended) and change sens to whatever the settings dictate.

What should be the expected behaviour in your view?

• prevent TT while override
• Auto cancel override when doing TT
• Only replace override settings that the TT changes
• Disregatd any sens change a tt would do (which could be done changing the overrides target now)
• …
  ??

[avouspierre]

Very good question @mountrcg because there's nothing to stop you running both at the same time. The logic of override is different (change ISF before sending to oref) to temp target (change the BG Target). So the conflict is probably (not sure) if you change the BG Target in override...I don't go in the oref code to analyse the result.

Perhaps, we could manage this case in a new PR after merging this ?

[dsnallfot]

@avouspierre I just noticed a (small) thing that I messed up in my previous PR 235. I always use dark mode in the app, and didn't look at this particular thing in light mode. The edit sheet that I thought looked good in dark mode doesn't look as good in light mode due to the .padding() when opening the sheet in OverrideProfilesRootView.

I don't want to make a new PR with the fix for this until after this PR is merged, but it would be great if you could change this (just delete the .padding() line) in this open PR ?

            .sheet(isPresented: $isEditSheetPresented) {
                editPresetPopover
                    .padding()

Dark mode edit sheet ok	Light mode edit sheet ugly borders

And @mountrcg, the same light mode ugliness as above is also present in the edit sheet for temp targets in AddTempTargetRootView after PR 236 and needs the same delete .padding()

(On a related note: I'm looking into your PR 241 but didn't immediately see any quick fixes to the outstanding questions - will get back to yours as soon as I have some ideas)

[polscm32]

Before this pull request can be merged, some changes need to be made in my opinion:

1. The current Core Data stack needs to be expanded to include methods like persistent history tracking, to merge changes from background contexts into the view context.
2. For this, background contexts need to be introduced. For example, the save function in the stack currently uses the view context. Everything in the OverrideStorage does so as well. However, we should try to keep the main thread as free as possible. The UI is already buggy in the app and the existing Core Data Setup is pretty bad. We should first improve the basic setup before adding more features in my opinion.
3. Error handling: We should avoid using try?. If it fails, it is currently not handled at all. We should definitely handle it. Also, we should not use fatalError in a shipping application, as it causes the app to crash if a Core Data save operation fails.
4. It would be better to create an entity for the overrides with a flag (e.g., "isPreset") to distinguish between presets and overrides.
5. The Core Data code should be thread-safe, meaning the app should build with the Core Data concurrency debug flag enabled. Otherwise, it can lead to incorrect access to the managed object context and the potential for data races.
6. We should be more cautious with performAndWait, as it always blocks the calling thread and can therefore cause performance issues.

[dnzxy]

First of all, thank you for tackling this important feature. Your introduction of the NS upload is crucial for caregivers and family/partners of Trio users, and it is appreciated.

I conducted a code review and performed a quick test in a simulator. While the initial implementation looks promising, there have been reports of some glitches and issues on Discord. Unfortunately, I couldn't test the upload part as my second NS instance is currently down.

That being said, we need to have a broader discussion about how significant backend rewrites, such as this, should be approached, given the current state of our codebase. As you know, Trio (and iAPS) are quite leaky, non-thread-safe, and performance-hungry in their current form. I concur with @polscm32 that before diving into a rewrite or enhancement that heavily relies on CoreData, we need to address several foundational issues.

I'm not sure how we can best tackle this in a timely manner before the release of Trio 1.0.0. My instinct is that we should aim to change as little as possible around Overrides for now and ensure they are uploaded to Nightscout using the Exercise data type. Larger rewrites can be addressed once we've made more of the backend improvements, particularly making it thread-safe.

Thank you again for your efforts on this. Your work is valuable, and I don't want to make it seem like I am diminishing your effort or rejecting your contributions. I'm certain, with some additional adjustments, it will significantly enhance our project. Let's continue the discussion to find the best path forward.

Feel free to add your own thoughts and suggestions.

[avouspierre]

@polscm32 @dnzxy I analysed quickly your implementation of CoreData and it is a amazing job... more important, I think, than the current PR. So I tried to answer to your proposal :

1. The current Core Data stack needs to be expanded to include methods like persistent history tracking, to merge changes from background contexts into the view context.

The core Data is used in different part of the code of Trio and this proposal seems to be a specific PR to create the structure to manage the core data. One subject is the purge of old data but could be pushed in a specific PR.

2. For this, background contexts need to be introduced. For example, the save function in the stack currently uses the view context. Everything in the OverrideStorage does so as well. However, we should try to keep the main thread as free as possible. The UI is already buggy in the app and the existing Core Data Setup is pretty bad. We should first improve the basic setup before adding more features in my opinion.

My objective is only to have a correct behavior of override and a maintenable code. That's why there is now only one function to save overrides in core Data... A new PR seems adequate to modify all save functions in Trio as background.

3. Error handling: We should avoid using try?. If it fails, it is currently not handled at all. We should definitely handle it. Also, we should not use fatalError in a shipping application, as it causes the app to crash if a Core Data save operation fails.

Understood. I modify the code with a do/catch

4. It would be better to create an entity for the overrides with a flag (e.g., "isPreset") to distinguish between presets and overrides.

I could but I'm not sure the interest of the attribute. The override table/entity stores all historical overrides without any linked with the preset entity. Probably the more interesting thing to do will be to store the id of the preset used for the override but should be in a more global remastering data in core data.

5. The Core Data code should be thread-safe, meaning the app should build with the Core Data concurrency debug flag enabled. Otherwise, it can lead to incorrect access to the managed object context and the potential for data races.

Here too, a new PR because the flag changes all core data access in the code of Trio.

6. We should be more cautious with performAndWait, as it always blocks the calling thread and can therefore cause performance issues.

OK but the interest is the fact thet performandwait is safe-thread and without to manage publishers strategy. 😌.

Thank you again for your efforts on this. Your work is valuable, and I don't want to make it seem like I am diminishing your effort or rejecting your contributions. I'm certain, with some additional adjustments, it will significantly enhance our project. Let's continue the discussion to find the best path forward.

I don't have any problem if you think the PR is not appropriate. I learn lot of swift techniques with your proposals 🙏. But I push two arguments 😊😊to review this PR : First, you asked me to split in more simple PR and the previous propositions are largely more than the objective of the PR. Second this PR cleaned the code of override in regard of the maintenance - one class manages the core data interface ! - and it brings some improvements (NS uploads and display the override in the graph in particular).

A last thing (but not a argument) : I used this PR in IRL for 2 months without issue.

I will integrate your different updates in the next commit !

[aug0211]

I think scope control is our biggest challenge here. If (and only if!) getting this merged is going to take more than 1 week, maybe we pair this into a couple PRs, focused on:

1. Nightscout and main graph visualizations (blocking issues: safety consideration)
2. Deletion and handling of indefinite overrides (I'm unclear whether this is a safety consideration or optimization)
3. Watch capabilities (feature)
4. Refactor for Core data storage (optimization)

I list in this prioritized order based on what I think is most urgent, with item 1 being high criticality and items 3 and 4 being things we absolutely should do, but I would not personally recommend blocking a release for. Item 2 I am not 100% clear on.

Again, I would only break this out if we think this PR is large enough that it'll take a week or more to work through and merge - so that we can get the top priority items merged in 1-2 days each (in serial, not parallel), hopefully.

[dnzxy]

I agree that the smaller the PR scope, the easier to work on it as a dev and the easier to test it for testers. I also think the NS upload of overrides (as they are) is the top priority, the other features are reaching further.

[avouspierre]

I updated with some proposals from @dnzxy and @polscm32 (minor improvements)

Thanks to @aug0211 to take the action to split in different PRs because it's a little bit frustrated (for me) to do this in regard of the work done (but I understand the logic and the difficulty to review by others).

[aug0211]

Sent ya a DM on Discord. It's out of reach for me to be a lead dev on any of the components (I've tried on my own the past week with no success 😂) but am motivated to help here however I can.

[bjornoleh]

The complexity of this PR makes a review quite far beyond my skillset, so I will have to give a pass on actual code review here. I could build and check that it works, but I am sure that it does so already, so there is limited value in that.

From what I read, a bigger rewrite of core data is planned after v1.0, or already in the works. Would it be possible to have working overrides and upload to NS with little or no changes to core data? The original idea for v1.0 was to make minimal changes, then do more involved changes later. Hopefully that would still be possible :-) I can't really participate much in the detailed technical discussions about this, I just wanted to ask this as a more general question (as others already have asked, sorry for repeating it :-) )

[aug0211]

I made some sort of mistake here. I'm sorry @avouspierre, this is in your fork and on your branch.

I was working on creating a branch in my own repo, off of @avouspierre's dev+overrides and dev+shortcuts branches, to do some work with getting overrides working in Nightscout.

I created a new branch "overrides" in my repo, intending to pull in first Pierre's shortcuts branch and then his overrides branch over top of this. Somehow I messed this up, and made a mess of Pierre's dev+overrides branch in his fork (again, I'm sorry Pierre!).

It almost seems as if I merged things into Pierre's branch by accident instead of my fresh branch in my repo, but I suspect something tricker like Github upstreams/remotes pointing going to the wrong places on my end (?), and then creating this spiderweb.

I did not expect (or intend) to see the following commits in Pierre's branch:
0994ae3 - was able to revert this one, but then stopped because I don't understand this situation and do not want to make it worse
1b41386 - unable to revert this one, and it seems the weirdest to me of the lot
848b5dc
0e7e8d4

[flyingpie101]

@avouspierre Would you be able to do a PR that just updates NS with Override info as a NS Exercise ?

[Sjoerd-Bo3]

@avouspierre Would you be able to do a PR that just updates NS with Override info as a NS Exercise ?

Is it not wise to wait for the core data rework and revisit this PR as a whole again? Then we have a good structure and we can get this setup the right way? Would hate to see @avouspierre his work get halted again by something like this.

[flyingpie101]

Honestly, I have no idea if it's wise or not ;)
As a caregiver, being able to see if my daughter has put on an Override, or forgot to turn one off, has been super helpful.

And, so after reading this Issue log, it seemed displaying an Override in NS using the an NS Exercise function, which seemed important to a few of us ;), looked like it got tied up with a core date re-work, when I am not sure it should have.

Since it has been a little more than a month, and rc1 is happening, I thought I would refresh the topic.

[Sjoerd-Bo3]

I feel you, but this PR has core data tangled into it. The PR (and shortcuts) also have core data rewrites in them. Making it a mess in combination with the whole core data rework. When 0.2 is released and core data is in test swing, it would be good to stream this in its wake. But if @avouspierre wants to fix this one. Ofc take a swing! But I’m just worrisome about lost efforts and this getting a dragging topic.. Just cautious for that, understand the need a lot!

[flyingpie101]

Absolutely. I have no idea of the effort. I wasn't sure who did the version in iAPS, so I kinda thought one of the experts here, might be able to leverage that fairly quickly.