-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Make X axis scaling consistent #3464
Conversation
…eriod is not always locked; and if it is locked, center the display on the current bg value, not the last prediction.
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. Thanks so much for the fast implementation. Thanks for your help. Thanks a lot. |
Wait! I missed the scroll part. So, one can scroll and see the prediction? Are you serious? That is awesome! |
@trallen Thanks for the prod to have a look at this. Nothing you propose here would have any negative impact on me. |
Thanks, I had not thought of this. This is the "before" screen -- no predictive info is loaded: In the current master, this is what the screen looks like when Predictive Simulations are turned on (note the change in the X axis): This PR changes the display behaviour when predictions are available: 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 |
This looks good to me. |
@trallen I'm sorry I had overlooked your previous contributions. |
Happy to help with such an awesome project! :-) |
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. |
I've been running it since I submitted the PR, and have not seen any issues yet. Thanks for taking a look! |
@trallen Thanks |
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.)