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.

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

fix(android): Always display HTML banner when suggestions aren't available #9696

Merged
merged 53 commits into from
Nov 17, 2023
Merged
Show file tree
Hide file tree
Changes from 44 commits
Commits
Show all changes
53 commits
Select commit Hold shift + click to select a range
4273140
change(android): Add image banner
darcywong00 Sep 19, 2023
9b06591
fix(android/engine): Try green banner
darcywong00 Sep 25, 2023
d3872ee
fix(web): Start with image banner
darcywong00 Sep 25, 2023
3a785f5
chore(android/app): Use white banner theme
darcywong00 Sep 25, 2023
b46d50f
chore(web): Revert initial banner
darcywong00 Sep 26, 2023
3df2668
refactor(android/engine): Move constants to KMManager
darcywong00 Sep 26, 2023
131180c
refactor(android): Change set/getBanner
darcywong00 Sep 26, 2023
db728df
fix(android): Set image path
darcywong00 Sep 26, 2023
b91d651
chore(web): Default to image banner on Android
darcywong00 Sep 27, 2023
7d3de5a
fix(web): Handle banner initialization
darcywong00 Oct 2, 2023
018abd4
fix(android/engine): Use white and Keyman theme banners
darcywong00 Oct 6, 2023
a5aef7a
fix(web): Update image path when setting Options
darcywong00 Oct 6, 2023
510de72
fix(android/engine): Remove refresh that wasn't working
darcywong00 Oct 6, 2023
d366f33
chore(web): Add Android tablet theme
darcywong00 Oct 6, 2023
b45c61e
chore(android): Merge remote-tracking branch 'origin/master' into fix…
darcywong00 Oct 16, 2023
8a48fd2
chore(android): Merge remote-tracking branch 'origin/master' into fix…
darcywong00 Oct 26, 2023
bb5a9f8
chore(android/engine): Move banner names
darcywong00 Oct 26, 2023
a6b7b95
feat(oem/fv/android): Add banner theme
darcywong00 Oct 26, 2023
926a2a2
feat(android/samples): Add sample code for theme on KMSample2
darcywong00 Oct 26, 2023
6247a52
chore(android): Merge remote-tracking branch 'origin/master' into fix…
darcywong00 Nov 6, 2023
8630121
fix(android/engine): Replace png's with svg's
darcywong00 Nov 7, 2023
1e30002
chore(android): Cleanup
darcywong00 Nov 7, 2023
6535acd
chore(android/app): Adjust svg dimensions
darcywong00 Nov 7, 2023
6593ae2
chore(android): Merge remote-tracking branch 'origin/master' into fix…
darcywong00 Nov 9, 2023
11af550
fix(android/engine): Try more svg
darcywong00 Nov 9, 2023
1d40beb
fix(android/engine): Get svg to fill banner
darcywong00 Nov 9, 2023
8961e9d
fix(android/app): Revert keyman_banner.svg
darcywong00 Nov 13, 2023
6170e04
refactor(android/engine): Pass contents of banner.html to KMW
darcywong00 Nov 13, 2023
c619873
chore(android): Merge remote-tracking branch 'origin/refactor/web/ina…
darcywong00 Nov 14, 2023
cb431fe
refactor(android): Use showBanner
darcywong00 Nov 14, 2023
879129e
chore(android): Merge remote-tracking branch 'origin/refactor/web/ina…
darcywong00 Nov 14, 2023
263da40
fix(android/engine): Handle mayPredict
darcywong00 Nov 14, 2023
712b2a2
refactor(android): Move banner logic to app
darcywong00 Nov 14, 2023
856ed13
fix(android/app): Always use Keyman theme
darcywong00 Nov 14, 2023
830c282
chore(android): Cleanup code
darcywong00 Nov 14, 2023
27480eb
chore(android): Additional cleanup
darcywong00 Nov 14, 2023
095b823
chore(android): More cleanup
darcywong00 Nov 14, 2023
c83410a
fix(oem/fv/android): Apply teal banner theme
darcywong00 Nov 15, 2023
d6a14a8
fix(oem/fv/android): Tweak FV banner theme
darcywong00 Nov 15, 2023
35a37bc
chore(android): Additional cleanup
darcywong00 Nov 15, 2023
76eab6e
fix(android/engine): Don't set banner on test mode
darcywong00 Nov 15, 2023
0bcdea1
chore(android): Merge remote-tracking branch 'origin/master' into fix…
darcywong00 Nov 16, 2023
887e6b2
chore(web): Revert 2 files to master
darcywong00 Nov 16, 2023
68f2661
chore(web): Revert take 2
darcywong00 Nov 16, 2023
44fa59b
chore(android/engine): Revert some changes in android-host.js
darcywong00 Nov 16, 2023
8249abd
fix(android): Handle single quotes in HTMl banner
darcywong00 Nov 16, 2023
37e9cf5
fix(android/app): Move banner asset folder
darcywong00 Nov 16, 2023
37e9bbd
fix(oem/fv/android): Move banner asset folder
darcywong00 Nov 16, 2023
b41f049
chore(android): Tweak banner sizing
darcywong00 Nov 16, 2023
2b34c64
Apply suggestions from code review
darcywong00 Nov 16, 2023
4c453cc
chore(android): Move banner.html up to assets/ folder
darcywong00 Nov 16, 2023
d77df39
fix(android): Refresh HTML banner when KeymanWeb reloads
darcywong00 Nov 17, 2023
4ebc1e2
fix(android/engine): Fix typo
darcywong00 Nov 17, 2023
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
10 changes: 10 additions & 0 deletions android/KMAPro/kMAPro/src/main/assets/svg/banner.html
Copy link
Member

Choose a reason for hiding this comment

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

  1. banner.html will go to android/KMAPro/kMAPro/src/main/assets/banner.html
  2. keyman_banner.svg will go to android/KMAPro/kMAPro/src/main/assets/banner/keyman_banner.svg
  3. the img src will be <img src="banner/keyman_banner.svg" ... in banner.html
  4. string replacement requirement goes away
  5. KMManager.copyHTMLBannerAssets(path) will call copyAsset() for each file in the path to copy it to the resources 'banner/' folder: copyAsset(context, filename, "banner/", true)
  6. KMAPro will call `KMManager.setHTMLBannerAssetsPath(getAssetsPath() + '/banner')

Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<!-- Custom banner theme -->
<div style="background: white; width: 100%; height: 100%; position: absolute; left: 0; top: 0">
<!--<div style="height: 1px; left: 0; position: absolute; top: 0; width: 100%; background: black"></div>-->
<img src="$BANNER" style="height:100%; background: white; display: block">
Copy link
Contributor

@jahorton jahorton Nov 16, 2023

Choose a reason for hiding this comment

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

I'm no CSS wizard, but...

Suggested change
<div style="background: white; width: 100%; height: 100%; position: absolute; left: 0; top: 0">
<!--<div style="height: 1px; left: 0; position: absolute; top: 0; width: 100%; background: black"></div>-->
<img src="$BANNER" style="height:100%; background: white; display: block">
<div style="background: white; width: 100%; height: 100%; position: absolute; left: 0; top: 0; box-sizing: border-box">
<!--<div style="height: 1px; left: 0; position: absolute; top: 0; width: 100%; background: black"></div>-->
<div style="width: 100%; height: 93%; position: absolute; left: 0; top: 0; box-sizing: border-box; padding: 5px>
<img src="$BANNER" style="height:100%; background: white; display: block">
</div>

Something like this should help us shrink the image.

box-sizing: border-box: ensures that addition of borders and padding counts as part of the width & height's "100%" spec - they thus subtract from the space allotted to nested contents - that SVG.

height: 93%: so that the SVG stays entirely above the tri-color line. 100% might actually be fine, though - not sure what our styling preference in that direction is. The 93% bit will also cause the text to move slightly upward instead of being perfectly vertically-centered, so do with that what you will.

Just spitballing an idea, but you may wish to replace padding: 5px with padding: $PADDING and do a replacement that's based upon the device resolution or similar, if it turns out we need different padding amounts for different devices.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, a small warning before you consider getting fancy: https://css-tricks.com/oh-hey-padding-percentage-is-based-on-the-parent-elements-width/

Padding percentage doesn't work ideally for us here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll update the user-testing screenshots with some updated styling (display: flex; and margin: auto 2%)


<!-- Tri-color line -->
<div style="height: 7%; left: 0; position: absolute; bottom: 0; width: 56%; background: #F68924"></div>
<div style="height: 7%; left: 56%; position: absolute; bottom: 0; width: 23%; background: #CC3846"></div>
<div style="height: 7%; left: 79%; position: absolute; bottom: 0; width: 21%; background: #79C3DA"></div>
</div>
372 changes: 372 additions & 0 deletions android/KMAPro/kMAPro/src/main/assets/svg/keyman_banner.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package com.keyman.android;

import android.content.Context;

import com.keyman.engine.KMManager;
import com.keyman.engine.util.FileUtils;

import java.io.File;

public class BannerController {

// Paths relative to assets folder for banner themes
public static final String KM_BANNER_THEME_KEYMAN = "svg/banner.html";
public static final String KM_BANNER_THEME_KEYMAN_SVG = "svg/keyman_banner.svg";


public static void setHTMLBanner(Context context, KMManager.KeyboardType keyboardType) {
if (keyboardType == KMManager.KeyboardType.KEYBOARD_TYPE_UNDEFINED) {
return;
}

// Always use Keyman banner theme
String contents = FileUtils.readContents(context, KM_BANNER_THEME_KEYMAN);

// If $BANNER string exists, replace with actual path
File bannerPath = new File(KMManager.getResourceRoot(), KM_BANNER_THEME_KEYMAN_SVG);
if (bannerPath.exists()) {
contents = contents.replace("$BANNER", bannerPath.getAbsolutePath());
}

KMManager.setHTMLBanner(keyboardType, contents);
KMManager.setBanner(keyboardType, KMManager.BannerType.HTML);
KMManager.showBanner(true);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

import com.tavultesoft.kmapro.BuildConfig;
import com.tavultesoft.kmapro.KeymanSettingsActivity;
import com.keyman.android.BannerController;
import com.keyman.engine.KMManager;
import com.keyman.engine.KMManager.KeyboardType;
import com.keyman.engine.KMHardwareKeyboardInterpreter;
Expand Down Expand Up @@ -71,6 +72,9 @@ public void onCreate() {
KMManager.SpacebarText spacebarText = KMManager.SpacebarText.fromString(prefs.getString(KeymanSettingsActivity.spacebarTextKey, KMManager.SpacebarText.LANGUAGE_KEYBOARD.toString()));
KMManager.setSpacebarText(spacebarText);

// Set the system keyboard HTML banner
BannerController.setHTMLBanner(this, KeyboardType.KEYBOARD_TYPE_SYSTEM);

boolean mayHaveHapticFeedback = prefs.getBoolean(KeymanSettingsActivity.hapticFeedbackKey, false);
KMManager.setHapticFeedback(mayHaveHapticFeedback);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import java.util.Map;

import com.keyman.android.CheckInstallReferrer;
import com.keyman.android.BannerController;
import com.keyman.engine.BaseActivity;
import com.keyman.engine.KMHelpFileActivity;
import com.keyman.engine.KMKeyboardDownloaderActivity;
Expand Down Expand Up @@ -499,6 +500,8 @@ public void onKeyboardChanged(String newKeyboard) {

@Override
public void onKeyboardShown() {
// Refresh banner theme
BannerController.setHTMLBanner(this, KeyboardType.KEYBOARD_TYPE_INAPP);
resizeTextView(true);
}

Expand Down
62 changes: 49 additions & 13 deletions android/KMEA/app/src/main/assets/android-host.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ if(window.parent && window.parent.jsInterface && !window.jsInterface) {
var device = window.jsInterface.getDeviceType();
var oskHeight = Math.ceil(window.jsInterface.getKeyboardHeight() / window.devicePixelRatio);
var oskWidth = 0;
var bannerHeight = 0;
var bannerImagePath = '';
var bannerHTMLContents = '';
var fragmentToggle = 0;

var sentryManager = new KeymanSentryManager({
Expand Down Expand Up @@ -37,11 +40,13 @@ function init() {
oninserttext: insertText,
root:'./'
}).then(function () { // Note: For non-upgraded API 21, arrow functions will break the keyboard!
const bannerHeight = Math.ceil(window.jsInterface.getDefaultBannerHeight() / window.devicePixelRatio);
bannerHeight = Math.ceil(window.jsInterface.getDefaultBannerHeight() / window.devicePixelRatio);
if (bannerHeight > 0) {

// The OSK is not available until initialization is complete.
keyman.osk.bannerView.activeBannerHeight = bannerHeight;
keyman.refreshOskLayout();
// The OSK is not available until initialization is complete.
keyman.osk.bannerView.activeBannerHeight = bannerHeight;
keyman.refreshOskLayout();
}
});

keyman.addEventListener('keyboardloaded', setIsChiral);
Expand All @@ -53,6 +58,29 @@ function init() {
notifyHost('pageLoaded');
}

function showBanner(flag) {
console_debug("Setting banner display for dictionaryless keyboards to " + flag);
console_debug("bannerHTMLContents: " + bannerHTMLContents);
var bc = keyman.osk.bannerController;
if (bc) {
if (bannerHTMLContents != '') {
bc.inactiveBanner = flag ? new bc.HTMLBanner(bannerHTMLContents) : null;
} else {
bc.inactiveBanner = flag ? new bc.ImageBanner(bannerImgPath) : null;
}
}
}

function setBannerImage(path) {
bannerImgPath = path;
}

// Set the HTML banner to use when predictive-text is not available
// contents - HTML content to use for the banner
function setBannerHTML(contents) {
bannerHTMLContents = contents;
}

function notifyHost(event, params) {
console_debug('notifyHost(event='+event+',params='+params+')');
// TODO: Update all other host notifications to use notifyHost instead of directly setting window.location.hash
Expand All @@ -65,13 +93,21 @@ function notifyHost(event, params) {
}

// Update the KMW banner height
// h is in dpi (different from iOS)
function setBannerHeight(h) {
if (h > 0) {
var osk = keyman.osk;
osk.banner.height = Math.ceil(h / window.devicePixelRatio);
// The banner itself may not be loaded yet. This will preemptively help set
// its eventual display height.
bannerHeight = Math.ceil(h / window.devicePixelRatio);

if (keyman.osk) {
keyman.osk.bannerView.activeBannerHeight = bannerHeight;
}
}
// Refresh KMW OSK

// Refresh KMW's OSK
keyman.refreshOskLayout();
doResetContext();
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to reset the context when adjusting the banner?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got the pattern from ios-host.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted this and the other call too

}

function setOskHeight(h) {
Expand All @@ -82,6 +118,7 @@ function setOskHeight(h) {
keyman.core.activeKeyboard.refreshLayouts();
}
keyman.refreshOskLayout();
doResetContext();
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to reset the context when adjusting the keyboard height?

}

function setOskWidth(w) {
Expand Down Expand Up @@ -168,18 +205,13 @@ function enableSuggestions(model, mayPredict, mayCorrect) {
keyman.core.languageProcessor.mayPredict = mayPredict;
keyman.core.languageProcessor.mayCorrect = mayCorrect;

registerModel(model);
keyman.addModel(model);
}

function setBannerOptions(mayPredict) {
keyman.core.languageProcessor.mayPredict = mayPredict;
}

function registerModel(model) {
//window.console.log('registerModel: ' + model);
keyman.addModel(model);
}

Copy link
Member

Choose a reason for hiding this comment

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

why is this change in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally started banner management by keeping similarity with ios-host.js

function enableSuggestions(model, mayPredict, mayCorrect) {
// Set the options first so that KMW's ModelManager can properly handle model enablement states
// the moment we actually register the new model.
keyman.core.languageProcessor.mayPredict = mayPredict;
keyman.core.languageProcessor.mayCorrect = mayCorrect;
keyman.addModel(model);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted

function resetContext() {
keyman.resetContext();
}
Expand Down Expand Up @@ -269,6 +301,10 @@ function hideKeyboard() {
window.location.hash = 'hideKeyboard' + fragmentToggle;
}

function doResetContext() {
keyman.resetContext();
}

darcywong00 marked this conversation as resolved.
Show resolved Hide resolved
function showKeyboard() {
// Refresh KMW OSK
keyman.refreshOskLayout();
Expand Down
70 changes: 35 additions & 35 deletions android/KMEA/app/src/main/java/com/keyman/engine/KMKeyboard.java
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,11 @@
import java.io.File;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;

import org.json.JSONArray;
import org.json.JSONException;
import org.json.JSONObject;

import com.keyman.engine.BaseActivity;
import com.keyman.engine.data.Keyboard;
import com.keyman.engine.data.KeyboardController;
import com.keyman.engine.KMManager.KeyboardType;
Expand All @@ -25,19 +23,11 @@
import com.keyman.engine.util.KMLog;
import com.keyman.engine.util.KMString;

import android.Manifest;
import android.annotation.SuppressLint;
import android.content.Context;
import android.content.SharedPreferences;
import android.content.pm.ApplicationInfo;
import android.content.pm.PackageManager;
import android.content.res.Configuration;
import android.graphics.Color;
import android.graphics.Rect;
import android.graphics.RectF;
import android.graphics.Typeface;
import android.os.Build;
import android.os.Bundle;
import android.os.Handler;
import android.util.DisplayMetrics;
import android.util.Log;
Expand All @@ -46,7 +36,6 @@
import android.view.LayoutInflater;
import android.view.MotionEvent;
import android.view.View;
import android.view.ViewGroup;
import android.view.WindowManager;
import android.view.inputmethod.ExtractedText;
import android.view.inputmethod.ExtractedTextRequest;
Expand All @@ -56,12 +45,9 @@
import android.webkit.WebSettings;
import android.webkit.WebView;
import android.widget.Button;
import android.widget.FrameLayout;
import android.widget.GridLayout;
import android.widget.PopupWindow;
import android.widget.PopupWindow.OnDismissListener;
import android.widget.RelativeLayout;
import android.widget.TextView;
import android.widget.Toast;

import io.sentry.Breadcrumb;
Expand All @@ -84,19 +70,10 @@ final class KMKeyboard extends WebView {

private static String currentKeyboard = null;

/**
* Banner state value: "blank" - no banner available.
*/
protected static final String KM_BANNER_STATE_BLANK = "blank";
/**
* Banner state value: "suggestion" - dictionary suggestions are shown.
*/
protected static final String KM_BANNER_STATE_SUGGESTION = "suggestion";

/**
* Current banner state.
*/
protected static String currentBanner = KM_BANNER_STATE_BLANK;
protected static KMManager.BannerType currentBanner = KMManager.BannerType.HTML;

private static String txtFont = "";
private static String oskFont = null;
Expand All @@ -105,6 +82,10 @@ final class KMKeyboard extends WebView {
private GestureDetector gestureDetector;
private static ArrayList<OnKeyboardEventListener> kbEventListeners = null;

// Stores the current html string for use by the Banner
// when predictive text is not active
protected String htmlBannerString = "";

// Facilitates a 'lazy init' - we'll only check the preference when it matters,
// rather than at construction time.
private Boolean _shouldShowHelpBubble = null;
Expand Down Expand Up @@ -400,6 +381,9 @@ public void onConfigurationChanged(Configuration newConfig) {

int bannerHeight = KMManager.getBannerHeight(context);
int oskHeight = KMManager.getKeyboardHeight(context);
if (this.htmlBannerString != null && !this.htmlBannerString.isEmpty()) {
loadJavascript(KMString.format("setBannerHTML('%s')", this.htmlBannerString));
Copy link
Member

Choose a reason for hiding this comment

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

Will this work with arbitrary HTML, even HTML that contains ' characters? i.e. are they escaped?

Copy link
Member

Choose a reason for hiding this comment

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

And shouldn't this read:

Suggested change
loadJavascript(KMString.format("setBannerHTML('%s')", this.htmlBannerString));
setHTMLBanner(this.htmlBannerString);

Copy link
Contributor

@jahorton jahorton Nov 16, 2023

Choose a reason for hiding this comment

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

This is setting up a WebView operation. Don't forget about that JS queue we set up a while back; this adds an entry to that queue (when necessary). (We are on the Java / Android side here; your suggestion would be a JS-side call.) We want the equivalent of what you've typed when within the WebView; the trick is making sure the values marshal across the WebView boundary properly in order to do so.

We'd probably be best off by using JSON.stringify encoding directly on this.htmlBannerString here, then JSON.parse() within the inner setBannerHTML function from the WebView-internal receiver.

I forget how we do the JSON.stringify bit within Java-land, but I know it's an existing pattern already in the code base.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if you are responding to my suggestion here @jahorton?

This is setting up a WebView operation. Don't forget about that JS queue we set up a while back; this adds an entry to that queue (when necessary).

See ll.387-389. those are also all calling loadJavascript too

(We are on the Java / Android side here; your suggestion would be a JS-side call.)

See ll.650-654.

Copy link
Member

@mcdurdin mcdurdin Nov 16, 2023

Choose a reason for hiding this comment

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

FWIW, all of these should be setup as a single loadJavascript() call, appending each of the functions, so we are not doing multiple callbacks on keyboard load. That should improve performance and probably reduce flicker too. (That would undo my suggestion of calling setBannerHTML but I would be okay with that)

Copy link
Contributor

@jahorton jahorton Nov 16, 2023

Choose a reason for hiding this comment

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

See #8534 - what has been noted here is a very likely contributor to that issue. If those calls are kept separately, each call is a separate async call into the WebView, and those delays could very easily contribute to layout reflow / flashing... especially since each affects the keyboard's layout.

Copy link
Member

Choose a reason for hiding this comment

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

For proper escaping, here's the basic pattern:

JSONObject reg = new JSONObject();
try {
reg.put("KN", keyboardName);
reg.put("KI", "Keyboard_" + keyboardID);
reg.put("KLC", languageID);
reg.put("KL", languageName);
reg.put("KF", keyboardPath);
reg.put("KP", packageID);
if (jDisplayFont != null) reg.put("KFont", jDisplayFont);
if (jOskFont != null) reg.put("KOskFont", jOskFont);
if (displayName != null) reg.put("displayName", displayName);
} catch(JSONException e) {
KMLog.LogException(TAG, "", e);
return false;
}
String jsString = KMString.format("setKeymanLanguage(%s)", reg.toString());
loadJavascript(jsString);

Something like:

Suggested change
loadJavascript(KMString.format("setBannerHTML('%s')", this.htmlBannerString));
loadJavascript(KMString.format("setBannerHTML(%s)",
JSONObject.quote(this.htmlBannerString)));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW, all of these should be setup as a single loadJavascript() call, appending each of the functions, so we are not doing multiple callbacks on keyboard load. That should improve performance and probably reduce flicker too. (That would undo my suggestion of calling setBannerHTML but I would be okay with that)

This will be addressed on separate PR

}
loadJavascript(KMString.format("setBannerHeight(%d)", bannerHeight));
loadJavascript(KMString.format("setOskWidth(%d)", newConfig.screenWidthDp));
loadJavascript(KMString.format("setOskHeight(%d)", oskHeight));
Expand All @@ -425,22 +409,15 @@ public static String currentKeyboard() {
return currentKeyboard;
}

public static void setCurrentBanner(String banner) {
currentBanner = banner;
}

public static String currentBanner() { return currentBanner; }

protected void toggleSuggestionBanner(HashMap<String, String> associatedLexicalModel, boolean keyboardChanged) {
//reset banner state if new language has no lexical model
if (currentBanner != null && currentBanner.equals(KM_BANNER_STATE_SUGGESTION)
if (currentBanner == KMManager.BannerType.SUGGESTION
&& associatedLexicalModel == null) {
setCurrentBanner(KMKeyboard.KM_BANNER_STATE_BLANK);
currentBanner = KMManager.BannerType.HTML;
}

if(keyboardChanged) {
setLayoutParams(KMManager.getKeyboardLayoutParams());
}
showBanner(true);
// Since there's always a banner, no need to update setLayoutParams()
}

/**
Expand Down Expand Up @@ -653,6 +630,29 @@ public boolean setKeyboard(String packageID, String keyboardID, String languageI
return retVal;
}

public void showBanner(boolean flag) {
String jsString = KMString.format("showBanner(%b)", flag);
loadJavascript(jsString);
}

public KMManager.BannerType getBanner() {
return currentBanner;
}

public void setBanner(KMManager.BannerType bannerType) {
currentBanner = bannerType;
}

public String getHTMLBanner() {
return this.htmlBannerString;
}

public void setHTMLBanner(String contents) {
this.htmlBannerString = contents;
String jsString = KMString.format("setBannerHTML('%s')", contents);
Copy link
Member

Choose a reason for hiding this comment

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

Will this work with arbitrary HTML, even HTML that contains ' characters? i.e. are they escaped?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Handled now with JSONObject.quote()

loadJavascript(jsString);
}

public void setChirality(boolean flag) {
this.isChiral = flag;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -173,8 +173,7 @@ public boolean shouldOverrideUrlLoading(WebView view, String url) {
if (KMManager.currentLexicalModel != null) {
modelPredictionPref = prefs.getBoolean(KMManager.getLanguagePredictionPreferenceKey(KMManager.currentLexicalModel.get(KMManager.KMKey_LanguageID)), true);
}
kmKeyboard.setCurrentBanner((isModelActive && modelPredictionPref) ?
KMKeyboard.KM_BANNER_STATE_SUGGESTION : KMKeyboard.KM_BANNER_STATE_BLANK);
KMManager.setBannerOptions(isModelActive && modelPredictionPref);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This tells KMW whether predictions are enabled or not

RelativeLayout.LayoutParams params = KMManager.getKeyboardLayoutParams();
kmKeyboard.setLayoutParams(params);
} else if (url.indexOf("suggestPopup") >= 0) {
Expand Down
Loading