refactor(android)!: rename lifecycle bindings - #1288
Conversation
Package Changes Through 229ae4fThere are 2 changes which include tao-macros with patch, tao with minor Planned Package VersionsThe following package releases are the planned based on the context of changes in this pull request.
Add another change file through the GitHub UI by following this link. Read about change files or the docs at github.com/jbolda/covector |
|
do we have a better name for https://github.com/Legend-Master/tao/blob/ca69bcc2fc310472beaf678701f7a72a1411ea62/src/platform_impl/android/ndk_glue.rs#L330 ? maybe a bit confusing to have onCreate and create. |
|
Ah, I sorta see why it was renamed to |
|
Ok so basically wry is doing what tao is supposed to do on Android so it calls |
|
I assume the idea was to have create be called as early as possible (to register the logger whatever as early as possible) but i agree that that doesn't make much sense here if it's also simply called in activity onCreate |
This comment was marked as resolved.
This comment was marked as resolved.
|
@FabianLars have you tried multi-window setup on Android before? The current code looks quite weird that |
| /// - `private external fun onResume(activity: WryActivity)` | ||
| /// - `private external fun onPause(activity: WryActivity)` | ||
| /// - `private external fun onStop(activity: WryActivity)` | ||
| /// - `private external fun onSaveInstanceState(activity: WryActivity)` |
There was a problem hiding this comment.
this getting the activity but not the instance is a bit awkward imo. since we're not using this callback, maybe remove it and only add it when we actually need it so we know what input we actually need?
similar situation for onLowMemory which isn't used anywhere
There was a problem hiding this comment.
this getting the activity but not the instance is a bit awkward imo.
Sorry what instance?
I honestly don't know why onSaveInstanceState is even here that there's no onRestoreInstanceState for it to be useful. Also it doesn't even pass in the bundle for you to store the data which made it completely useless. Maybe removing it?
There was a problem hiding this comment.
similar situation for onLowMemory which isn't used anywhere
That one seems to go through to the event loop but we never mapped it to anything... I don't mind removing the activity here since it's not tied to the activity.
|
yes i tried it before merging lucas' prs. the changes were a bit too much to approve without testing. I did not notice any issues with the current code but i totally agree it's weird. still trying to follow lucas' train of thought on why he added that lifecycle thing |
|
Also found that we're calling |
|
And i just re-tested the multiwindow project, seems like i misunderstood how the lifecycle observer works. It looks like once set it uses the same observer instance across all activities, meaning the observer's onCreate is only called once, not on every activity onCreate. So Rust.create and wryCreate are indeed only called once. Edit: This is true for all of the observer methods we have defined here. They are all called app-wide, not per-activity. I also tested the onpause hook for example when going to background while the main activity is not onscreen. Edit2: This may be because of #1266 (comment) actually |
|
Thanks for testing, that really helped 😂 This is still super confusing I must say that Edit: I see, https://developer.android.com/reference/androidx/lifecycle/ProcessLifecycleOwner I though it was activity lifecycle owner |
Which means all of them were wrong and should be fixed in tauri-apps/wry#1720 ? |
Yeah, that's the part i still don't get either. I don't see any docs saying that it gets ignored in any cases but i guess there's some "normal" java or android thing at play here that notices it's the same instance being added all the time idk. I'll stop thinking about this for now 😂
Not sure what exactly you mean. The PR moves Resumed/Suspended from app-wide to per-activity == from Lifecycle to Activity which makes sense now. But i think i just don't get what you mean. What's also fun is that the lifecycle onCreate fires after activity onCreate so this #1288 (comment) isn't actually the case (or may not be the case) |
The doc says
right right right, that's correct since we didn't move the events yet |
| /// - `private external fun onActivitySaveInstanceState()` | ||
| /// - `private external fun onActivityDestroy(activity: WryActivity)` | ||
| /// - `private external fun onActivityLowMemory()` | ||
| /// - `private external fun onFirstActivityCreate(activity: WryActivity)` |
There was a problem hiding this comment.
we currently don't give it that in wry - we can i think because the lifecycle owner is the activity
if we were i'm thinking about moving the ndk-context thing into here but it sucks that it's called after the activity's onCreate
There was a problem hiding this comment.
we currently don't give it that in wry - we can i think because the lifecycle owner is the activity
We do, it should be easier to see after tauri-apps/wry#1790
if we were i'm thinking about moving the ndk-context thing into here but it sucks that it's called after the activity's onCreate
I don't mind it but I'd probably keep it just for the timing, I could make a comment on the reasons if you want
There was a problem hiding this comment.
No we don't: https://github.com/tauri-apps/wry/pull/1790/changes#diff-76280dd9be7cec50ab33d778adb7380d01ebfe63a7b5373ee637ef710fe60322R29 - i was talking about the function argument.
There was a problem hiding this comment.
I don't mind it but I'd probably keep it just for the timing, I could make a comment on the reasons if you want
Talking about timing, i wonder if we even should use the lifecycle thing or if we just just call it in onActivity if there's a way to have a variable across all activities to check if it has been called already. The incorrect timing here really puts me off.
There was a problem hiding this comment.
No we don't: https://github.com/tauri-apps/wry/pull/1790/changes#diff-76280dd9be7cec50ab33d778adb7380d01ebfe63a7b5373ee637ef710fe60322R29 - i was talking about the function argument.
Ah, I see, was testing it in tauri and it worked so didn't notice
Talking about timing, i wonder if we even should use the lifecycle thing or if we just just call it in onActivity if there's a way to have a variable across all activities to check if it has been called already. The incorrect timing here really puts me off.
Trying something like tauri-apps/wry@11dab6b but I really don't know what java land smokes that if this works. I would honestly say just leave this alone since it didn't cause any problems so far.
There was a problem hiding this comment.
The timing is actually correct I think, we need to run the android entry point after we setup many things.
There was a problem hiding this comment.
Trying something like tauri-apps/wry@11dab6b but I really don't know what java land smokes that if this works. I would honestly say just leave this alone since it didn't cause any problems so far.
That works in the multiwindow example i tested everything else at least
The timing is actually correct I think, we need to run the android entry point after we setup many things.
we're talking about https://github.com/tauri-apps/tao/blob/dev/src/platform_impl/android/ndk_glue.rs#L330 and https://github.com/tauri-apps/tao/blob/dev/src/platform_impl/android/ndk_glue.rs#L389 right? to me create looks like it wants to run first.
I almost want to merge these 2 callbacks, we could do the isFirstActivity check in there instead but i think i'm just going crazy - we can keep everything as is and focus on getting it released instead 😂
There was a problem hiding this comment.
we're talking about https://github.com/tauri-apps/tao/blob/dev/src/platform_impl/android/ndk_glue.rs#L330 and https://github.com/tauri-apps/tao/blob/dev/src/platform_impl/android/ndk_glue.rs#L389 right? to me
createlooks like it wants to run first.
Yeah, it looks like so but it doesn't and it's probably correct (not semantically, functionally) 😂
I would honestly just leave it to another day, I have documented the timings for now.
FabianLars
left a comment
There was a problem hiding this comment.
can't test the last commit rn 🤷
|
I tested or else I wouldn't know there's a |
|
i assumed as much, i just like to document what i did in the approval, hence the |
Waiting on #1209