Make custom sync server preferences easier to understand #12367

oakkitten · 2022-09-10T13:36:49Z

This implements my own suggestion from the point 2 in Remaining issues in #12112 nearly verbatim, except I added a note about Sync url preference expecting the full URL as discussed in #12269. @dobefore What do you think?

device-2022-09-08-015053.mp4

As usual, please see the commits for individual changes.

Note that in the first commit there's a link to the fragment https://docs.ankidroid.org/#_custom_sync_server which doesn't exist yet. I suppose the docs will have to be amended for the new release, and this section could be added later.

Also note that Accessibility scanner complains about two things; not sure if this is actionable.

Help preference summary: Link text. Consider using more descriptive text in the link. The link “Learn more” may not independently convey the link's purpose.
Sync url preference switch widget: Item descriptions. Multiple items have the same description. This clickable item's speakable text: "OFF" is identical to that of 1 other item(s).

Edit: The test pass in my repo.

How Has This Been Tested?

Manually tested on my Xiaomi Mi A2 (Android 11) and Motorola Moto G 1st Gen (Android 6).
Added a unit test for preference migration.

Checklist

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

github-actions · 2022-09-10T13:37:01Z

Message to maintainers, this PR contains strings changes.

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.
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,

BrayanDSO

Overall I like it. It's a great improvement on this screen. Even being the least used screen, it's a nice progress for the app

AnkiDroid/src/main/java/com/ichi2/anki/preferences/HtmlHelpPreference.kt

AnkiDroid/src/main/java/com/ichi2/anki/preferences/VersatileTextPreference.kt

AnkiDroid/src/main/java/com/ichi2/anki/preferences/VersatileTextWithASwitchPreference.kt

AnkiDroid/src/main/java/com/ichi2/anki/web/CustomSyncServer.kt

AnkiDroid/src/main/res/values/10-preferences.xml

BrayanDSO · 2022-09-10T17:34:33Z

AnkiDroid/src/main/res/values/10-preferences.xml

@@ -206,6 +206,9 @@
    <string name="advanced_forecast_stats_mc_n_iterations_title" maxLength="41">Number of iterations of the simulation</string>
    <!-- Custom sync server settings -->
    <string name="custom_sync_server_title" maxLength="41">Custom sync server</string>
+    <string name="custom_sync_server_summary_none_of_the_two_servers_used">Not used</string>
+    <string name="custom_sync_server_summary_both_or_either_of_the_two_servers_used">Collection: %1$s\nMedia: %2$s</string>
+    <string name="custom_sync_server_summary_placeholder_default">default</string>


Couldn't we use the AnkiWeb string here? I think that it is more specific + would be one less string to translate

I thought about that and while I don't have a strong preference I like “default” more. If you read Custom server? AnkiWeb! it may sound a bit like maybe AnkiWeb is the name of a server application that you can install for yourself. “Default” also chimes a bit better with “Not used”, which I liked better than having “Default” if neither server is used. If you say “AnkiWeb” then “Not used” is a bit awkward... I guess in that case you'd also want to say “AnkiWeb” and that would be less ideal. Anyway, I'not a native so this is not a strong opinion.

To me, it's more of a question that I have to think what default means, while AnkiWeb is straighthead, no thinking and no confusion. I'm not a native as well and I don't find having Not used first and AnkiWeb awkward at all. On my mind, their uses are distinct enough

I think that in the context of custom x, it's rather clear that default x means not using the custom x. Or maybe not. Where are the natives when you need them? 😪

I think the confusion can be minimised if we just mention that AnkiWeb is the default server with something like:
AnkiWeb (default)

I still dislike AnkiWeb, even with (default), for the reasons mentioned above. I am willing to give in to peer pressure, but I request more peers (or one very assertive peer).

AnkiDroid/src/main/res/values/10-preferences.xml

AnkiDroid/src/main/java/com/ichi2/anki/preferences/CustomSyncServerSettingsFragment.kt

AnkiDroid/src/main/res/xml/preferences_custom_sync_server.xml

BrayanDSO · 2022-09-24T23:23:10Z

Pending to me will be only

the URL extraction to a constant from the string, so it may be changed in the future if necessary
Adding search:ignore to the HtmlHelpPreference

I'll leave the strings' texts to the natives.

oakkitten · 2022-09-25T15:19:07Z

Rebased onto v2.16alpha85 to resolve a conflict and to add search:ignore to HtmlHelpPreference

mikehardy

I like all of this except:

I liked the language but tried to make it even more precise + prescriptive
we must maintain URL flexibility without trashing translations, so that should be interpolated

AnkiDroid/src/main/res/values/10-preferences.xml

mikehardy · 2022-10-13T15:05:46Z

And took a conflict during my current merge-fest, apologies. Probably an AlertDialog thing 🙈

oakkitten · 2022-10-15T20:09:11Z

Rebased on current main to resolve conflicts, also incorporated suggested wording changes into the first commit and added a commit extracting the URL into a separate string. Now if the tests would be so kind to pass...

BrayanDSO

The last changes LGTM.

Noticed only now, after reading HtmlHelpPreference javadoc, that the new preferences are on com.ichi2.anki.preferences, but all the other individual Preference classes are on com.ichi2.preferences, so they should be moved.

oakkitten · 2022-10-15T22:28:54Z

Good catch! Rebased. (Rebasing with renaming is hell!)

I put the helper methods into a new file com/ichi2/preferences/PreferenceUtil.kt since they belong with the preferences. They were in com/ichi2/preferences/anki/PreferenceUtils.kt before. Didn't have a better name for the file, opted to change Utils to Util so it's not the same.

BrayanDSO

Renaming on a rebase is quite painful. My condolences

edit: There is a lint error. Good old missing copyright header

mikehardy · 2022-10-17T04:20:15Z

and a conflict, just for extra fun 🙈

oakkitten · 2022-10-17T09:48:08Z

Done, and rebased on current main to resolve conflict.

mikehardy · 2022-10-17T14:37:08Z

Great - running through ./tools/test-commits.sh <hash> on the commit stream now, and will do a strings sync to clear things out for all the changes here

mikehardy

Well...that's weird, on the very first commit ./gradlew ktlintFormat fails with:

/home/mike/work/AnkiDroid/Anki-Android-Upstream/AnkiDroid/src/main/java/com/ichi2/preferences/PreferenceUtil.kt
interface DialogFragmentProvider should be declared in a file named DialogFragmentProvider.kt

...but....it works on the other 5 commits (!?)

Something about the extra sauce added in the following commit: d0e1209 makes ktlint okay with the interface in file not named after the interface 🤔

Could either dig deep and learn a bunch of things that may never be needed again to fix it, or just squash those two commits and be done, or squash them all 😆

Let me know what you think

HtmlHelpPreference is a preference that only provides help. It does not need a key and it is not selectable. It treats its summary text as HTML an renders it as such--including links. This commit implements it and adds it to the top of Custom sync server preference screen.

DialogFragmentProvider is an interface that slightly simplifies finding relations between the preferences that display dialogs on click, and the dialogs themselves.

At the time being, it serves two purposes: * It supports changing input type via android:inputType XML attribute, which can help the keyboard app with choosing a more suitable keyboard layout. * If continuousValidator is set, it is used to prevent the user from entering invalid data. On each text change, the validator is run; if it throws, the Ok button gets disabled. It was designed as a drop-in replacement of EditTextPreference with the extra functionality disabled by default. Additional functionality can be added as needed.

VersatileTextWithASwitchPreference is a combination of a TextPreference and a SwitchPreference. It mostly looks like a SwitchPreference, but the switch and the title can be clicked separately. The behavior is a bit special, see the javadoc for explanation.

Before, Custom sync server preferences were a bit confusing. There was a master switch for both of them, but you could actually disable either of the custom servers, or both of them, by not specifying the URL. This is an attempt to resolve the confusion by removing the master switch, and adding individual switches to the two custom server preferences. Also, the parent preference, that is the one that you click to get to Custom sync server preferences, now shows the status of both child preferences in its summary. Hopefully this is less confusing. Additionally, instead of showing an error dialog in the case of invalid URL, the edit text dialog doesn't allow accepting such an URL by disabling the Ok button. Allowing the user to okay bad input means it is going to be discarded, and that's pretty awful. What if user's custom sync URL is 420 characters long? Note that the URLs are now validated using OkHttp, which is more strict. Preference migration follows in the next commit.

Use custom sync server preference was the master switch for both collection and media custom sync servers. These preferences now have individual switches; this migration applies the old master switch to both individual switches, as long as the custom sync URLs are set.

oakkitten · 2022-10-17T19:03:22Z

I assume this rule is supposed to catch typos, like when you have a class FooBar in FooBaz.kt or something. Rebased to reorder commits 1 & 2 that should remove the lint error.

(Why squash though? I'm not exactly sure why one of commits failing lint is an issue. As long as you don't merge with --ff-only—I prefer --no-ff for almost every merge—you get a merge commit and you can revert a merge commit. You can eat a cake and have it too eh?

I'd even go so far to say there's not much value in ensuring every commit builds. If something breaks you can still use ~~git blame~~git bisect with --first-parent, and if this finds that the offending commit comes from a feature branch that has broken commits, well, fixing those commits while doing ~~git blame~~git bisect is about the same amount of work as doing it right away. Of course, if you happen to find that the feature is broken in another ~~git blame~~git bisect you'll have to redo the work but what are the chances of that.)

mikehardy · 2022-10-17T19:31:31Z

No idea what ktlint is unhappy with at a technical level here
github doesn't allow FF control when using web UI and we're using the web UI so it's a constraint we're stuck with
we have tools that git bisect, every commit building is a useful thing

will re-check...

oakkitten · 2022-10-17T19:38:30Z

I think that rule is not like an actual lint rule but more of a warning
It doesn't allow --ff-only, but I think the default merge strategy is --no-ff, so as long as you don't squash or rebase it will make a neat merge commit
I was actually talking about git bisect (I always confuse the two!), it also has --first-parent

mikehardy

/bin/nice -n 19 ./tools/test-commits.sh 0746ab0b4b08a74b6b7f609703d2628c496e8c35 clean

mikehardy · 2022-10-17T20:08:33Z

merge commit is not currently enabled for the repo, I've got squash or rebase - this is "rebase clean" now, so I'm going with that - will do the strings sync after

github-actions · 2022-11-01T16:37:25Z

Hi there @oakkitten! This is the OpenCollective Notice for PRs merged from 2022-10-01 through 2022-10-31

If you are interested in compensation for this work, the process with details is here:

https://github.com/ankidroid/Anki-Android/wiki/OpenCollective-Payment-Process#how-to-get-paid

We only post one comment per person per month to avoid spamming you, regardless of the number of PRs merged, but this note applies to all PRs merged for this month

Please note that GSoC contributions are okay for this process. Our philosophy is that our users have donated to AnkiDroid for all contributions. The only PRs that will not go through the OpenCollective process are ones directly related to am accepted GSoC project from a selected participant, since those receive a stipend from GSoC itself.

Please understand that our monthly budget is never guaranteed to cover all claims - the cap on payments-per-person may be lower, but we try to make our process as fair and transparent as possible, we just need your understanding.

Thanks!

github-actions bot added the Strings label Sep 10, 2022

BrayanDSO requested changes Sep 10, 2022

View reviewed changes

BrayanDSO added Needs Author Reply Waiting for a reply from the original author Needs Review labels Sep 10, 2022

BrayanDSO reviewed Sep 10, 2022

View reviewed changes

AnkiDroid/src/main/res/values/10-preferences.xml Outdated Show resolved Hide resolved

oakkitten force-pushed the custom-sync-server branch 2 times, most recently from 77cc1e5 to 19792cf Compare September 10, 2022 21:40

BrayanDSO reviewed Sep 10, 2022

View reviewed changes

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

oakkitten force-pushed the custom-sync-server branch from 19792cf to 4709427 Compare September 10, 2022 23:41

BrayanDSO reviewed Sep 24, 2022

View reviewed changes

AnkiDroid/src/main/res/xml/preferences_custom_sync_server.xml Outdated Show resolved Hide resolved

oakkitten force-pushed the custom-sync-server branch from 4709427 to 08fa835 Compare September 25, 2022 15:14

BrayanDSO 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 Sep 25, 2022

BrayanDSO approved these changes Sep 25, 2022

View reviewed changes

mikehardy requested changes Oct 13, 2022

View reviewed changes

AnkiDroid/src/main/res/values/10-preferences.xml Outdated Show resolved Hide resolved

mikehardy added the Needs Author Reply Waiting for a reply from the original author label Oct 13, 2022

oakkitten force-pushed the custom-sync-server branch from 08fa835 to 99943f6 Compare October 15, 2022 20:05

BrayanDSO self-requested a review October 15, 2022 20:29

BrayanDSO requested changes Oct 15, 2022

View reviewed changes

BrayanDSO removed the Needs Second Approval Has one approval, one more approval to merge label Oct 15, 2022

oakkitten force-pushed the custom-sync-server branch from 99943f6 to e4ab1a2 Compare October 15, 2022 22:24

BrayanDSO approved these changes Oct 15, 2022

View reviewed changes

oakkitten force-pushed the custom-sync-server branch from e4ab1a2 to 5574dc2 Compare October 16, 2022 00:25

BrayanDSO removed the Needs Author Reply Waiting for a reply from the original author label Oct 16, 2022

mikehardy added Needs Author Reply Waiting for a reply from the original author and removed Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) labels Oct 17, 2022

oakkitten force-pushed the custom-sync-server branch from 5574dc2 to 0292199 Compare October 17, 2022 09:29

mikehardy added Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) and removed Needs Author Reply Waiting for a reply from the original author labels Oct 17, 2022

mikehardy mentioned this pull request Oct 17, 2022

Updated strings from Crowdin 20221017-1 #12689

Merged

mikehardy requested changes Oct 17, 2022

View reviewed changes

mikehardy added the Needs Author Reply Waiting for a reply from the original author label Oct 17, 2022

oakkitten added 6 commits October 17, 2022 19:36

Add & use DialogFragmentProvider

9b1289c

DialogFragmentProvider is an interface that slightly simplifies finding relations between the preferences that display dialogs on click, and the dialogs themselves.

oakkitten force-pushed the custom-sync-server branch from 0292199 to 84ce399 Compare October 17, 2022 18:43

mikehardy approved these changes Oct 17, 2022

View reviewed changes

mikehardy merged commit d18cba8 into ankidroid:main Oct 17, 2022

github-actions bot added this to the 2.16 release milestone Oct 17, 2022

github-actions bot removed Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) squash-merge The pull request currently requires maintainers to "Squash Merge" Needs Author Reply Waiting for a reply from the original author labels Oct 17, 2022

oakkitten mentioned this pull request Oct 25, 2022

Add documentation for custom sync server preference screen ankidroid/ankidroiddocs#104

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make custom sync server preferences easier to understand #12367

Make custom sync server preferences easier to understand #12367

oakkitten commented Sep 10, 2022 •

edited

Loading

github-actions bot commented Sep 10, 2022

BrayanDSO left a comment

BrayanDSO Sep 10, 2022

oakkitten Sep 10, 2022

BrayanDSO Sep 10, 2022

oakkitten Sep 10, 2022

viciousAegis Sep 25, 2022

oakkitten Sep 25, 2022

BrayanDSO commented Sep 24, 2022

oakkitten commented Sep 25, 2022

mikehardy left a comment

mikehardy commented Oct 13, 2022

oakkitten commented Oct 15, 2022 •

edited

Loading

BrayanDSO left a comment •

edited

Loading

oakkitten commented Oct 15, 2022

BrayanDSO left a comment •

edited

Loading

mikehardy commented Oct 17, 2022

oakkitten commented Oct 17, 2022

mikehardy commented Oct 17, 2022

mikehardy left a comment

oakkitten commented Oct 17, 2022 •

edited

Loading

mikehardy commented Oct 17, 2022

oakkitten commented Oct 17, 2022

mikehardy left a comment

mikehardy commented Oct 17, 2022

github-actions bot commented Nov 1, 2022

Make custom sync server preferences easier to understand #12367

Make custom sync server preferences easier to understand #12367

Conversation

oakkitten commented Sep 10, 2022 • edited Loading

How Has This Been Tested?

Checklist

github-actions bot commented Sep 10, 2022

BrayanDSO left a comment

Choose a reason for hiding this comment

BrayanDSO Sep 10, 2022

Choose a reason for hiding this comment

oakkitten Sep 10, 2022

Choose a reason for hiding this comment

BrayanDSO Sep 10, 2022

Choose a reason for hiding this comment

oakkitten Sep 10, 2022

Choose a reason for hiding this comment

viciousAegis Sep 25, 2022

Choose a reason for hiding this comment

oakkitten Sep 25, 2022

Choose a reason for hiding this comment

BrayanDSO commented Sep 24, 2022

oakkitten commented Sep 25, 2022

mikehardy left a comment

Choose a reason for hiding this comment

mikehardy commented Oct 13, 2022

oakkitten commented Oct 15, 2022 • edited Loading

BrayanDSO left a comment • edited Loading

Choose a reason for hiding this comment

oakkitten commented Oct 15, 2022

BrayanDSO left a comment • edited Loading

Choose a reason for hiding this comment

mikehardy commented Oct 17, 2022

oakkitten commented Oct 17, 2022

mikehardy commented Oct 17, 2022

mikehardy left a comment

Choose a reason for hiding this comment

oakkitten commented Oct 17, 2022 • edited Loading

mikehardy commented Oct 17, 2022

oakkitten commented Oct 17, 2022

mikehardy left a comment

Choose a reason for hiding this comment

mikehardy commented Oct 17, 2022

github-actions bot commented Nov 1, 2022

oakkitten commented Sep 10, 2022 •

edited

Loading

oakkitten commented Oct 15, 2022 •

edited

Loading

BrayanDSO left a comment •

edited

Loading

BrayanDSO left a comment •

edited

Loading

oakkitten commented Oct 17, 2022 •

edited

Loading