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 X axis scaling consistent #3464

Merged

Conversation

trallen
Copy link
Contributor

@trallen trallen commented May 15, 2024

At present, when predictions are added to the BG line chart, the chart changes its scale.

For example, when displaying 3 hours of data, and a prediction event is added (adding a bolus, for example), the chart will now display ~6 hours (depending on the decay of the insulin) in the same view. This re-scales the chart, which can be confusing if you have locked the time period.

This commit changes this behavior, but only when "Locked Time Period Always Used" is on.

If "Locked Time Period Always Used" is set, the existing X axis scale will remain unchanged when predictions are made, instead adding the prediction offscreen to the right of the current BG value. (That is, you will see the start of the prediction, but will have to scroll right to see the full prediction.)

…eriod is not always locked; and if it is locked, center the display on the current bg value, not the last prediction.
@Navid200
Copy link
Collaborator

Thanks for the PR.

Everything I state is my opinion. You should not take it as an official xDrip statement. I am not making decisions about what is approved or not. So, if I suggest that you do something, it is only my opinion. :o)

Because this affects what is seen on the screen, it may help with the review if you show before and after screenshots. I know that may be a lot of work. But, if I was the one who had to decide to approve or reject this, I would need to see it.

One of the major concerns is how something changes xDrip. Just because we improve something, it may not be enough to go for it. If every user has got used to it the way it is, changing it may not be a very good idea. Again, that is why it will be useful to be able to see it to decide how big a change is it.

For example, I am curious exactly what it looks like and how much of the curve in the future it still shows.
If nothing is shown in the future, I wonder if we can shot by one hour to shows one hour less in the past so that we can show one hour more in the future when this kicks in. Just wondering.

Thanks so much for the fast implementation. Thanks for your help. Thanks a lot.

@Navid200
Copy link
Collaborator

Wait! I missed the scroll part. So, one can scroll and see the prediction? Are you serious? That is awesome!

@jonprogers
Copy link

@trallen Thanks for the prod to have a look at this. Nothing you propose here would have any negative impact on me.

@trallen
Copy link
Contributor Author

trallen commented May 16, 2024

Because this affects what is seen on the screen, it may help with the review if you show before and after screenshots. I know that may be a lot of work. But, if I was the one who had to decide to approve or reject this, I would need to see it.

Thanks, I had not thought of this.

This is the "before" screen -- no predictive info is loaded:

Screenshot_20240516-125856_xDrip+

In the current master, this is what the screen looks like when Predictive Simulations are turned on (note the change in the X axis):

Screenshot_20240516-130111_xDrip+

This PR changes the display behaviour when predictions are available:

Screenshot_20240516-130748_xDrip+

In particular, the slope is maintained as-is. (Well, technically, it's not -- the Y axis does change a little, since the prediction drops below 2 at some point, so instead of a Y range of ~2 - ~12, both screenshots with predictive simulations have a Y range of 0 - ~12. This is outside the scope of this PR, and I'll be looking into this behaviour in more detail later.)

Lastly, to show how predictions are reached via scrolling, I'm trying a small video; hopefully it'll work:

output.mp4

@Navid200
Copy link
Collaborator

This looks good to me.

@Navid200 Navid200 requested a review from jamorham May 16, 2024 13:06
@Navid200
Copy link
Collaborator

@trallen I'm sorry I had overlooked your previous contributions.
Thanks for your service then and now.

@trallen
Copy link
Contributor Author

trallen commented May 18, 2024

@trallen I'm sorry I had overlooked your previous contributions. Thanks for your service then and now.

Happy to help with such an awesome project! :-)

@jamorham
Copy link
Collaborator

This looks like it should work and would be a positive change. @trallen how long have you been running this to confirm the change is robust? The viewport changes can sometimes be tricky.

@trallen
Copy link
Contributor Author

trallen commented May 22, 2024

I've been running it since I submitted the PR, and have not seen any issues yet. Thanks for taking a look!

@jamorham jamorham merged commit 8cc5c66 into NightscoutFoundation:master May 22, 2024
1 check passed
@Navid200
Copy link
Collaborator

@trallen Thanks

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.

4 participants