State tracking based - isa swizzle #492
Conversation
|
@GLinnik21 @bamx23 for review, low pri. |
| _transitionComplete = YES; | ||
| NSLog(@"[AC] %s, completed: %d", ksapp_transition_state_to_string(transitionState), _transitionComplete); | ||
|
|
||
| // send delegate |
| if (completeOnNextLoop) { | ||
| __weak typeof(self)weakMe = self; | ||
| CFRunLoopPerformBlock(CFRunLoopGetCurrent(), kCFRunLoopCommonModes, ^{ | ||
| [weakMe _locked_completeState:transitionState]; |
There was a problem hiding this comment.
So you ensure that all current processing related to the state change is fully completed before marking the transition as complete, but notify observers right away?
|
|
||
| + (void)load | ||
| { | ||
| static BOOL sDontSwizzle = [NSProcessInfo.processInfo.environment[@"KSCRASH_APP_SCENE_DELEGATE_SWIZZLE_DISABLED"] boolValue]; |
There was a problem hiding this comment.
Is this env for debug purposes only? Is there a way to disable it in a production env, for instance in SPM?
|
|
||
| @end | ||
|
|
||
| @interface Proxier : NSObject |
There was a problem hiding this comment.
Maybe it's better to move to another file?
|
|
||
| static void __KS_CALLING_DELEGATE__(id self, SEL cmd, id arg) | ||
| { | ||
| std::string name(sel_getName(cmd)); |
There was a problem hiding this comment.
Again, I would avoid C++ if it's needed only for collections
| NSLog(@"[MAP] %s", name.c_str()); | ||
| const auto it = gMappings.find(name); | ||
| if (it != gMappings.end()) { | ||
| NSLog(@"[MAP:implemented] %s", name.c_str()); |
|
|
||
| @implementation __KS_CALLING_DELEGATE_TEMPLATE__ | ||
|
|
||
| static BOOL SwizzleInstanceMethod(Class klass, SEL originalSelector, SEL swizzledSelector) |
There was a problem hiding this comment.
Can this function be useful in other parts of the library?
| if (sDontSwizzle) { | ||
| return; | ||
| } | ||
| #if KSCRASH_HAS_UIAPPLICATION |
There was a problem hiding this comment.
You check it in the beginning of the file. In case this check is needed, maybe there is no point to run +load if the OS does'n support UIApplication?
|
@naftaly I noticed that there are conflicts in this pull request. Do you still plan to merge it? |
I don’t have time in the near future, but I think it’s an improvement to KS so if you want maybe you can commandeer and move it forward? |
|
@naftaly My apologies for the very delayed response. I've been quite busy lately and have also been focusing on preparing version 2.0. Given that it's been two months, this PR likely has even more conflicts now, which will only worsen over time. Perhaps we should revisit this feature after the 2.0 release? When you have time, it would be great if you could take another look and possibly rebase it. You're more familiar with the initial implementation, so your drive on this would be valuable. Let me know your thoughts on timing and next steps, considering the current state of the PR. |
|
@GLinnik21 No issues in the delay, there's really no rush. I look forward to v2.0 being released and hopefully merged into larger toolsets that use it. We can revisit in the future. Ping me when you get around to planning for the next versions and we can discuss. |
If we can, we try and swizzle App and Scene delegates.
This is a proposal. What you have already works just fine, but it could be a bit more precise.
Lifecycle delegate and notifications don't happen in the same stack, the stack unwinds after the delegate calls.
run loop cycle 1: Application -> Delegate
run loop cycle 2: Application -> Notification
Due to this, it's very hard for a system to receive lifecycle events before the app it is being installed in. This can lead to mismatched user perception (foreground when the app is actually background). That mismatch leads to wrongly categorizing issues reported by the system.
To fix this, we need to swizzle app a scene delegates. This allows us to receive delegate callbacks before anyone else and correctly record the state of the app before the app has a chance to run any code in those transitions and possibly cause a reliability event.
The swizzle is the same kind of swizzling used in KVC as described here.