search

flowkey / UIKit-cross-platform

Cross-platform Swift implementation of UIKit, mostly for Android
MIT License
594 stars 40 forks source link

simplify onNativeTouch method #368

Closed michaelknoch closed 2 years ago

michaelknoch commented 2 years ago

Type of change: bugfix

Motivation (current vs expected behavior)

Get rid of unused function parameters to maybe fix a crash in onNativeTouch function.

trace from bugsnag 

SIGTRAP Trace/breakpoint trap 
    build/arm64-v8a/<compiler-generated>:265 UIKit.onNativeTouch(env

trace from google play

#00  pc 00000000000a17b0  /data/app/com.flowkey.app-Cfv7n2a_ZlqbZd_l5StZdg==/lib/arm64/libUIKit.so ($s5UIKit13onNativeTouch3env4view13touchDeviceID013pointerFingerI06action1x1y8pressure11timestampMsySpySPySo18JNINativeInterfaceVGSgGSg_SvSgs5Int32VA2TS3fs5Int64VtF+244)
  #00  pc 0000000000162ee4  /data/app/com.flowkey.app-Cfv7n2a_ZlqbZd_l5StZdg==/oat/arm64/base.odex (art_jni_trampoline+180)

https://app.bugsnag.com/flowkey-gmbh/mobile-app/errors/61a30ddf3b2d9500071b9eea?filters[app.release_stage]=production&filters[release.seen_in][]=2.37.3&filters[release.seen_in][]=2.37.3%20(*)&filters[event.unhandled]=true

Please check if the PR fulfills these requirements

ephemer commented 2 years ago

Hey @michaelknoch I really like the idea. From memory there is potentially an arbitrary change in Swift's calling convention when a function has more than 5 arguments. With the changes here the function still takes more than 5 arguments, so I'm not sure it will make much difference as written.

Can we do this slightly differently and just pass a single object as an argument, i.e. this.onNativeTouchUIKit(parameters), where parameters is an object containing all the existing properties (reverting the change to remove some from this PR), and unwrap them via JNI's GetField function in Swift?

Alternatively we could do this a bit lower level and use a java array ([x, y, pressure, timestamp, etc]) and then access / decode the array contents in Swift via JNI GetArrayContents (forget the exact name). That would be slightly more performant but also more error prone – hopefully it isn't necessary.

michaelknoch commented 2 years ago

Makes sense @ephemer ! Seems to work with a class a0c83834be9dbc04da8a91983acbc6258206ea33. I deliberately still omitted the unused parameters because maybe the crash is just coming from one of these type castings when converting the different java types to swift types. If you are certain it can't be the cause of the crash I'm happy to add them. Otherwise I'd suggest to add them in a second iteration.

ephemer commented 2 years ago

Hey @michaelknoch, looks great! Thanks for the update. I am fairly sure the conversion is not causing the issue (since the crash appears to occur when calling the function I can't really imagine how that would happen), and if we omit the parameters now we won't know whether moving to the object structure actually fixed the issue or if it was something else. I would try a reduction in parameters as a further step if we still see the crash after this change. If it is an issue in conversions somewhere then, after this change, we'd see a crash in the GetField function rather than when calling the function, which would be really useful to know.

The only thing I'm wondering is how this goes performance-wise – I imagine it's fine, but I am also wondering whether we can cache some of the intermediate steps in getting the field values from this object. Were you able to test this on a device? If scrolling the sheet still feels smooth then we can just leave it as is for now

michaelknoch commented 2 years ago

Ok fair point. I wanted to go the opposite way and try to fix it asap and then find out what actually fixed it. But your suggested way is also fine. I agree that it's very unlikely that the crash comes from the conversions. I added the remaining fields: https://github.com/flowkey/UIKit-cross-platform/pull/368/commits/10c23817ffddac6f30b82c62965dc9318481800e

Performance seems to be fine on my device. Touches never feel snappy to me on android and it's hard to tell if it got worse with this change. I don't think so. @ephemer @rikner

ephemer commented 2 years ago

Looks great to me @michaelknoch! Hope we can finally stamp out this crash once and for all 🙏🏼