-
Notifications
You must be signed in to change notification settings - Fork 596
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(device): model now reflects exact model on iOS #929
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the simulator it's returning arm64 (on M1 macs), I think the model check should see if the device is a simulator with the #if targetEnvironment(simulator)
macro and return the old UIDevice.current.model
instead or maybe UIDevice.current.model + " Simulator"
, despite it's not gives the real mode I think it's better to get "iPhone" or "iPhone Simulator" than "arm64".
Good call, I like the idea of falling back to |
device/ios/Plugin/DevicePlugin.swift
Outdated
@@ -24,6 +24,7 @@ public class DevicePlugin: CAPPlugin { | |||
let diskFree = implementation.getFreeDiskSize() ?? 0 | |||
let realDiskFree = implementation.getRealFreeDiskSize() ?? 0 | |||
let diskTotal = implementation.getTotalDiskSize() ?? 0 | |||
let modelName = isSimulator ? ProcessInfo().environment["SIMULATOR_MODEL_IDENTIFIER"]! + " Simulator" : implementation.getModelName() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don’t force unwrap. I thought we had a lint rule to prevent it, but looks like we don’t.
Also, now that we can get the actual name I don’t think the model should contain the “Simulator” part, if users need to know if it’s a simulator they can check the “isVirtual” property
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code produces two warnings
- Will never be executed (line 27)
- Expression implicitly coerced from 'String?' to 'Any' (line 36)
They should be fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now it errors with
Value of optional type 'String?' must be unwrapped to a value of type 'String'
on line 22
closes: #804
This is a feature for the plugin when released along side Capacitor 4 as it's a breaking change.
This work is derived from the work done by @robingenz on their PR #809