Allow file loading (including ROMs and pkgs) from Files app and share sheet #202
Conversation
WalkthroughThe pull request adds iOS document handling capabilities to the iEinstein app. It configures the app to accept Newton ROM images and package files through the Files app by updating Info.plist files with document type declarations and implementing file handlers in the app delegate and view controller to process incoming files. Changes
Sequence DiagramsequenceDiagram
participant FileApp as Files App
participant AppDel as App Delegate
participant FileIO as Filesystem
participant ViewController as View Controller
participant Emulator as Emulator
FileApp->>AppDel: openURL (ROM or .pkg file)
activate AppDel
AppDel->>AppDel: handleIncomingFile:url
AppDel->>FileIO: Acquire security scope
AppDel->>FileIO: Resolve Documents directory
alt File is ROM
AppDel->>FileIO: Copy ROM to destination
AppDel->>ViewController: checkForROMImage
activate ViewController
ViewController->>FileIO: Discover/load ROM
ViewController->>Emulator: Load ROM image
ViewController->>Emulator: Reset emulator
deactivate ViewController
else File is Package
AppDel->>FileIO: Copy package to destination
AppDel->>ViewController: Install packages
AppDel->>FileIO: Remove copied file
end
AppDel->>FileIO: Release security scope
deactivate AppDel
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
rbruels
force-pushed
the
support-file-sharing-from-ios
branch
from
14769ea to
b41b12a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
app/iEinstein/Classes/iEinsteinViewController.h (1)
50-51: Consider migrating toUIAlertController(optional).
UIAlertViewhas been deprecated since iOS 9. While it still works, consider migrating toUIAlertControllerfor future compatibility. This is consistent with the existing use ofUIActionSheetin the codebase, so this could be addressed as part of a broader modernization effort.app/iEinstein/Classes/iEinsteinAppDelegate.mm (1)
45-46: Remove unnecessarysynchronizecall.
registerDefaults:only sets in-memory defaults for keys that haven't been set by the user—it doesn't write to disk. Callingsynchronizehere has no effect and is generally unnecessary in modern iOS.[[NSUserDefaults standardUserDefaults] registerDefaults:defaults]; - [[NSUserDefaults standardUserDefaults] synchronize]; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Resources/iOSEinstein Storyboards-Info.plist(3 hunks)Resources/iOSEinstein-Info.plist(1 hunks)app/iEinstein/Classes/iEinsteinAppDelegate.mm(2 hunks)app/iEinstein/Classes/iEinsteinViewController.h(2 hunks)app/iEinstein/Classes/iEinsteinViewController.mm(5 hunks)
🔇 Additional comments (5)
Resources/iOSEinstein Storyboards-Info.plist (1)
11-33: LGTM! Well-structured document type declarations.The document types and UTI declarations are correctly configured:
- Proper
LSHandlerRank: Ownerclaims ownership for ROM and pkg files- Case-insensitive file extensions covered (rom/ROM, pkg/PKG)
- UTIs conform to
public.dataas expected for binary file typesAlso applies to: 78-121
app/iEinstein/Classes/iEinsteinViewController.mm (3)
135-147: LGTM! Clean dismissal and reset flow.The method properly dismisses the alert, releases resources (under manual memory management), and triggers emulator reset to reinitialize with the newly available ROM.
202-220: Nice approach for Files app visibility.Creating a placeholder file to ensure the Documents folder appears in the Files app is a good UX improvement. This helps users locate where to place ROM files.
360-372: LGTM! Clean extraction of ROM loading logic.The ROM loading logic is now properly encapsulated in
loadROMImage, making the code more maintainable and enabling reuse fromcheckForROMImage.Resources/iOSEinstein-Info.plist (1)
36-41: This file is not actively used in the build configuration. The Xcode project usesiOSEinstein Storyboards-Info.plistfor the iOS Einstein app, not this file. The Storyboards plist already contains the necessaryCFBundleDocumentTypesandUTExportedTypeDeclarationsfor handling Newton Package Data and ROM Image files. No changes are needed here.
| if(![srcPath isEqualToString:dstPath]) { | ||
| if ([[NSFileManager defaultManager] fileExistsAtPath:dstPath]) { | ||
| [[NSFileManager defaultManager] removeItemAtPath:dstPath error:&error]; | ||
| if(error){ | ||
| NSLog(@"%@ %ld %@",[error domain],(long)[error code],[[error userInfo] description]); | ||
| } | ||
| else{ | ||
| NSLog(@"File removed."); | ||
| } | ||
| } | ||
| [[NSFileManager defaultManager] copyItemAtPath:srcPath toPath:dstPath error:&error]; | ||
| } | ||
|
|
||
| if(error){ | ||
| NSLog(@"%@ %ld %@",[error domain],(long)[error code],[[error userInfo] description]); | ||
| } |
There was a problem hiding this comment.
Reset error before copyItemAtPath to avoid logging stale errors.
If removeItemAtPath: fails (setting error), then copyItemAtPath: succeeds (not touching error), the stale error will still be logged at line 132-134, incorrectly suggesting the copy failed.
if(error){
NSLog(@"%@ %ld %@",[error domain],(long)[error code],[[error userInfo] description]);
}
else{
NSLog(@"File removed.");
}
+ error = nil;
}
[[NSFileManager defaultManager] copyItemAtPath:srcPath toPath:dstPath error:&error];
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if(![srcPath isEqualToString:dstPath]) { | |
| if ([[NSFileManager defaultManager] fileExistsAtPath:dstPath]) { | |
| [[NSFileManager defaultManager] removeItemAtPath:dstPath error:&error]; | |
| if(error){ | |
| NSLog(@"%@ %ld %@",[error domain],(long)[error code],[[error userInfo] description]); | |
| } | |
| else{ | |
| NSLog(@"File removed."); | |
| } | |
| } | |
| [[NSFileManager defaultManager] copyItemAtPath:srcPath toPath:dstPath error:&error]; | |
| } | |
| if(error){ | |
| NSLog(@"%@ %ld %@",[error domain],(long)[error code],[[error userInfo] description]); | |
| } | |
| if(![srcPath isEqualToString:dstPath]) { | |
| if ([[NSFileManager defaultManager] fileExistsAtPath:dstPath]) { | |
| [[NSFileManager defaultManager] removeItemAtPath:dstPath error:&error]; | |
| if(error){ | |
| NSLog(@"%@ %ld %@",[error domain],(long)[error code],[[error userInfo] description]); | |
| } | |
| else{ | |
| NSLog(@"File removed."); | |
| } | |
| error = nil; | |
| } | |
| [[NSFileManager defaultManager] copyItemAtPath:srcPath toPath:dstPath error:&error]; | |
| } | |
| if(error){ | |
| NSLog(@"%@ %ld %@",[error domain],(long)[error code],[[error userInfo] description]); | |
| } |
🤖 Prompt for AI Agents
In app/iEinstein/Classes/iEinsteinAppDelegate.mm around lines 119 to 134, the
NSError *error value from removeItemAtPath: can remain set and later be
misinterpreted after copyItemAtPath:—reset the error to nil before calling
copyItemAtPath: (or declare and use a separate NSError variable for the copy
call) and check/log each call's error immediately after that specific API
returns so stale errors are never logged for a subsequent operation.
| - (BOOL)application:(UIApplication *)app openURL:(NSURL *)url options:(NSDictionary<UIApplicationOpenURLOptionsKey, id> *)options | ||
| { | ||
| if ([url.scheme isEqualToString:@"file"]) { | ||
| [self handleIncomingFile:url]; | ||
| return YES; | ||
| } | ||
| else { | ||
| [[UIApplication sharedApplication] openURL:url options:options completionHandler:nil]; | ||
| return YES; | ||
| } | ||
| return NO; | ||
| } |
There was a problem hiding this comment.
Fix unreachable code and reconsider URL forwarding logic.
Line 167 (return NO) is unreachable since both branches return YES. Additionally, forwarding unknown URL schemes to openURL: in the else branch may cause unexpected behavior or infinite loops if the system routes it back.
- (BOOL)application:(UIApplication *)app openURL:(NSURL *)url options:(NSDictionary<UIApplicationOpenURLOptionsKey, id> *)options
{
if ([url.scheme isEqualToString:@"file"]) {
[self handleIncomingFile:url];
return YES;
}
- else {
- [[UIApplication sharedApplication] openURL:url options:options completionHandler:nil];
- return YES;
- }
- return NO;
+ return NO;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - (BOOL)application:(UIApplication *)app openURL:(NSURL *)url options:(NSDictionary<UIApplicationOpenURLOptionsKey, id> *)options | |
| { | |
| if ([url.scheme isEqualToString:@"file"]) { | |
| [self handleIncomingFile:url]; | |
| return YES; | |
| } | |
| else { | |
| [[UIApplication sharedApplication] openURL:url options:options completionHandler:nil]; | |
| return YES; | |
| } | |
| return NO; | |
| } | |
| - (BOOL)application:(UIApplication *)app openURL:(NSURL *)url options:(NSDictionary<UIApplicationOpenURLOptionsKey, id> *)options | |
| { | |
| if ([url.scheme isEqualToString:@"file"]) { | |
| [self handleIncomingFile:url]; | |
| return YES; | |
| } | |
| return NO; | |
| } |
| - (void)applicationDidBecomeActive:(UIApplication*)application | ||
| { | ||
| // Check if ROM was added while app was in background | ||
| // This will dismiss the missing ROM sheet if ROM is now available | ||
| [viewController checkForROMImage]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the checkForROMImage method implementation
fd "\.mm$|\.h$" | xargs rg -A 15 "checkForROMImage" --color=never | head -100Repository: pguyot/Einstein
Length of output: 5923
🏁 Script executed:
#!/bin/bash
# Find the loadROMImage method
fd "\.mm$|\.h$" | xargs rg -A 10 "loadROMImage" --color=never | head -100Repository: pguyot/Einstein
Length of output: 3724
🏁 Script executed:
#!/bin/bash
# Check how viewController is initialized in the delegate
rg -A 20 "applicationDidFinishLaunching|viewController\s*=" app/iEinstein/Classes/iEinsteinAppDelegate.mm | head -80Repository: pguyot/Einstein
Length of output: 1591
🏁 Script executed:
#!/bin/bash
# Check the full context around line 175-179
sed -n '160,190p' app/iEinstein/Classes/iEinsteinAppDelegate.mmRepository: pguyot/Einstein
Length of output: 951
🏁 Script executed:
#!/bin/bash
# Find mLog initialization in iEinsteinViewController
rg "mLog\s*=" app/iEinstein/Classes/iEinsteinViewController.mm | head -20Repository: pguyot/Einstein
Length of output: 127
🏁 Script executed:
#!/bin/bash
# Find viewDidLoad and init methods in iEinsteinViewController to see initialization order
rg -A 30 "(viewDidLoad|^- \(id\)init)" app/iEinstein/Classes/iEinsteinViewController.mm | head -80Repository: pguyot/Einstein
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Search for where mLog is declared and any guards around its usage
rg "mLog" app/iEinstein/Classes/iEinsteinViewController.hRepository: pguyot/Einstein
Length of output: 71
🏁 Script executed:
#!/bin/bash
# Find the context where mLog is initialized with TStdOutLog
rg -B 10 -A 5 "mLog = new TStdOutLog" app/iEinstein/Classes/iEinsteinViewController.mmRepository: pguyot/Einstein
Length of output: 360
🏁 Script executed:
#!/bin/bash
# Look at the full viewDidLoad and viewWillAppear methods
sed -n '1,500p' app/iEinstein/Classes/iEinsteinViewController.mm | rg -A 40 "viewDidLoad|viewWillAppear"Repository: pguyot/Einstein
Length of output: 2681
🏁 Script executed:
#!/bin/bash
# Check if there are any nil-safety checks or guards before checkForROMImage call
rg -B 5 "checkForROMImage" app/iEinstein/Classes/iEinsteinAppDelegate.mmRepository: pguyot/Einstein
Length of output: 504
🏁 Script executed:
#!/bin/bash
# Look for any initialization of viewController besides the storyboard assignment
rg "viewController" app/iEinstein/Classes/iEinsteinAppDelegate.mm | head -30Repository: pguyot/Einstein
Length of output: 834
Add a nil check before calling checkForROMImage in applicationDidBecomeActive.
While the iOS lifecycle typically ensures viewController and its mLog are initialized before this method runs, the code lacks defensive guards. Consider checking that viewController is non-nil and initialized before proceeding, or add a nil check within loadROMImage to guard mLog access.
🤖 Prompt for AI Agents
In app/iEinstein/Classes/iEinsteinAppDelegate.mm around lines 175 to 179, the
code calls [viewController checkForROMImage] without verifying viewController is
non-nil which can lead to messaging nil-dependent state; add a defensive nil
check before invoking the method (for example, if (viewController) {
[viewController checkForROMImage]; }) or, alternatively, add a nil guard inside
loadROMImage to ensure mLog is valid before use so the call is safe when
viewController or its mLog is not yet initialized.
| - (void)alertView:(UIAlertView*)alertView clickedButtonAtIndex:(NSInteger)buttonIndex { | ||
| [[UIApplication sharedApplication] performSelector:@selector(terminateWithSuccess)]; | ||
| } |
There was a problem hiding this comment.
Using private API terminateWithSuccess will cause App Store rejection.
terminateWithSuccess is a private API. Apps using private APIs are rejected from the App Store, which directly conflicts with the PR objective of making the app suitable for the App Store (Issue #48).
Consider using exit(0) as a fallback (still discouraged by Apple) or redesigning the flow to hide the app instead of terminating (e.g., show a persistent "ROM required" screen).
🤖 Prompt for AI Agents
In app/iEinstein/Classes/iEinsteinViewController.mm around lines 57-59, the
alert handler uses the private API terminateWithSuccess which will cause App
Store rejection; remove the performSelector:@selector(terminateWithSuccess) call
and instead either (preferred) dismiss the alert and present a persistent UI
state (e.g., push or present a "ROM required" modal/fullscreen view that
prevents further use until resolved) or (if absolutely unavoidable and
documented) call exit(0) as a last-resort fallback with a comment warning about
App Store risks; ensure the replacement runs on the main thread and cleanly
disables or hides interactive controls so the app does not silently terminate
using private APIs.
| mMissingROMAlertView = [[UIAlertView alloc] | ||
| initWithTitle:@"Newton ROM not found!" message:message delegate:self cancelButtonTitle:nil otherButtonTitles:@"Quit Einstein", nil]; | ||
| [mMissingROMAlertView show]; | ||
| } |
There was a problem hiding this comment.
Potential memory leak if alert is shown multiple times.
If explainMissingROM: is called while mMissingROMAlertView is already showing, the old alert reference is overwritten without being dismissed or released, causing a leak.
+ if (mMissingROMAlertView) {
+ return; // Already showing
+ }
+
mMissingROMAlertView = [[UIAlertView alloc]
initWithTitle:@"Newton ROM not found!" message:message delegate:self cancelButtonTitle:nil otherButtonTitles:@"Quit Einstein", nil];
[mMissingROMAlertView show];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mMissingROMAlertView = [[UIAlertView alloc] | |
| initWithTitle:@"Newton ROM not found!" message:message delegate:self cancelButtonTitle:nil otherButtonTitles:@"Quit Einstein", nil]; | |
| [mMissingROMAlertView show]; | |
| } | |
| if (mMissingROMAlertView) { | |
| return; // Already showing | |
| } | |
| mMissingROMAlertView = [[UIAlertView alloc] | |
| initWithTitle:@"Newton ROM not found!" message:message delegate:self cancelButtonTitle:nil otherButtonTitles:@"Quit Einstein", nil]; | |
| [mMissingROMAlertView show]; | |
| } |
🤖 Prompt for AI Agents
In app/iEinstein/Classes/iEinsteinViewController.mm around lines 130-133, the
new UIAlertView is assigned to mMissingROMAlertView without handling any
existing alert, which leaks if explainMissingROM: is called while an alert is
already shown; before creating/assigning a new alert, check if
mMissingROMAlertView is non-nil and either dismiss it (call
dismissWithClickedButtonIndex:animated: if it's visible) or release it (and set
its delegate to nil) and then set mMissingROMAlertView = nil, then create and
show the new alert and store it in mMissingROMAlertView to ensure the previous
alert is properly dismissed/released.
| - (TROMImage*)loadROMImage | ||
| { | ||
| // Create the ROM. | ||
| TROMImage* romImage = nil; | ||
| NSString* einsteinRExPath = nil; | ||
| NSBundle* thisBundle = [NSBundle mainBundle]; | ||
|
|
||
| if (!(einsteinRExPath = [thisBundle pathForResource:@"Einstein" ofType:@"rex"])) | ||
| { | ||
| //[self abortWithMessage: @"Couldn't load Einstein REX"]; | ||
| mLog->LogLine("Couldn't load Einstein REX"); | ||
| return nil; | ||
| } |
There was a problem hiding this comment.
Potential null pointer dereference on mLog.
loadROMImage assumes mLog is initialized (line 159), but checkForROMImage can be called from applicationDidBecomeActive: before initEmulator creates mLog. This will crash when the Einstein REX isn't found.
Add a null check:
if (!(einsteinRExPath = [thisBundle pathForResource:@"Einstein" ofType:@"rex"]))
{
//[self abortWithMessage: @"Couldn't load Einstein REX"];
- mLog->LogLine("Couldn't load Einstein REX");
+ if (mLog) {
+ mLog->LogLine("Couldn't load Einstein REX");
+ }
return nil;
}🤖 Prompt for AI Agents
In app/iEinstein/Classes/iEinsteinViewController.mm around lines 149 to 161, the
method loadROMImage dereferences mLog when logging the missing Einstein REX path
but mLog may be nil if checkForROMImage is invoked before initEmulator; update
the code to check whether mLog is non-null before calling mLog->LogLine and, if
mLog is nil, use a safe fallback (e.g., NSLog or no-op) so no dereference
occurs; ensure the function returns nil afterwards as before.
| - (BOOL)checkForROMImage | ||
| { | ||
| // If we have a ROM already, do nothing | ||
| if (mROMImage) { | ||
| return YES; | ||
| } | ||
|
|
||
| // Try loading the ROM | ||
| mROMImage = [self loadROMImage]; | ||
| if (mROMImage) { | ||
| if (mMissingROMAlertView) { | ||
| // ROM now exists and action sheet is showing - dismiss it and reset | ||
| [self dismissMissingROMAlertAndReset]; | ||
| } | ||
| return YES; | ||
| } | ||
|
|
||
| return NO; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add guard for uninitialized state.
checkForROMImage can be called from applicationDidBecomeActive: before initEmulator runs (when ROM was previously missing). In this state, mLog is NULL, causing a crash in loadROMImage. Consider adding a guard:
- (BOOL)checkForROMImage
{
+ // Can't load ROM if log isn't initialized (initEmulator hasn't run)
+ if (!mLog) {
+ return NO;
+ }
+
// If we have a ROM already, do nothing
if (mROMImage) {
return YES;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - (BOOL)checkForROMImage | |
| { | |
| // If we have a ROM already, do nothing | |
| if (mROMImage) { | |
| return YES; | |
| } | |
| // Try loading the ROM | |
| mROMImage = [self loadROMImage]; | |
| if (mROMImage) { | |
| if (mMissingROMAlertView) { | |
| // ROM now exists and action sheet is showing - dismiss it and reset | |
| [self dismissMissingROMAlertAndReset]; | |
| } | |
| return YES; | |
| } | |
| return NO; | |
| } | |
| - (BOOL)checkForROMImage | |
| { | |
| // Can't load ROM if log isn't initialized (initEmulator hasn't run) | |
| if (!mLog) { | |
| return NO; | |
| } | |
| // If we have a ROM already, do nothing | |
| if (mROMImage) { | |
| return YES; | |
| } | |
| // Try loading the ROM | |
| mROMImage = [self loadROMImage]; | |
| if (mROMImage) { | |
| if (mMissingROMAlertView) { | |
| // ROM now exists and action sheet is showing - dismiss it and reset | |
| [self dismissMissingROMAlertAndReset]; | |
| } | |
| return YES; | |
| } | |
| return NO; | |
| } |
🤖 Prompt for AI Agents
In app/iEinstein/Classes/iEinsteinViewController.mm around lines 224 to 242,
checkForROMImage currently calls loadROMImage even when the controller/emulator
hasn't been initialized (mLog is NULL), which leads to a crash; add a guard at
the top of checkForROMImage to return NO if the emulator state required by
loadROMImage isn't ready (for example if mLog is nil or an explicit
"initialized" flag is false) so loadROMImage is not invoked prematurely, and
keep existing logic to load/dismiss alerts only after the guard passes.
1 participant
(This PR builds on (and supercedes) awesome work done by @AlohaYos in PR #189. 🤝)
This PR adds iOS file sharing support and removes the need to plug your device into your Mac. Users can now import the all-important
.romfile and additional.pkgfiles via the iOS share sheet or the Files app (which now showsiOSEinstein's Documents folder within the "On My iPhone" section).I shuffled around the ROM loading a bit so that we can detect the ROM at any point in the app lifecycle (so the "Quit Einstein" alert will dismiss if the user didn't press the button but instead went to go load a ROM... which I did a number of times 😆).
Fixes #48.
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.