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

Persistent High Alert threshold fix #3729

Closed

Conversation

Navid200
Copy link
Collaborator

@Navid200 Navid200 commented Oct 22, 2024

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

Screenshot_20241022-170836 Screenshot_20241022-170907 Screenshot_20241022-170934

@Navid200 Navid200 marked this pull request as draft October 23, 2024 13:02
@Navid200 Navid200 marked this pull request as ready for review October 24, 2024 04:14
@Navid200
Copy link
Collaborator Author

Navid200 commented Oct 24, 2024

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.

PHA_Range.mp4

@Navid200
Copy link
Collaborator Author

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

@jamorham
Copy link
Collaborator

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
Copy link
Collaborator Author

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 Navid200 marked this pull request as draft October 25, 2024 11:34
@Navid200
Copy link
Collaborator Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants