[GSoC] Add preference to allow/warn if using metered connections #12121

BrayanDSO · 2022-08-20T23:19:12Z

Pull Request template

Purpose / Description

Internet may be expensive 💵💰💲💸

Fixes

Fixes #10706

Approach

Clean things a bit
Create a "Advanced" category under "Sync", so things get a little more organized with the increasing number of preferences
Add a preference to allow sync on metered connections (off by default)
- If not allowed, auto sync under metered connections won't occur + the user will be warned if they trigger a manual sync under a metered connection
- I'm quite open to string changes

How Has This Been Tested?

Screen_Recording_20220820-200646_AnkiDroid.mp4

Checklist

Please, go through these checks before submitting the PR.

You have a descriptive commit message with a short title (first line, max 50 chars).
You have commented your code, particularly in hard-to-understand areas
You have performed a self-review of your own code
UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
UI Changes: You have tested your change using the Google Accessibility Scanner

lukstbit

Looks good with two minor issues.

AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt

AnkiDroid/src/main/res/values/03-dialogs.xml

AnkiDroid/src/main/java/com/ichi2/anki/AnkiDroidJsAPI.kt

AnkiDroid/src/main/java/com/ichi2/utils/NetworkUtils.kt

Arthur-Milchior · 2022-08-21T22:21:00Z

AnkiDroid/src/main/java/com/ichi2/utils/NetworkUtils.kt

+                .getSystemService(Context.CONNECTIVITY_SERVICE) as ConnectivityManager
+            cm.isActiveNetworkMetered
+        } catch (e: Exception) {
+            Timber.w(e, "Exception obtaining metered connection - assuming metered connection")


I am very much against it.
I prefer that we use an enum with three values, yes, no, unknown.

I'll admit that in any case, we will never get real valid information, and that's a shame. So we should never be assertive one way or another.
This is requiring the user to grasp a concept that is not necessarily straighforward. I prefer that we are able to tell them honestly when we can't know instead of mixing it with the case where we are sure.
I agree it makes sense to behave in the same way in both case, and ask for confirmation. However, I hope we can have different warning messages

I'm not sure for JS how to transform it. I'd consider using two boolean function, except that I would not want that each card do the request twice. Is there an easy way to send an enum to JS? Otherwise, I'd want the function to be
isConnectionNotMetered with 0 meaning false, 1 true, and 2 "unknown". So that if a code just do if(!isConnectionNotMetered) {useNetwork} it uses network only if it is certain it's okay to do so.

I actually just copied and pasted the method, a 100% straight extraction. It wasn't my goal to refactor it on this PR.

But I agree if your points. Looking on the source code, I only expect it to return an error in the case there isn't a specific permission on the manifest. AnkiDroid's manifest has it, so I don't think that the try catch is necessary.

Sorry, I didn't realize on first read that you were extracting a method. Then I guess fine.

I still believes my remarks make sense, but it can wait then.

AnkiDroid/src/main/java/com/ichi2/utils/NetworkUtils.kt

AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt

Arthur-Milchior · 2022-08-21T23:08:40Z

AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt

-                        )
-                    )
-                )
+                val shouldFetchMedia = preferences.getBoolean("syncFetchesMedia", true)


I'd appreciate for a next time that you do this in multiple commit.

Extracting variables in a commit,

extracting doSync

then taking into account whether it's metered

We can put it in a separate PR and squash it, but it would really help to review and save me time

doSync is mostly only useful inside sync(), so to reduce boilerplate, I've kept it as a nested function. Added a javadoc to make it more clear. Let me know if that is fine

mikehardy · 2022-08-24T00:44:05Z

Took a conflict just now, and I'm not sure what the state on it is? Is it ready for re-review from previous reviewers?
Only asking because I'm scouring for things to merge before doing a new build and this caught my eye 👀

BrayanDSO · 2022-08-24T09:07:01Z

Yes, it is ready for review. Just rebased to solve conflicts. Only the last 3 commits are new.

AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt

mikehardy

This looks good to me, my limited data friends (and myself on occasion - if I'm syncing a big deck) will appreciate this.

mikehardy · 2022-08-24T22:55:13Z

AnkiDroid/src/main/java/com/ichi2/utils/NetworkUtils.kt

+        cm.isActiveNetworkMetered
+    } catch (e: Exception) {
+        Timber.w(e, "Exception obtaining metered connection - assuming metered connection")
+        true


@Arthur-Milchior in your review you wanted an enum, with this refactor here to a simple util method, I think this is clean and correct and just binary. The assertion it is correct is that from the perspective of consuming people's money (using metered data) defaulting to a cautious stance of assuming it is metered when we catch an (almost entirely impossible) exception is the right one.

So I think this may answer your review comments well

As far as I can tell, this commit does not change the slightest the actual code used. It just ensures that we remove the object from the file.

If most people agree that as soon as we get an exception, we warn in the same way, fine by me I guess.
Anyway, this PR is already a great improvement, and if I can convince peolpe, the change can be done later easily.

One regret I have, but that always can be dealt with later too is that we should send a crash report. I'd love to know if it's something that actually occurs, hether it's related to specific version of the API or specific manufacturers. Also maybe what exceptions we actually get. So that maybe later we could get a better catching and warning

(By the way, this did not seems to require an extra commit. You could just have directly create the file without any enclosing "object NetworkUtils" when you created the file

Issue resolved

Arthur-Milchior

Generally approve.
I believe we could save two commits.
If we squash, feel free to squash.
If we don't, then feel free to merge as soon as the correction is done

Arthur-Milchior · 2022-08-26T12:13:44Z

AnkiDroid/src/main/java/com/ichi2/utils/NetworkUtils.kt

+                .getSystemService(Context.CONNECTIVITY_SERVICE) as ConnectivityManager
+            cm.isActiveNetworkMetered
+        } catch (e: Exception) {
+            Timber.w(e, "Exception obtaining metered connection - assuming metered connection")


Sorry, I didn't realize on first read that you were extracting a method. Then I guess fine.

I still believes my remarks make sense, but it can wait then.

Arthur-Milchior · 2022-08-26T12:35:36Z

AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt

-        // Check whether the option is selected, the user is signed in and last sync was AUTOMATIC_SYNC_TIME ago
-        // (currently 10 minutes)
+        // Check whether the option is selected, the user is signed in, last sync was AUTOMATIC_SYNC_TIME ago
+        // (currently 10 minutes), and is not under a metered connection (if not allowed by preference)


I'm fine with the commint "Add preference to check if sync is on metered connection" except for his title
You're adding a permission request, that should be in the commit message.
Also, you are not adding adding a preference but taking it into account. I suggest it may be nice to indicates both fact

Thanks for the heads up. Changed the commit message to:

Add "Allow sync on metered connections" preference
If disabled, auto-sync isn't triggered on metered connections and the user is asked if they want to continue if they trigger a manual sync
If enabled, sync occurs normally

Arthur-Milchior · 2022-08-26T12:56:23Z

AnkiDroid/src/main/java/com/ichi2/utils/NetworkUtils.kt

+        cm.isActiveNetworkMetered
+    } catch (e: Exception) {
+        Timber.w(e, "Exception obtaining metered connection - assuming metered connection")
+        true


As far as I can tell, this commit does not change the slightest the actual code used. It just ensures that we remove the object from the file.

If most people agree that as soon as we get an exception, we warn in the same way, fine by me I guess.
Anyway, this PR is already a great improvement, and if I can convince peolpe, the change can be done later easily.

One regret I have, but that always can be dealt with later too is that we should send a crash report. I'd love to know if it's something that actually occurs, hether it's related to specific version of the API or specific manufacturers. Also maybe what exceptions we actually get. So that maybe later we could get a better catching and warning

(By the way, this did not seems to require an extra commit. You could just have directly create the file without any enclosing "object NetworkUtils" when you created the file

AnkiDroid/src/main/java/com/ichi2/utils/NetworkUtils.kt

AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt

for better organization

If disabled, auto-sync isn't triggered on metered connections and the user is asked if they want to continue if they trigger a manual sync If enabled, sync occurs normally

mikehardy · 2022-08-26T22:40:23Z

I think we are good to go with the standard rebase merge here? I'll do a strings run now

BrayanDSO added Review High Priority Request for high priority review Strings labels Aug 20, 2022

BrayanDSO changed the title ~~[GSoC] Add preference to~~ [GSoC] Add preference to allow/warn if using metered connections Aug 20, 2022

lukstbit previously requested changes Aug 21, 2022

View reviewed changes

AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt Outdated Show resolved Hide resolved

AnkiDroid/src/main/res/values/03-dialogs.xml Show resolved Hide resolved

lukstbit added the Needs Author Reply Waiting for a reply from the original author label Aug 21, 2022

BrayanDSO force-pushed the meteredSync branch from c30ee58 to c37f31e Compare August 21, 2022 11:14

lukstbit added Needs Second Approval Has one approval, one more approval to merge and removed Needs Author Reply Waiting for a reply from the original author labels Aug 21, 2022

Arthur-Milchior reviewed Aug 21, 2022

View reviewed changes

BrayanDSO force-pushed the meteredSync branch from 1fa2a29 to a081ebf Compare August 24, 2022 09:06

BrayanDSO force-pushed the meteredSync branch from a081ebf to 96dd7f2 Compare August 24, 2022 10:24

oakkitten reviewed Aug 24, 2022

View reviewed changes

AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt Outdated Show resolved Hide resolved

BrayanDSO requested a review from Arthur-Milchior August 24, 2022 17:30

mikehardy approved these changes Aug 24, 2022

View reviewed changes

Arthur-Milchior approved these changes Aug 26, 2022

View reviewed changes

Extract isActiveNetworkMetered()

d33441a

BrayanDSO force-pushed the meteredSync branch 2 times, most recently from 3cc6fb9 to 844dcbf Compare August 26, 2022 16:09

BrayanDSO added 4 commits August 26, 2022 13:16

NF: refactor isActiveNetworkMetered()

57856ec

Create an Advanced category under "Sync" preferences

0942a29

for better organization

NF: make DeckPicker.automaticSync() more readable

de6d4c5

Add "Allow sync on metered connections" preference

763cb17

If disabled, auto-sync isn't triggered on metered connections and the user is asked if they want to continue if they trigger a manual sync If enabled, sync occurs normally

BrayanDSO force-pushed the meteredSync branch from 844dcbf to 763cb17 Compare August 26, 2022 16:16

mikehardy mentioned this pull request Aug 26, 2022

Updated strings from Crowdin 20220826 #12190

Merged

mikehardy merged commit f67bb49 into ankidroid:main Aug 27, 2022

github-actions bot removed the Review High Priority Request for high priority review label Aug 27, 2022

github-actions bot removed the Needs Second Approval Has one approval, one more approval to merge label Aug 27, 2022

github-actions bot added this to the 2.16 release milestone Aug 27, 2022

BrayanDSO deleted the meteredSync branch August 27, 2022 11:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GSoC] Add preference to allow/warn if using metered connections #12121

[GSoC] Add preference to allow/warn if using metered connections #12121

BrayanDSO commented Aug 20, 2022

lukstbit left a comment

Arthur-Milchior Aug 21, 2022

BrayanDSO Aug 22, 2022

Arthur-Milchior Aug 26, 2022

Arthur-Milchior Aug 21, 2022

BrayanDSO Aug 22, 2022

mikehardy commented Aug 24, 2022

BrayanDSO commented Aug 24, 2022

mikehardy left a comment

mikehardy Aug 24, 2022

Arthur-Milchior Aug 26, 2022

Arthur-Milchior left a comment

Arthur-Milchior Aug 26, 2022

Arthur-Milchior Aug 26, 2022

BrayanDSO Aug 26, 2022

Arthur-Milchior Aug 26, 2022

mikehardy commented Aug 26, 2022

[GSoC] Add preference to allow/warn if using metered connections #12121

[GSoC] Add preference to allow/warn if using metered connections #12121

Conversation

BrayanDSO commented Aug 20, 2022

Pull Request template

Purpose / Description

Fixes

Approach

How Has This Been Tested?

Checklist

lukstbit left a 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

mikehardy commented Aug 24, 2022

BrayanDSO commented Aug 24, 2022

mikehardy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arthur-Milchior left a 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

mikehardy commented Aug 26, 2022