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

refactor(android): merge layout JS calls into a single call #11734

Open
jahorton opened this issue Jun 7, 2024 · 1 comment · May be fixed by #11828
Open

refactor(android): merge layout JS calls into a single call #11734

jahorton opened this issue Jun 7, 2024 · 1 comment · May be fixed by #11828
Assignees
Milestone

Comments

@jahorton
Copy link
Contributor

jahorton commented Jun 7, 2024

[...] It seems like part of the problem here is the existing spaghetti is making it very difficult to reason about the orientation state.

One simple refactor I think we should try and apply is to reduce the number of loadJavascript calls:

  • Each call is costly
  • If we have a setDimensions function which is passed an object, then we can set as many of the dimensions as are passed in, in the specific order that they should be set, and without having to bubble that ordering detail all the up into the Java code.
  • It helps us see where we are only setting width or height or vice-versa.

Originally posted by @mcdurdin in #11722 (review)

This smells like it should be a single function call, setDimensions({bannerHeight:%d,oskWidth:%d,oskHeight:%d}).

This may also be relevant:

public void onResume() {
DisplayMetrics dms = context.getResources().getDisplayMetrics();
int kbWidth = (int) (dms.widthPixels / dms.density);
// Ensure window is loaded for javascript functions
loadJavascript(KMString.format(
"window.onload = function(){ setOskWidth(\"%d\");"+
"setOskHeight(\"0\"); };", kbWidth));
if (this.getShouldShowHelpBubble()) {
this.showHelpBubbleAfterDelay(2000);
}
}

Calls to these functions (on master branch):

  • Only 1 call to setBannerHeight, right here.
  • 4 calls to setOskHeight
  • 2 calls to setOskWidth

Originally posted by @mcdurdin in #11722 (comment)

@jahorton jahorton added this to the A18S4 milestone Jun 7, 2024
@jahorton jahorton self-assigned this Jun 7, 2024
@jahorton
Copy link
Contributor Author

jahorton commented Jun 7, 2024

  • Only 1 call to setBannerHeight, right here.

"Here" being:

loadJavascript(KMString.format("setBannerHeight(%d)", bannerHeight));
loadJavascript(KMString.format("setOskWidth(%d)", width));
// Must be last - it's the one that triggers a Web-engine layout refresh.
loadJavascript(KMString.format("setOskHeight(%d)", oskHeight));

This was part of KMKeyboard.onConfigurationChanged before #11722, which spins it off into onSizeChanged, which does (also) override an existing built-in View method. ("Also" because onConfigurationChanged does too.)

  • 4 calls to setOskHeight
  • Same code block as seen under setBannerHeight
  • KMKeyboard.onResume, as noted in the description
    • The intent of that code, re: height, is to reuse the previously-set height and force the internal Web engine to re-layout the OSK.
  • public static void applyKeyboardHeight(Context context, int height) {
    if (isKeyboardLoaded(KeyboardType.KEYBOARD_TYPE_INAPP)) {
    InAppKeyboard.loadJavascript(KMString.format("setOskHeight('%s')", height));
    RelativeLayout.LayoutParams params = getKeyboardLayoutParams();
    InAppKeyboard.setLayoutParams(params);
    }
    if (isKeyboardLoaded(KeyboardType.KEYBOARD_TYPE_SYSTEM)) {
    SystemKeyboard.loadJavascript(KMString.format("setOskHeight('%s')", height));
    RelativeLayout.LayoutParams params = getKeyboardLayoutParams();
    SystemKeyboard.setLayoutParams(params);
    }
    }
  • 2 calls to setOskWidth
  • Same code block as seen under setBannerHeight
  • KMKeyboard.onResume, as noted in the description

I feel as if onResume is always paired with an onConfigurationChanged call. If it's possible to determine this to be true by investigation, we can probably drop onResume entirely in favor of onConfigurationChanged, passing all keyboard-dimension synchronization through the single code block seen under setBannerHeight's entry above.

If I'm wrong about that, worst-case, we can probably just .setLayoutParams to trigger a relayout, then use onSizeChanged from #11722 to set the dimensions - also from a single, central location.

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

Successfully merging a pull request may close this issue.

2 participants