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.

Sign up for GitHub

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

Jump to bottom

Improve the dialog's features related to BackendSyncException #17017

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
+38 −2
Draft

Improve the dialog's features related to BackendSyncException #17017

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 36 additions & 2 deletions AnkiDroid/src/main/java/com/ichi2/anki/CoroutineHelpers.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import android.app.Activity
import android.app.Dialog
import android.content.Context
import android.content.DialogInterface
import android.content.Intent
import android.provider.Settings
import android.view.WindowManager
import android.view.WindowManager.BadTokenException
import androidx.annotation.StringRes
Expand Down Expand Up @@ -60,6 +62,9 @@ import net.ankiweb.rsdroid.exceptions.BackendNetworkException
import net.ankiweb.rsdroid.exceptions.BackendSyncException
import org.jetbrains.annotations.VisibleForTesting
import timber.log.Timber
import java.time.LocalDateTime
import java.time.format.DateTimeFormatter
import java.util.Locale
import kotlin.coroutines.CoroutineContext
import kotlin.coroutines.EmptyCoroutineContext
import kotlin.coroutines.resume
Expand Down Expand Up @@ -169,8 +174,12 @@ suspend fun <T> FragmentActivity.runCatching(
Timber.w(exc, errorMessage)
exc.localizedMessage?.let { showSnackbar(it) }
}
is BackendNetworkException, is BackendSyncException -> {
// these exceptions do not generate worthwhile crash reports
is BackendSyncException -> {
// this exceptions do not generate worthwhile crash reports
showInvalidClockDialog(this, exc.localizedMessage!!, exc)
Comment on lines +177 to +179
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you certain that the only BackendSyncException is from invalid device time?
Even if you are certain of that now are we certain that will remain true forever?
Is there some way to constrain this new dialog to present only when we are certain that it was from an invalid device time?

Perhaps a string match on exception message "your clock is not set to correct time"? But I believe this message comes through localized and may be translated to whatever we set the backend language too (which is ideally the user's selected language) which means a string search is not valid

So, a specific question for @dae then: is BackendError.Kind.SYNC_OTHER_ERROR a set of errors of different sub-types - sort of a grab bag - or is it always device time related? If it is multiple different types is there any way to differentiate with certainty whether a specific SYNC_OTHER_ERROR is device time related ?

I trolled through and I couldn't see anything specific about BackendError.Kind.SYNC_OTHER_ERROR being constrained only to device time issues:

https://github.com/ankitects/anki/blob/73c97de5d022abcec0ab1fb70921c6bf813cfa28/proto/anki/backend.proto#L33

For this PR, if this is always a device time error, great. If it is not always device time but we can differentiate with certainty, then we need to only show this specific dialog when we are certain, otherwise use old code path. If there is no way to differentiate then the Dialog needs to be made much more ambiguous and say something like "if the error is device time related, you may wish to open time settings and update your time, current device time is xxxx" or similar
...that's the only reference I have really, as the ankiweb sync server code does not seem to be searchable

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, lost track of this one. If AD wants to be able to identify the clock message separately, it should send through a PR that moves the error out of OTHER and into a separate specific error, like we currently use to detect sync auth failures. Other is basically a catch-all for errors that we don't (currently) need to special-case in the frontend.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 pretty far out of my wheelhouse but after some spelunking, is that roundabouts here?

https://github.com/ankitects/anki/blob/1b7390f22d56cb566ee3aba2faff3615275c5aa4/rslib/src/backend/error.rs#L65-L72

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that maps our internal error to the limited protobuf errors we export. With a new case in the .proto file, ClockIncorrect could be mapped to it.

I had AnkiDroid notifications turned off which is why I missed this (again). I should see future pings.

}
is BackendNetworkException -> {
// this exceptions do not generate worthwhile crash reports
showError(this, exc.localizedMessage!!, exc, false)
}
is BackendException -> {
Expand Down Expand Up @@ -278,6 +287,31 @@ fun showError(context: Context, msg: String, exception: Throwable, crashReport:
}
}

fun showInvalidClockDialog(context: Context, msg: String, exception: Throwable) {
if (throwOnShowError) throw IllegalStateException("throwOnShowError: $msg", exception)
try {
val currentTime = LocalDateTime.now()
val dateFormatter = DateTimeFormatter.ofPattern("yyyy-MM-dd hh:mm:ss a", Locale.getDefault())
val formattedTime = currentTime.format(dateFormatter)
val invalidClockMessage = context.getString(R.string.InvalidClockDialog)

val fullMessage = "$msg\n\n$invalidClockMessage\n$formattedTime"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 normally I would be wary of string concatenation as it may break RTL languages, but it is on a new line so it should be okay

AlertDialog.Builder(context).show {
title(R.string.vague_error)
message(text = fullMessage)
positiveButton(R.string.dialog_go_to_setting) {
// Navigate to Date and Time settings
val intent = Intent(Settings.ACTION_DATE_SETTINGS)
context.startActivity(intent)
}
negativeButton(R.string.dialog_cancel)
}
} catch (ex: BadTokenException) {
// issue 12718: activity provided by `context` was not running
Timber.w(ex, "unable to display error dialog")
}
}

/** In most cases, you'll want [AnkiActivity.withProgress]
* instead. This lower-level routine can be used to integrate your own
* progress UI.
Expand Down
2 changes: 2 additions & 0 deletions AnkiDroid/src/main/res/values/03-dialogs.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@
<string name="dialog_positive_overwrite">Overwrite</string>
<string name="dialog_positive_repair">Repair</string>
<string name="dialog_positive_replace">Replace</string>
<string name="dialog_go_to_setting">Go to setting</string>

<string name="dialog_collection_path_not_dir">The path specified wasn’t a valid directory</string>
<string name="dialog_invalid_custom_certificate">The provided text does not resolve to a valid PEM-encoded X509 certificate</string>
Expand Down Expand Up @@ -222,6 +223,7 @@
sched V2. For V1, it was counting the number of repetitions of cards in learning. This approximation is acceptable
for the sake of simplicity and because V1 will leave.-->
<string name="deck_picker_counts">Open the deck overview page containing the number of cards to see today.</string>
<string name="InvalidClockDialog">Your clock is currently set to:</string>

<string name="import_deck_package">Deck package (.apkg)</string>
<string name="import_collection_package">Collection package (.colpkg)</string>
Expand Down