Don't ask user to start G6 before connectivity #2757

Navid200 · 2023-03-31T05:00:47Z

After selecting G6 in source wizard, a red notification on the main screen asks user to start sensor or change settings.
Shortly after, a dialog appears asking user to start sensor.

This PR delays those until transmitter number of days resolves to a valid integer.
In the meantime, it shows the following instead.

This PR complements this PR: #2746
But, the two PRs are independent. Meaning if you merge only one and hold off on the other for more info, there will be no issue. It will still be a step forward,

fwolter · 2023-04-07T18:03:31Z

app/src/main/java/com/eveningoutpost/dexdrip/Home.java

@@ -2612,7 +2614,12 @@ private void updateCurrentBgInfoCommon(DexCollectionType collector, TextView not
        }

        if (!isSensorActive) {
-            notificationText.setText(R.string.now_start_your_sensor);
+            boolean G6_not_connected_yet = Pref.getBooleanDefaultFalse("using_g6") && DexTimeKeeper.getTransmitterAgeInDays(getTransmitterID()) == -1;


You could stick to the Java convention that variable names should be camelCase, i.e. start with a lower character.

Suggested change

boolean G6_not_connected_yet = Pref.getBooleanDefaultFalse("using_g6") && DexTimeKeeper.getTransmitterAgeInDays(getTransmitterID()) == -1;

boolean g6NotConnectedYet = Pref.getBooleanDefaultFalse("using_g6") && DexTimeKeeper.getTransmitterAgeInDays(getTransmitterID()) == -1;

My objective is to get this change added so that xDrip functionality improves for the inexperienced user.
I am more than happy to make any necessary change to get approval.

Therefore, if I am told that this is what's holding this PR up, I will change it.

Let me add that looking at this same file, I can see plenty of existing uppercase instances. For example:

xDrip/app/src/main/java/com/eveningoutpost/dexdrip/Home.java

Line 258 in 35d548f

private static final int SHOWCASE_G5FIRMWARE = 9;

So, SHOWCASE_G5FIRMWARE is already there. If we are going to change the code for some reason, we should be consistent.
If, we want to say, let's leave the existing cases alone. But, let's follow this guideline for the new variables, I am happy to follow such a guideline if I can get an approval of it.

So, I thank you for having a look and making the suggestion. But, I am not going to make your requested change because I have no idea it is going to make any difference in what really matters in getting this approved or not.

It just caught my eye when skimming through your PR. The constant in line 258 is fine by convention. https://www.oracle.com/java/technologies/javase/codeconventions-namingconventions.html

Thanks a lot for providing this guidance.
I will change to follow the standard. I have no reason not to follow it.

jamorham · 2023-04-10T14:21:59Z

app/src/main/java/com/eveningoutpost/dexdrip/Home.java

@@ -2628,7 +2635,7 @@ private void updateCurrentBgInfoCommon(DexCollectionType collector, TextView not
                    dialog = builder.create();
                    dialog.show();
                } else {
-                    if (!Experience.gotData() && !QuickSettingsDialogs.isDialogShowing() && JoH.ratelimit("start-sensor_prompt", 20)) {
+                    if (!Experience.gotData() && !QuickSettingsDialogs.isDialogShowing() && JoH.ratelimit("start-sensor_prompt", 20) && !notConnectedToG6Yet) {


So your !notConnectedToG6Yet condition should come before the ratelimit as everything to the right of the ratelimit is not evaluated when the ratelimit has been exceeded. I believe in this case you would only want the ratelimit to restricting firing of the conditional block after all conditions have been met.

I have looked at this again. I don't believe there is any issue:

If anything in the if statement fails to be true, like the ratelimit, we don't care, we don't want it to be true then. In that case, we want the "if" statement to fail as it did before this PR anyway.

But, if everything in the "if" statement is true, we are adding one more check. In that case, only if the user has not selected G6, or G6 is already connected, do we allow the command to be executed.

In other words, if the user has not selected G6 or if they have and it is already connected, the dialog will be shown as it did before this PR.
However, if G6 is selected and is not connected yet, this PR does not allow the dialog to be shown.

Yes but if the user reaches the condition and the timekeeper condition is not met, but then the connection occurs in the background and would be true and the user then re-enters this conditional block again within the 20 second rate limit period, it will fail because the ratelimit has been triggered even though it otherwise would have passed if the check order is changed so the ratelimiting is last. The && operator means the other clauses are not evaluated once one is false. Simply checking the ratelimit by calling its method triggers the time to be stored so that the condition will fail the next time you call it within the specified time period.

I'm so sorry I don't follow you.
I keep comparing what xDrip does now to what I want it to do after this PR, to help me see that this PR is correct.

If ratelimit fails, xDrip does not show the dialog right now. In that case, I don't want xDrip to show the dialog after this PR either. So, I don't understand the dellima you are describing.

Sorry!
I misunderstood the purpose of the bracket. As soon as ratelimit executes, it is going to show the dialog.
Thanks for catching this.

jamorham · 2023-04-10T14:27:21Z

This doesn't seem to be conditional on using dex collection type so I think it would break for other collector types as the condition can never be met.

Navid200 · 2023-04-10T15:16:02Z

This doesn't seem to be conditional on using dex collection type so I think it would break for other collector types as the condition can never be met.

Thanks for the review.
I will respond to/modify PR with respect to your other comment separately. But, I would like to address this one first.

This PR behaves differently than what exists currently on two occasions only and in both of those G6 has been selected. Please let me explain.

Because of the following variable definition, it only becomes true if G6 has been selected.

boolean notConnectedToG6Yet = Pref.getBooleanDefaultFalse("using_g6") && DexTimeKeeper.getTransmitterAgeInDays(getTransmitterID()) == -1;

1- This is the first case where this PR behaves differently than before:

if (notConnectedToG6Yet) {
notificationText.setText(R.string.wait_to_connect);
}

And you can see that it happens only if the variable is true, meaning only if G6 is selected by the user.

2- This is the only other case this PR behaves differently than before:

if (!Experience.gotData() && !QuickSettingsDialogs.isDialogShowing() && JoH.ratelimit("start-sensor_prompt", 20) && !notConnectedToG6Yet) {

When the if condition above is true, the conditional command shows a dialog. If the user has chosen any sensor other than G6, "!notConnectedToG6Yet" will be true. In that case, the "if" condition result is not affected by this PR (A && TRUE = A). The only time the above "if" condition fails to be true due to this PR is if the user has selected a G6 and G6 is not connected yet. In that case, this change will not allow the dialog to be shown.

Therefore, the xDrip behavior is not affected by this PR if any sensor other than G6 is selected.

I admit that this is not very easy to understand.
Would this be accepted if I add comments to the code to explain all of this?

Again, I will make changes to address your comment about ratelimit.

Navid200 · 2023-04-11T14:42:59Z

Now that the other PR is merged, should I edit this one to use the new method from it?
So that if some day it is modified, everything stays related and consistent?

Navid200 · 2023-04-17T16:51:34Z

Resolved conflict after refactoring to utilitymodels.

Tested. Everything works fine.

Navid200 · 2023-04-21T20:46:33Z

Resolved conflicts after refactoring to models.

Tested. Everything works fine.

jamorham · 2023-05-02T17:10:32Z

I think if you test by selecting G6 from the source wizard and then change collector type to Libre then your condition will still be triggered as using_g6 is not sufficient to tell the collector type. You probably want something like getDexCollectionType() but even then you should test for using native mode for completeness. Let me know if you think this is incorrect?

Navid200 · 2023-05-05T04:03:41Z

Thanks for catching my error, and offering guidance.

jamorham

I think this should work and I believe you have tested it.

Navid200 · 2023-06-23T22:20:37Z

Yes, it is tested.
I hope you don't mind. But, we may need to tighten this. But, we may not. This is a first, and hopefully last, step.

fwolter reviewed Apr 7, 2023

View reviewed changes

Navid200 marked this pull request as draft April 7, 2023 23:12

Navid200 marked this pull request as ready for review April 7, 2023 23:46

jamorham reviewed Apr 10, 2023

View reviewed changes

Navid200 closed this Apr 21, 2023

Navid200 force-pushed the Navid_2023_03_30 branch from a2d5682 to 9153bba Compare April 21, 2023 12:24

Refactoring to models

5053747

Navid200 reopened this Apr 21, 2023

Navid200 marked this pull request as draft May 2, 2023 17:12

Conclusively detecting native G6

acc64c0

Navid200 marked this pull request as ready for review May 5, 2023 04:03

Navid200 requested a review from jamorham May 10, 2023 05:20

jamorham approved these changes Jun 23, 2023

View reviewed changes

jamorham merged commit 67340c4 into NightscoutFoundation:master Jun 24, 2023

Navid200 deleted the Navid_2023_03_30 branch June 24, 2023 12:42

Navid200 mentioned this pull request Jul 5, 2024

Delay G6 start prompt by 5 more minutes! #3558

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't ask user to start G6 before connectivity #2757

Don't ask user to start G6 before connectivity #2757

Navid200 commented Mar 31, 2023 •

edited

Loading

fwolter Apr 7, 2023

Navid200 Apr 7, 2023

fwolter Apr 7, 2023

Navid200 Apr 7, 2023

jamorham Apr 10, 2023

Navid200 Apr 10, 2023 •

edited

Loading

Navid200 Apr 10, 2023

jamorham Apr 10, 2023

Navid200 Apr 10, 2023

Navid200 Apr 10, 2023

Navid200 Apr 10, 2023

Navid200 Apr 10, 2023

jamorham commented Apr 10, 2023

Navid200 commented Apr 10, 2023

Navid200 commented Apr 11, 2023

Navid200 commented Apr 17, 2023

Navid200 commented Apr 21, 2023

jamorham commented May 2, 2023

Navid200 commented May 5, 2023 •

edited

Loading

jamorham left a comment

Navid200 commented Jun 23, 2023

	boolean G6_not_connected_yet = Pref.getBooleanDefaultFalse("using_g6") && DexTimeKeeper.getTransmitterAgeInDays(getTransmitterID()) == -1;
	boolean g6NotConnectedYet = Pref.getBooleanDefaultFalse("using_g6") && DexTimeKeeper.getTransmitterAgeInDays(getTransmitterID()) == -1;

Don't ask user to start G6 before connectivity #2757

Don't ask user to start G6 before connectivity #2757

Conversation

Navid200 commented Mar 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Navid200 Apr 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamorham commented Apr 10, 2023

Navid200 commented Apr 10, 2023

Navid200 commented Apr 11, 2023

Navid200 commented Apr 17, 2023

Navid200 commented Apr 21, 2023

jamorham commented May 2, 2023

Navid200 commented May 5, 2023 • edited Loading

jamorham left a comment

Choose a reason for hiding this comment

Navid200 commented Jun 23, 2023

Navid200 commented Mar 31, 2023 •

edited

Loading

Navid200 Apr 10, 2023 •

edited

Loading

Navid200 commented May 5, 2023 •

edited

Loading