Skip to content

fix(web): accidental suggestion-banner retoggle when disabling predictions#10014

Merged
jahorton merged 1 commit intomasterfrom
fix/web/unwanted-banner-retoggle
Dec 7, 2023
Merged

fix(web): accidental suggestion-banner retoggle when disabling predictions#10014
jahorton merged 1 commit intomasterfrom
fix/web/unwanted-banner-retoggle

Conversation

@jahorton
Copy link
Copy Markdown
Contributor

@jahorton jahorton commented Nov 16, 2023 •
edited
Loading

While @darcywong00 was working on the Android app keyboard banner, we discovered that when predictions are turned off, but a model is already loaded, the internal model-management code would still send the configured event (from model loading) when turning predictions off. This should not happen, as that signaled the predictive-text banner to reappear.

User Testing

TEST_EUROLATIN_PREDICTIONS_OFF: Using the Keyman for Android app...

  1. Set the keyboard to English - EuroLatin (SIL).
  2. Using the app menu, navigate to Settings > Installed Languages > English.
  3. Toggle "Enable predictions" off.
  4. Leave the app menu.
  5. Verify that neither a banner or blank space for a banner is shown.

TEST_KEYBOARD_SWAP: Using the Keyman for Android app...

  1. Download the IPA (SIL) keyboard and install it.
  2. Within the Settings menu, navigate to Settings > Installed Languages > English.
  3. Toggle "Enable predictions" on.
  4. Return to the app's main screen.
  5. Verify that no predictive-text banner (nor corresponding blank space) is active.
  6. Swap to English - EuroLatin (SIL).
  7. Verify that the predictive-text banner appears, with suggestions visible.
  8. Swap to the IPA keyboard again.
  9. Verify that no predictive-text banner (nor corresponding blank space) is active.

@jahorton jahorton requested a review from mcdurdin as a code owner November 16, 2023 01:24
@keymanapp-test-bot keymanapp-test-bot bot added has-user-test user-test-required User tests have not been completed labels Nov 16, 2023
@keymanapp-test-bot
Copy link
Copy Markdown

keymanapp-test-bot bot commented Nov 16, 2023 •
edited
Loading

User Test Results

Test specification and instructions

  • ✅ TEST_EUROLATIN_PREDICTIONS_OFF (PASSED): Tested with the attached PR build Keyman 17.0.212-alpha-test-10014 in the Android Mobile (Samsung Galaxy A23 - version 13) here is my observation: 1. Verified that neither a banner nor blank space for a banner is shown in the keyboard. Seems to be working as expected. (notes)
  • ✅ TEST_KEYBOARD_SWAP (PASSED): Tested with the attached PR build Keyman 17.0.212-alpha-test-10014 in the Android Mobile (Samsung Galaxy A23 - version 13) here is my observation: 1. Verified that no predictive-text banner is active after step 4. Also, verified that after swapping to English -EuroLatin (SIL) keyboard the suggestion banner is visible on the keyboard. Again swapped to the IPA keyboard the suggestion banner disappears from the keyboard. Seems to be working as expected. (notes)

Test Artifacts

@keymanapp-test-bot keymanapp-test-bot bot added this to the A17S26 milestone Nov 16, 2023
darcywong00
darcywong00 approved these changes Nov 16, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@darcywong00 darcywong00 left a comment

Choose a reason for hiding this comment

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

lgtm

@mcdurdin
Copy link
Copy Markdown
Member

mcdurdin commented Nov 16, 2023

Should we add a user test for other predictive text actions to ensure there are no other cases where this might cause banner to not appear when it should?

@jahorton
Copy link
Copy Markdown
Contributor Author

jahorton commented Nov 16, 2023

Should we add a user test for other predictive text actions to ensure there are no other cases where this might cause banner to not appear when it should?

Like... what? I'm drawing a blank on what those "other cases" might even be.

@mcdurdin
Copy link
Copy Markdown
Member

mcdurdin commented Nov 16, 2023

Perhaps I'm stretching too far, but things like, install dictionary, uninstall dictionary, switch keyboard (with/without dictionary for keyboard/to keyboard, so 4 possible combos), start app, switch to Keyman?

@jahorton
Copy link
Copy Markdown
Contributor Author

jahorton commented Nov 16, 2023

Perhaps I'm stretching too far, but things like, install dictionary, uninstall dictionary, switch keyboard (with/without dictionary for keyboard/to keyboard, so 4 possible combos), start app, switch to Keyman?

Latter two are overkill here; we'd surely pick that up during normal regression testing in the worst case.

I went ahead and did a keyboard-swap user test; that's fairly reasonable.

Install and uninstall dictionary: I suppose that's doable, but if an issue arises there... it'd actually be a separate issue, I think? Not that it's completely unreasonable to add a fix for such a hypothetical issue to this PR, given its small size.

I'm not as familiar with the dictionary-related instruction specifics, so I'll get to that one later if @darcywong00 doesn't beat me to it.

@bharanidharanj
Copy link
Copy Markdown

bharanidharanj commented Nov 16, 2023

Test Results

  • TEST_EUROLATIN_PREDICTIONS_OFF (PASSED): Tested with the attached PR build Keyman 17.0.212-alpha-test-10014 in the Android Mobile (Samsung Galaxy A23 - version 13) here is my observation: 1. Verified that neither a banner nor blank space for a banner is shown in the keyboard. Seems to be working as expected.

  • TEST_KEYBOARD_SWAP (PASSED): Tested with the attached PR build Keyman 17.0.212-alpha-test-10014 in the Android Mobile (Samsung Galaxy A23 - version 13) here is my observation: 1. Verified that no predictive-text banner is active after step 4. Also, verified that after swapping to English -EuroLatin (SIL) keyboard the suggestion banner is visible on the keyboard. Again swapped to the IPA keyboard the suggestion banner disappears from the keyboard. Seems to be working as expected.

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Nov 16, 2023
@darcywong00 darcywong00 modified the milestones: A17S26, A17S27 Nov 27, 2023
@mcdurdin mcdurdin mentioned this pull request Dec 5, 2023
Closed
69 tasks
@jahorton
Copy link
Copy Markdown
Contributor Author

jahorton commented Dec 7, 2023

Didn't realize I hadn't merged this yet. Explains why I was still seeing the bug when checking something else out recently.

@jahorton jahorton merged commit ef901a5 into master Dec 7, 2023
@jahorton jahorton deleted the fix/web/unwanted-banner-retoggle branch December 7, 2023 02:52
@keyman-server
Copy link
Copy Markdown
Collaborator

keyman-server commented Dec 7, 2023

Changes in this pull request will be available for download in Keyman version 17.0.226-alpha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@darcywong00 darcywong00 darcywong00 approved these changes

@mcdurdin mcdurdin Awaiting requested review from mcdurdin

Assignees

No one assigned

Labels

common/web/ common/ fix has-user-test web/

Projects

None yet

Milestone

A17S27

Development

Successfully merging this pull request may close these issues.

5 participants

@jahorton @mcdurdin @bharanidharanj @keyman-server @darcywong00