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

Make sensor expiry notifications customizable to the minute #3675

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DmitriyShepelev
Copy link

Implements idea introduced in #3666.

Note that the current way of hard coding notifications at 24, 12, 6, and 2 hours before sensory expiry is preserved if a custom value isn't supplied.

Tested locally using phone and smartwatch.

@DmitriyShepelev
Copy link
Author

@Navid200 Kindly requesting your review. 😊

@Navid200
Copy link
Collaborator

I can only give you my opinion. Only the approval of the repository owner matters as far as what is merged and what is not.

In general, I would never include translations in a PR. It makes your PR look bigger than it really is. And translations should be added by translators in Crowdin, not be developers in a PR.

But, my main concern with this is that I don't think we should have alerts for sensor expiry. We should instead have reminders.
I don't believe anyone should wait for their sensor to expire before they insert the next one. They can if they choose to. But, no one should feel that it is necessary to wait for a sensor to stop before starting the next one.

To expand on that, imagine if something comes up for you and you don't get to start your sensor at the time you would have liked. Let's say by the time you take care of the unexpected, you end up starting your sensor 4 hours later than you would have liked.
There is no reason to start your next sensor 4 hours late just because you started the current one 4 hours late.
Four hours before your current sensor stops, you can start your next so that you go back to your preferred schedule. No one can complain if you do that.
In this scenario, if you have an alert that goes off with respect to when the sensor is going to expire, it will actually go off 4 hours too late.

That's why reminders, multiple of them, instead of one alert will let you plan your day such that you get to start the sensor exactly when you need to.

I agree that if a sensor expires, that requires an alert. If I forgot that my sensor was expiring and I let it expire, I need an alert. That alert (notification) already exists.
You pointed out, in the discussion, that instead of that alert going off exactly when the sensor expires, it should go off may be 10-15 minutes before.
I buy that. I am happy to work on that with you to improve xDrip. But, that is not giving the user the option to define when it should go off. I don't see a reason for that.

In my opinion, we can modify the existing sensor alert (not the existing sensor expiry notification) such that in addition to the timings that it currently has, it would have an additional notification at 15 minutes to expiry and we can set only that one not to be silent.
Would that be acceptable to you?

We will still need to get the lead developer to review and approve. I can not promise approval. But, that change makes sense to me at least.
That's why I am asking your opinion.

@wirbel-at-vdr-portal
Copy link

"To expand on that, imagine if something comes up for you and you don't get to start your sensor at the time you would have liked. Let's say by the time you take care of the unexpected, you end up starting your sensor 4 hours later than you would have liked."

That's why i would like to see that the 12hr/24hr sensor expiry time would be more clearly understandable for non-english users. They're always confused by the AM versus PM difference (which one of these is before or after lunch..), instead of 24hrs as they use.

That would add up to an 12hr+ issue in case of misunderstanding.

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.

3 participants