Ability to reorder popup characters. #1921

ArenaL5 · 2019-12-19T16:08:02Z

This pull request is mainly to close the issue discussed at the end of AnySoftKeyboard/LanguagePack#231. This will immediately fix #1681 and, with time, several issues in both LanguagePack and AnySoftKeyboard repos.

It's... underwhelming, because @martholomew had the perfect interface for configuring this:

These would all be re-orderable checkboxes ([edit] not sure the exact name, but its the thing that looks like this) in settings.
I think that this would solve all sorts of problems people have.

But I realized I don't have the ability to implement that. The closest preference class in the Android libraries is MultiSelectListPreference, but that doesn't save the order of the checked elements. Writing event listeners for Android is already complex, and the drag-and-drop mechanism that @martholomew describes is so complex that several Maven libraries exist to implement it. And even then, we'd still have to add the checkboxes.

As I had already work half-done and I'd rather not delay this upwards of month, I implemented a very basic EditTextPreference, as seen in commit c7ba078. You can configure it from UI → Tweaks and more → Order of popup characters. I hope that this preference will be replaced for a more user-friendly one.

The instructions are very short but I think you devs will understand it pretty easily. The preference value is not case-sensitive, characters other than N, L, O, S have no effect, an empty string will disable popupCharacters altogether, and any duplicate character that would remain in the popup after reordering them all is discarded.

Besides all that, there's also fixes for some other issues, as you can see in the commit summaries (#1914, #1418, #1662) and a small addition to README.md.

codecov · 2019-12-19T16:20:21Z

Codecov Report

Merging #1921 (a0adb61) into master (28bf659) will increase coverage by 0.05%.
The diff coverage is 89.65%.

@@             Coverage Diff              @@
##             master    #1921      +/-   ##
============================================
+ Coverage     65.10%   65.15%   +0.05%     
- Complexity     3004     3023      +19     
============================================
  Files           237      237              
  Lines         15863    15903      +40     
  Branches       2087     2101      +14     
============================================
+ Hits          10327    10362      +35     
  Misses         4776     4776              
- Partials        760      765       +5

Impacted Files	Coverage Δ	Complexity Δ
...ava/com/anysoftkeyboard/keyboards/AnyKeyboard.java	`90.57% <ø> (-0.10%)`	`108.00 <0.00> (-2.00)`
...om/anysoftkeyboard/keyboards/AnyPopupKeyboard.java	`87.27% <75.00%> (+1.43%)`	`24.00 <2.00> (+1.00)`
...yboards/views/PopupKeyboardPositionCalculator.java	`80.76% <77.77%> (-8.12%)`	`5.00 <0.00> (+2.00)`	⬇️
...anysoftkeyboard/keyboards/ExternalAnyKeyboard.java	`73.21% <92.77%> (+0.92%)`	`74.00 <0.00> (+14.00)`
...om/anysoftkeyboard/ime/AnySoftKeyboardRxPrefs.java	`95.69% <100.00%> (+0.09%)`	`23.00 <0.00> (+1.00)`
...n/java/com/anysoftkeyboard/keyboards/Keyboard.java	`84.65% <100.00%> (-0.18%)`	`62.00 <0.00> (ø)`
...yboards/views/AnyKeyboardViewWithMiniKeyboard.java	`91.72% <100.00%> (+0.12%)`	`41.00 <2.00> (+2.00)`
...nysoftkeyboard/ui/tutorials/VersionChangeLogs.java	`98.86% <100.00%> (+0.02%)`	`1.00 <0.00> (ø)`
... and 1 more

ArenaL5 · 2019-12-19T20:46:22Z

Sorry, I always forget to use the automated tests.

The entirety of testShortPressWhenNoPrimaryKeyAndPopupCharactersShouldShowPopupWindow is throwing assertion errors because I broke something (I suppose with my first commit) and I hadn't noticed while testing it manually because it's not really a common occurence: a key with popupCharacters but no keycode.

I'll try to fix it soon.

ArenaL5 · 2020-01-04T11:33:35Z

I don't know if I made it clear, this works now. I fixed the bug I created with my last commit.

Is there anything left to fix before merging this?

menny · 2020-02-13T20:02:10Z

@ArenaL5 would you mind trying to rebase this?

ArenaL5 · 2020-02-14T17:06:02Z

No problem!

EDIT: Please wait a bit, somehow I lost an edit during rebase.

ArenaL5 · 2020-02-14T19:32:54Z

This should do it. I've done all the automated tests and I tested it manually. ./scripts/ci/ci_check.sh throws an error because it finds duplicate IDs I didn't edit. (for dictionaries and keyboards)

Everything else is okay.

UPDATE FROM 8TH OF MARCH: What the hell, I just clicked a button to merge changes from master. ...good for me. ¯_(ツ)_/¯

nicoursi · 2020-03-23T18:51:14Z

-------------------------edit----------------------------------
I made some more tests, and I think that it's not working at all. It doesn't matter how I change those letters order, the popups always appear at the same order. I don't know if this is device specific or if something got broken by this: Merge branch 'master' into patches
-------------------------edit----------------------------------

Hi! I was curious about this commit, so I compiled it to test it on my device. I was surprised to see how this commit breaks a symbols layout.

As an example, try to use qwerty with symbols. You'll notice that none of those symbols designed for that layout will be available in the supposed order. I tried to change all the combinations with the letters NLOS, but there was no way that layout could be useful to me any more.
I don't know if I just didn't grasp its functioning or if this is really what this fix does.

If I may give you my opinion, I think L, or a new letter, should leave the active layout as it is. And then the user can play with the other letters if he wants to change it.

I notice, though, that it doesn't afffect long press popups like this one:

<Key android:codes="101" android:popupKeyboard="@xml/popup_qwerty_e"/>

So, if this PL is going to be merged, I hope there'll be a letter to preserve the current layout popup caracters order ;)

ArenaL5 · 2020-03-26T18:53:15Z

Sorry for the late reply.

popupKeyboard are custom layouts which can even have pre-recorded phrases, and my code can't analyze them, so they stay as they are.

My patch is only supposed to reorder symbols in popupCharacters, and no, I didn't think of adding an option to leave them as they are. Did you intend to have symbols at both sides of the pop-up or something?

About it not working, it's possible, I haven't tested it on a long time. I'll rebase this branch and try to fix it. Thank you! I'll let you know when I know something.

menny

Great! I love it.
A few comments and also, please, add some unit-tests.
And add an entry in the change-log here and here.

ime/app/src/main/java/com/anysoftkeyboard/keyboards/AnyKeyboard.java

ime/app/src/main/java/com/anysoftkeyboard/keyboards/ExternalAnyKeyboard.java

ime/app/src/main/res/values-es/strings.xml

nicoursi · 2020-03-27T09:23:54Z

Sorry for the late reply.

popupKeyboard are custom layouts which can even have pre-recorded phrases, and my code can't analyze them, so they stay as they are.

My patch is only supposed to reorder symbols in popupCharacters, and no, I didn't think of adding an option to leave them as they are. Did you intend to have symbols at both sides of the pop-up or something?

About it not working, it's possible, I haven't tested it on a long time. I'll rebase this branch and try to fix it. Thank you! I'll let you know when I know something.

Thanks for answering back. I tested it again after rebasing.

About not working. I found out it does indeed work but changes in the string do not happen straight away. They take effect after the app is cleared from recents. This can be very confusing.

If someone uses some of the layouts with symbols (eg. QWERTY with symbols), after an ASK update (including this PR) he won't have easy access to those symbols on long press. And changing the settings won't work unless he clears everything in recents.

With NSLO I get most of the keys right except:

E: supposed to give 3 but gives ₃
T: supposed to give 5 but gives ț
U: supposed to give 7 but gives ű
C: supposed to give - but gives ĉ

I hope you find this discussion useful, otherwise just let me know and I'll leave you alone with your tests 🙂

ArenaL5 · 2020-03-27T11:46:18Z

@menny. Thanks for the compliment but I didn't do anything yet. I just rebased again to clean up the history. 😆 I'll try to fix those issues soon.

@nicoursi: It was useful, actually. I did notice that the keyboard retained its old settings for a while, but I didn't know what to do to load the new ones.

Options programmed for other people do take effect straight away, so there's probably a line of code to flush the cached keyboard. I'll investigate.

menny · 2020-03-27T18:15:46Z

Yes, there is a way to flush the cache. Look for other places where we react to preference changes (in the AnySoftKeyboard* classes)

This commits also fixes a style issue at AnyKeyboardViewWithMiniKeyboard.java, and squashes two very minor bugs: - The setting for popupCharacters order was still forcibly made uppercase, interfering with the 1-character groups; - A key in a test at ExternalAnyKeyboardTest.java was not correctly checked.

Replacing info_footer.xml with actual code for an info footer that's compliant with https://developer.android.com/reference/android/preference/EditTextPreference would be appreciated.

ArenaL5 · 2020-12-25T23:50:20Z

Okay, I've been using this branch for a couple days and I see no hint of memory leakage or any breaking. I think it's good to go.

@menny: Merging this branch will change how popupCharacters are laid out, as discussed earlier, but you have the final word on this.

Currently, for keys on the left half on the keyboard, symbols in popupCharacters (after reordering) will be laid out in columns, bottom to top, then left to right.

3 or less characters: 1 2 3

4-12 characters:      2  4  6  8 10 12
                      1  3  5  7  9 11

13-18 characters:     3  6  9 12 15 18
                      2  5  8 11 14 17
                      1  4  7 10 13 16

For keys on the right hand, the rules are the same but the result is mirrored. If you feel another order is better or want to add a user preference just in case, please say so. If you find no problems, merge away!

Merry Christmas. 🎄

menny

Another question: how does the reverse value affect the currency XMLs in the app? And did you do a quick survey of if/how this will affect XMLs in various language-packs?

menny · 2020-12-29T18:17:19Z

ime/app/src/main/java/com/anysoftkeyboard/ime/AnySoftKeyboardRxPrefs.java

@@ -147,6 +147,8 @@ protected void onSharedPreferenceChange(String key) {
                || key.startsWith(KeyboardExtensionFactory.BOTTOM_ROW_PREF_ID_PREFIX)
                || key.startsWith(KeyboardExtensionFactory.TOP_ROW_PREF_ID_PREFIX)) {
            onAddOnsCriticalChange();
+        } else if (key.equals("settings_key_popup_characters_order")) {
+            onAddOnsCriticalChange();


Any chance to add a unit-test for this?
Basically, change this setting value and see that the cached keyboards were re-created.

Please use the R.string.settings_key_popup_characters_order in the test. It is fine to use this string here in the keyboard code, but in tests we should use the value from the resources.

Is this enough? ad47846

ime/app/src/main/java/com/anysoftkeyboard/keyboards/AnyKeyboard.java

menny · 2020-12-29T18:30:03Z

ime/app/src/main/java/com/anysoftkeyboard/keyboards/AnyPopupKeyboard.java

+
+        if (count <= 3) return 1;
+        if (count <= 12) return 2;
+        return 3;


It's strange that we do not have a unit-test for this.
Maybe in the allAddons variant?

testCodesParsing() at ExternalAnyKeyboardTest.java tests it indirectly.

I could add a test-unit to test this directly, but how do I test a private method?

ime/app/src/main/java/com/anysoftkeyboard/keyboards/ExternalAnyKeyboard.java

ime/app/src/main/res/values-en/strings.xml

menny · 2020-12-30T16:48:52Z

ime/app/src/main/res/values/settings_keys_dont_translate.xml

@@ -374,6 +374,8 @@

    <string name="settings_key_allow_layouts_to_provide_generic_rows">settings_key_allow_layouts_to_provide_generic_rows</string>
    <string name="settings_key_apply_remote_app_colors">settings_key_apply_remote_app_colors</string>
+    <string name="settings_key_popup_characters_order">settings_key_popup_characters_order</string>
+    <string name="settings_key_popup_characters_order_default">NLOS</string>


the default value should be in settings_defaults_dont_translate.xml

It is there. The default value is NLOS, line 378.

Is there any other settings_defaults_dont_translate.xml?

menny · 2020-12-30T16:49:35Z

ime/app/src/main/res/values/strings.xml

@@ -485,6 +485,16 @@
    <string name="bottom_generic_row_dialog_title">Select row type</string>
    <string name="bottom_generic_row_summary">Selected bottom row: %s</string>

+    <string name="popup_characters_order_title">Characters shown upon long press</string>
+    <string name="popup_characters_order_summary">Order is important. Uppercase letters represent different characters for every layout:\n


How does this long explanation look in the UI? Can you share a screenshot?

Sure, here. Please excuse the Spanglish.

I don't remember the exact details, but I had to tweak a template for an info section in XML. It must be at 380af81 (“Add better instructions for the characters reordering”).

menny · 2020-12-30T16:51:44Z

Overall, this is a great change!

ArenaL5 · 2021-01-07T17:16:02Z

Thank you!

Sorry I've been a little disappeared lately. I'll push new commits to fulfill your requests. It should take a few days to close them all.

This commit will be squashed later.

The only difference between the Spanish and the default symbols layout is an interrobang. There should probably not be a Spanish version for the common symbols layout anyways.

…boardRxPrefs.java#L151

ArenaL5 · 2021-01-11T22:29:16Z

Answering your first question (which I didn't see at the moment):

Another question: how does the reverse value affect the currency XMLs in the app? And did you do a quick survey of if/how this will affect XMLs in various language-packs?

I had only tried it with the Spanish layout, which only defines the main layout and popupCharacters (it doesn't include XML files for static popups).

I'm testing now other keyboards:

the Brazilian layout with no Ç or ' key: it reverses the popup for U, but this is consistent with the popups created from popupCharacters and this code)
emoji keyboards: it reverses the popup for some emojis, but this is confusing because the race selection switches directions for ~40% of the emojis.

I will push an extra commit to fix the emoji problem and any other layout my code breaks.

Only two files were changed by hand in this commit: - buildSrc/src/main/java/emoji/EmojiKeyboardCreator.java - gradle/emoji_generator.gradle Every other change in this commit is the result of running ./gradlew makeEmojiKeyboards. ime/app/src/main/res/xml-v27/quick_text_unicode_uncollected.xml deleted after running that command, as it was unused.

ArenaL5 · 2021-01-12T23:52:24Z

I made more tests; apparently these commits won't break anything. The currency key in the main symbols keyboard is a normal key with popupCharacters on the left hand of the keyboard, so it doesn't even get mirrored. The main symbol is the € sign for me.

The keyboards using predefined popups (most if not all of them could be changed to popupCharacters) appear correctly. Popups opened on the right half are mirrored exactly as they're supposed to do, no matter if they're defined as XML or built up from popupCharacters:

Unless you remember some other popup that should not be mirrored (emojis were already taken care of), this looks good. Maybe the 16-keys keyboards, but the popups in that were already faulty (too uneven and often too wide). I propose myself to convert them to popupCharacters in a different PR; if you have a better idea, please tell me so.

I think I addressed everything now. If you notice something wrong, please tell me; if you're okay with merging this, warn me first so I can clean up the history.

ArenaL5 mentioned this pull request Dec 19, 2019

[FEATURE REQUEST] Keyboard layout independent of language #1590

Closed

menny added this to the 1.10-r4 milestone Dec 27, 2019

This was referenced Jan 15, 2020

Added Polish AnySoftKeyboard/LanguagePack#342

Merged

English language pack popup characters are not sane AnySoftKeyboard/LanguagePack#337

Closed

menny force-pushed the master branch 5 times, most recently from bf7635c to 6edc8f1 Compare February 7, 2020 20:45

menny mentioned this pull request Feb 14, 2020

Subkeys / second keys in Dutch Language pack #1668

Open

ArenaL5 force-pushed the patches branch from 4d2b5c9 to 11d0ea7 Compare February 14, 2020 18:31

ArenaL5 requested review from a team as code owners March 8, 2020 16:59

menny modified the milestones: 1.10-r4, 1.10-r5 Mar 24, 2020

ArenaL5 force-pushed the patches branch from 49aed10 to 2e548ec Compare March 26, 2020 20:04

menny requested changes Mar 27, 2020

View reviewed changes

ime/app/src/main/java/com/anysoftkeyboard/keyboards/AnyKeyboard.java Outdated Show resolved Hide resolved

ime/app/src/main/java/com/anysoftkeyboard/keyboards/ExternalAnyKeyboard.java Show resolved Hide resolved

ime/app/src/main/res/values-es/strings.xml Outdated Show resolved Hide resolved

ArenaL5 force-pushed the patches branch from 2e548ec to 087683e Compare March 29, 2020 14:57

ArenaL5 requested a review from a team as a code owner March 29, 2020 14:57

ArenaL5 added 4 commits December 23, 2020 19:23

Add better instructions for the characters reordering

380af81

Replacing info_footer.xml with actual code for an info footer that's compliant with https://developer.android.com/reference/android/preference/EditTextPreference would be appreciated.

Fix test suite

d44cebc

private final String mPopupCharactersOrder

c16f2d5

ArenaL5 force-pushed the patches branch from 7c70c6b to c16f2d5 Compare December 23, 2020 18:23

menny requested changes Dec 30, 2020

View reviewed changes

ArenaL5 added 3 commits January 7, 2021 18:19

Remove extraneous execution flag

3e71793

Reminder not to change values-en/strings.xml

ece84f8

This commit will be squashed later.

Reminder to rephrase comment in AnyKeyboard.java

b728ef7

ArenaL5 force-pushed the patches branch from 3ef6c68 to d4869d2 Compare January 11, 2021 21:36

ArenaL5 added 2 commits January 11, 2021 22:55

Remove complexity

ceb9748

The only difference between the Spanish and the default symbols layout is an interrobang. There should probably not be a Spanish version for the common symbols layout anyways.

Coverage for ime/app/src/main/java/com/anysoftkeyboard/ime/AnySoftKey…

ad47846

…boardRxPrefs.java#L151

ArenaL5 force-pushed the patches branch from d4869d2 to ad47846 Compare January 11, 2021 21:56

ArenaL5 force-pushed the patches branch from 65dc666 to aab7401 Compare January 12, 2021 16:05

Re-trigger Github Actions

a0adb61

menny force-pushed the master branch 5 times, most recently from 2999072 to ebc9b44 Compare February 10, 2021 16:39

menny modified the milestones: 1.11-r1, 1.11-r2 Jun 22, 2021

menny modified the milestones: 1.11-r2, 1.11-r3 Apr 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ability to reorder popup characters. #1921

Ability to reorder popup characters. #1921

ArenaL5 commented Dec 19, 2019

codecov bot commented Dec 19, 2019 •

edited

ArenaL5 commented Dec 19, 2019

ArenaL5 commented Jan 4, 2020

menny commented Feb 13, 2020

ArenaL5 commented Feb 14, 2020 •

edited

ArenaL5 commented Feb 14, 2020 •

edited

nicoursi commented Mar 23, 2020 •

edited

ArenaL5 commented Mar 26, 2020

menny left a comment •

edited

nicoursi commented Mar 27, 2020 •

edited

ArenaL5 commented Mar 27, 2020 •

edited

menny commented Mar 27, 2020

ArenaL5 commented Dec 25, 2020

menny left a comment

menny Dec 29, 2020

menny Dec 29, 2020

ArenaL5 Jan 11, 2021

menny Dec 29, 2020

ArenaL5 Jan 11, 2021

menny Dec 30, 2020

ArenaL5 Jan 7, 2021

menny Dec 30, 2020

ArenaL5 Jan 7, 2021

menny commented Dec 30, 2020

ArenaL5 commented Jan 7, 2021

ArenaL5 commented Jan 11, 2021

ArenaL5 commented Jan 12, 2021

Ability to reorder popup characters. #1921

Are you sure you want to change the base?

Ability to reorder popup characters. #1921

Conversation

ArenaL5 commented Dec 19, 2019

codecov bot commented Dec 19, 2019 • edited

Codecov Report

ArenaL5 commented Dec 19, 2019

ArenaL5 commented Jan 4, 2020

menny commented Feb 13, 2020

ArenaL5 commented Feb 14, 2020 • edited

ArenaL5 commented Feb 14, 2020 • edited

nicoursi commented Mar 23, 2020 • edited

ArenaL5 commented Mar 26, 2020

menny left a comment • edited

Choose a reason for hiding this comment

nicoursi commented Mar 27, 2020 • edited

ArenaL5 commented Mar 27, 2020 • edited

menny commented Mar 27, 2020

ArenaL5 commented Dec 25, 2020

menny 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

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

menny commented Dec 30, 2020

ArenaL5 commented Jan 7, 2021

ArenaL5 commented Jan 11, 2021

ArenaL5 commented Jan 12, 2021

codecov bot commented Dec 19, 2019 •

edited

ArenaL5 commented Feb 14, 2020 •

edited

ArenaL5 commented Feb 14, 2020 •

edited

nicoursi commented Mar 23, 2020 •

edited

menny left a comment •

edited

nicoursi commented Mar 27, 2020 •

edited

ArenaL5 commented Mar 27, 2020 •

edited