keymanapp / keyman

Keyman cross platform input methods system running on Android, iOS, Linux, macOS, Windows and mobile and desktop web
https://keyman.com/
Other
376 stars 104 forks source link

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

Open jahorton opened 2 weeks ago

jahorton commented 2 weeks ago

[...] 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 https://github.com/keymanapp/keyman/pull/11722#pullrequestreview-2103479072

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

This may also be relevant: https://github.com/keymanapp/keyman/blob/91e91bf91b56efb6cc20ebabd4a6f0a257801af0/android/KMEA/app/src/main/java/com/keyman/engine/KMKeyboard.java#L426-L436

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 https://github.com/keymanapp/keyman/pull/11722#discussion_r1630512393_

jahorton commented 2 weeks ago
  • Only 1 call to setBannerHeight, right here.

"Here" being:

https://github.com/keymanapp/keyman/blob/d86833783d5bfdf0333f1a4d3365edbc1b8dd0dd/android/KMEA/app/src/main/java/com/keyman/engine/KMKeyboard.java#L478-L481

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
  • 2 calls to setOskWidth

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.