Extract libimobiledevice tools interface #10759
Conversation
Extract out IMobileDevice class, move class to idevice_id, ideviceinfo (and eventually other libimobiledevice tools such as iproxy) behind this interface. Add tests for the case where libimobiledevice is not installed, the case where it returns no devices, and the case where it returns device IDs.
|
|
||
| IOSWorkflow get iosWorkflow => context.putIfAbsent(IOSWorkflow, () => new IOSWorkflow()); | ||
|
|
||
| IMobileDevice get iMobileDevice => IMobileDevice.instance; |
There was a problem hiding this comment.
Could move this to mac.dart and kill the instance getter there. If we do that, probably worth doing the same for the Xcode.instance getter. Thoughts?
|
|
||
| // If a device is attached, verify that we can get its name. | ||
| final ProcessResult result = (await runAsync(<String>['idevice_id', '-l'])).processResult; | ||
| if (result.exitCode == 0 && result.stdout.isNotEmpty && !await exitsHappyAsync(<String>['idevicename'])) |
There was a problem hiding this comment.
First, this logic seems wrong... if idevice_id -l exits non-zero, we say isWorking==true??
In any case, with the logic as it is, how abut the following phrasing?
return result.exitCode != 0
|| result.stdout.isEmpty
|| await exitsHappyAsync(<String>['idevicename']);
There was a problem hiding this comment.
The breakage this is detecting happens when idevice_id returns 0, but idevicename returns non-zero.
Specifically we're looking for the case where:
- There are devices attached:
idevice_idreturns 0 when devices are attached, non-zero otherwise. - We fail to get the name of the attached device(s):
idevicenamereturns non-zero in this case.
This typically happens when an older version of libimobiledevice is installed that can't handle the iOS version on the device. (Unfortunately libimobiledevice tools don't include a --version flag or we'd just do that)
The reason for the current phrasing was an effort to make the above condition slightly more direct, but if that's not the case, I can definitely demorgan's it the other way.
There was a problem hiding this comment.
Same logic, but broke it up a bit and added comments.
| } | ||
|
|
||
| List<String> getAttachedDeviceIDs() { | ||
| return runSync(<String>['idevice_id', '-l']).trim().split('\n').where((String line) => line.isNotEmpty).toList(); |
There was a problem hiding this comment.
newlines for cascade?
return runSync(<String>['idevice_id', '-l'])
.trim()
.split('\n')
.where((String line) => line.isNotEmpty)
.toList();
| return true; | ||
| } | ||
|
|
||
| List<String> getAttachedDeviceIDs() { |
There was a problem hiding this comment.
How tough is it to change this to Stream<String> and use runAsync()? We've had bugs in this code where sync process invocations hang flutter tools indefinitely (if the child process doesn't exit), whereas in async, we can time them out.
There was a problem hiding this comment.
Painful -- it cascades down to a whole bunch of API changes to shared iOS and Android interfaces :( I think it's worth doing, but I'd rather avoid doing it as part of this change.
| } | ||
|
|
||
| static String _getDeviceInfo(String deviceID, String infoKey) { | ||
| final String informerPath = _checkForCommand('ideviceinfo'); |
There was a problem hiding this comment.
This isn't needed - informerPath exists as an instance getter (and is the same value).
There was a problem hiding this comment.
Even better, after killing off _getDeviceInfo, informerPath isn't needed either.
| } | ||
| } | ||
|
|
||
| static String _getDeviceInfo(String deviceID, String infoKey) { |
There was a problem hiding this comment.
Looks like this can be removed in favor of IMobileDevice.getInfoForDevice()
Extract out IMobileDevice class, move class to idevice_id, ideviceinfo (and eventually other libimobiledevice tools such as iproxy) behind this interface. Add tests for the case where libimobiledevice is not installed, the case where it returns no devices, and the case where it returns device IDs.
3 participants
Extract out IMobileDevice class, move class to idevice_id, ideviceinfo
(and eventually other libimobiledevice tools such as iproxy) behind this
interface.
Add tests for the case where libimobiledevice is not installed, the case
where it returns no devices, and the case where it returns device IDs.