Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Improve the dialog's features related to BackendSyncException #17017

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

Conversation

peyman79
Copy link
Contributor

@peyman79 peyman79 commented Sep 4, 2024

Purpose / Description

After clicking the sync button, if your current time is invalid, a dialog will appear notifying you that your time settings are incorrect. This dialog now includes a "Go to Settings" button, allowing you to easily adjust your date and time to the correct settings. Additionally, the dialog displays the current time for your reference.

Fixes

How Has This Been Tested?

Here is a screenshot of the result:

Screenshot 2024-09-05 114330

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

Copy link
Contributor

github-actions bot commented Sep 4, 2024

Message to maintainers, this PR contains strings changes.

  1. Before merging this PR, it is best to run the "Sync Translations" GitHub action, then make and merge a PR from the i18n_sync branch to get translations cleaned out.
  2. Then merge this PR, and immediately do another translation PR so the huge change made by this PR's key changes are all by themselves.

Read more about updating strings on the wiki,

@user1823
Copy link
Contributor

user1823 commented Sep 4, 2024

Additionally, the dialog displays the current time for your reference.

Is this the server time or the device time?

@peyman79
Copy link
Contributor Author

peyman79 commented Sep 4, 2024

Additionally, the dialog displays the current time for your reference.

Is this the server time or the device time?

It's device time.

@user1823
Copy link
Contributor

user1823 commented Sep 4, 2024

So, what about using Current Device Time: ... to make it clearer?

Or what about using a sentence:
Your clock is currently set to ....

@peyman79 peyman79 force-pushed the Improve-clock-sync-error-dialog branch from e9827c2 to 750ea0e Compare September 5, 2024 08:33
@peyman79
Copy link
Contributor Author

peyman79 commented Sep 5, 2024

Your clock is currently set to ....

Yes you're right. I changed it, It's better now.

Now this dialog has a "Go to setting" button. Also it shows the current time.
@peyman79 peyman79 force-pushed the Improve-clock-sync-error-dialog branch from 750ea0e to b660a05 Compare September 6, 2024 16:03
Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

I'm not certain all "other" sync exceptions are device time related
If there are other subtypes in there, this would be helpful in some cases but harmful in others, so it needs some thought as mentioned in comment

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

Comment on lines +177 to +179
is BackendSyncException -> {
// this exceptions do not generate worthwhile crash reports
showInvalidClockDialog(this, exc.localizedMessage!!, exc)
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

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve "your clock is not set to the correct time" error
4 participants