search

NightscoutFoundation / xDrip

Nightscout version of xDrip+
https://jamorham.github.io/#xdrip-plus
GNU General Public License v3.0
1.41k stars 1.15k forks source link

Persistent High Alert threshold fix #3729

Closed Navid200 closed 4 weeks ago

Navid200 commented 1 month ago

I forgot to include verification for the threshold range. This PR adds that.

We wait for the threshold to be changed to trigger a listener. Then, we fix it if the value is out of range. This means that in the meantime, the alert could be using an incorrect threshold.
Also, if the app crashes just after a new threshold is added but before verification, it will never be corrected.

To avoid those, I have added a second preference. The alert only uses the second setting. This effectively isolates the alert from transitionary changes the UI setting may experience.

There is no before/after because there is no change to how the user interface looks. The new setting is invisible.

I wish I knew a way to accomplish these simple objectives with a PR much smaller than this.
I am open to suggestions.

I have intentionally left everything in English. After this is used and tested and verified, I will open a PR and add all the new text to strings so that they can be translated.

Thanks

Navid200 commented 1 month ago

Why a notification?
A toast should have sufficed. However, my tests show that the toast does not come up immediately after a new threshold is entered. It may take 5-10 seconds. Therefore, the user may not realize that the toast is related to the threshold they just changed. The (silent) notification (and log) are added to remedy that.

Navid200 commented 1 month ago

The toast, notification and log are shown in the following images.

Screenshot_20241022-170836 Screenshot_20241022-170907 Screenshot_20241022-170934

Navid200 commented 4 weeks ago

This clip shows that it takes 10 seconds for the toast to come up with the data source set to Nightscout follower.
With the data source set to fake data source, it never comes up until the next reading or when the user taps on settings.

https://github.com/user-attachments/assets/33088e6d-1103-4d99-b122-35f5f751a92d

Navid200 commented 4 weeks ago

I have tested this on Android 8, 9 and 11. It is ready for review.

jamorham commented 4 weeks ago

I don't really know what you're doing here. Why don't you just return false from the onPreferenceChange listener if you want to reject the new value?

https://developer.android.com/reference/android/preference/Preference.OnPreferenceChangeListener

Navid200 commented 4 weeks ago

Thanks for this. Apparently, I misunderstood that this was possible. I thought that the listener informed us after the preference changed not before. This should remove the need for the additional setting. I will fix that.

Thanks

Navid200 commented 4 weeks ago

OMG! It just takes a few lines to do all of this when doing it the right way, meaning when you know Java! I need to take a class.

Thanks so much for catching this. I will open a PR in about 9 hours after all tests.