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

Do not assign a y position to notes #3620

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

Conversation

Navid200
Copy link
Collaborator

@Navid200 Navid200 commented Aug 12, 2024

Fixes: #1299


Recreate the problem
Install the latest Nightly (https://github.com/NightscoutFoundation/xDrip/releases/tag/2024.08.07).
Disable predictive simulations.
Set the BG units to mmol/L.
Enter an insulin treatment.
Tap on it and add a note. You should see something like this.
Screenshot_20240812-175734

Change the BG unit setting to mg/dL. Now, you will see something like this.
Screenshot_20240812-180000

You can see the y axis range has been expanded to very low values. Now, delete the treatment. You will see the scale going back to normal as shown below.
Screenshot_20240812-180204

Enter a treatment again. Add a note to it. You should see something like this:
Screenshot_20240812-180331
Change the BG unit to mmol/L. You will see this:
Screenshot_20240812-180522
You can see that the y axis range is now unreasonably extended on the high side.


After this PR
The note will not have a y position value applied to them.
Let's repeat the second test. This is what you will see after entering a treatment and adding a note to it.
Screenshot_20240812-181019
Now, if you change the BG unit, you will see this.
Screenshot_20240812-181152
You can see that the y axis range remains reasonable.

@Navid200
Copy link
Collaborator Author

What gave me the idea that this was the solution was the existing note feature.
You can add a note to a treatment. You can also just place a note.
If you look at the code, you will see that the existing code for placing a note, which does not exhibit this problem, does not assign a y position to the note by setting it to -1.

Seeing that and knowing that note does not have the issue, I tried it with treatment note and tests showed that removing the y position from treatment note solved the problem for treatment note as well.

@jamorham
Copy link
Collaborator

I'm not too sure about this because it looks like you're removing the facility of notes being placed at particular points on the Y axis when the problem is that this position is encoded in the current units and if those change then the note positions become invalid.

I feel like a better solution would be to detect notes which are outside of the currently selected unit range and scale those appropriately or to convert the Y positions in the database during a units change. I think the first option is likely preferable, as for example mmol would always be under 24 and mgdl always above it.

@Navid200
Copy link
Collaborator Author

Thanks for the review.

But, why does this problem never occur for a note? It only occurs for a note attached to a treatment.
The only difference is that a note is set up with -1 as the parameter.

showNoteTextInputDialog(myitem, tsl(), -1);

Screenshot 2024-09-27 105338

When I saw that this problem only occurred when I attach a note to a treatment, but does not when I just enter a note, I decided to see what the difference was.
The only difference is that we enter a y position for a treatment attached to a treatment. While we leave it blank when we only create a note.

@Navid200
Copy link
Collaborator Author

@jamorham Please see my post above. This PR only does what xDrip does already for notes. It's already been in xDrip for years and tested. Why can we not use it for notes attached to treatments as well?

@Navid200
Copy link
Collaborator Author

@jamorham I can do what you have suggested.

But, will you do me a favor? Would you please tell me that you are aware that note (not treatment note) already does what this PR suggests and knowing that, you still prefer that I do what you have suggested?
I would just like to make sure you are aware that we are already doing what this PR suggests and we have been doing it for years.

Thanks

@Navid200
Copy link
Collaborator Author

Navid200 commented Sep 30, 2024

Installed the latest Nightly, 2024.09.27, on a test phone and performed a sequence of tests (Before). Then, installed this PR on the same phone and performed the exact same tests (After).


Set the units to mg/dL. Added a note.
1

Added a treatment.
2

Changed the unit to mmol/L. The y axis range remains unchanged.
3


Changed the unit back to mg/dL. Tapped on the treatment and added a note (Test2) to it.
4


Changed the unit to mmol/L.
5

You can see that with the latest Nightly, the note remains where it was. But, the treatment is now at a very high value causing the y axis range to expand.
After this PR, the treatment note does not have an impact on the position of the treatment unlike the latest Nightly.

@Navid200
Copy link
Collaborator Author

I'm not too sure about this because it looks like you're removing the facility of notes being placed at particular points on the Y axis when the problem is that this position is encoded in the current units and if those change then the note positions become invalid.

The y position is defined by the treatment. The note added to the treatment overrides that position in xDrip as is. This PR avoids impacting the position of the treatment when adding a note to it.

@Navid200 Navid200 requested a review from jamorham September 30, 2024 22:13
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.

xDrip follower sees compressed bg scale if on mmol/l while master on mg/dl
2 participants