Add physical device support #109
Conversation
|
@BasThomas would be nice to get a review here, as we want to release this version before publishing at Monday morning. |
Launcher/Classes/Constants.swift
Outdated
| @@ -9,6 +9,7 @@ enum Constants { | |||
| static let installSimulatorOrPluginDevice = "Please install a simulator or plug-in your device.".localized | |||
| static let useLocalBuild = "Skipping download. Use a local app version.".localized | |||
| static let notCompatibleWithDeviceType = "not compatible with chosen device type.".localized | |||
| static let wrongDeviceSetup = "Please, provide your device IP and bundle identifier. It can be done under 'Configure Device' settings.\n".localized | |||
There was a problem hiding this comment.
heh, good question. Didn't have time to investigate. We just need a new line there, as the method that outputs doesn't do it for free. Do you think it will be better to add the \n in the code?
There was a problem hiding this comment.
Yeah, I think it would be a better fit in the code.
|
|
||
| override func viewDidLoad() { | ||
| super.viewDidLoad() | ||
| if let deviceIP = applicationStateHandler.deviceIP { | ||
| textField.stringValue = deviceIP | ||
| } | ||
| if let bundleIDdata = applicationStateHandler.bundleID { |
There was a problem hiding this comment.
Why do you bind this to bundleIDdata and not just bundleID?
There was a problem hiding this comment.
Because bundleID is already an outlet name
There was a problem hiding this comment.
Ah, right. I think it would be more readable when we rename the bundleID outlet to bundleIDTextField (and also rename the vanilla textField; what's that?) and then in turn rename the bundleIDdata to bundleID.
There was a problem hiding this comment.
Done. Don't like bundleIDTextField: NSTextField tho. But this is always better than names before ;)
| let bundleID = applicationStateHandler.bundleID, | ||
| physicalDeviceRadioButton.state == .on, | ||
| !deviceIP.isEmpty { | ||
| arguments.append("export DEVICE_ENDPOINT=http://\(deviceIP):37265") |
There was a problem hiding this comment.
Is this port (the 37265, I think it's a port?) always the same? Maybe add a constant for it?
There was a problem hiding this comment.
Yes this will be always the same. It is Calabash default port. I will add it to the constants
| } | ||
|
|
||
| var simulatorRadioButtonState: Int { | ||
| var simulatorRadioButtonState: Bool? { |
There was a problem hiding this comment.
Why is this optional? bool(forKey:) returns a Bool: https://developer.apple.com/documentation/foundation/userdefaults/1416388-bool
There was a problem hiding this comment.
Thanks for pointing this out. It was actually causing a bug for a new user.
|
@BasThomas is it ok to be merged? |
| } | ||
|
|
||
| var simulatorRadioButtonState: Int { | ||
| var physicalRadioButtonState: Bool { |
There was a problem hiding this comment.
This doesn't seem right. You removed simulatorRadioButtonState, and the renamed physicalRadioButtonState references .simulatorRadioButtonState.
| } | ||
| enum Ruby { | ||
| static let helpers = main.path(forResource: "helpers", ofType: .ruby) | ||
| } | ||
| } | ||
|
|
||
| enum DeviceType { |
There was a problem hiding this comment.
Not sure if this should be here. I can imagine that this enum will have some more functionality in the future, and I feel it's not strictly just constants.
| import AppKit | ||
|
|
||
| class DeviceSettingsViewController: NSViewController { | ||
| let applicationStateHandler = ApplicationStateHandler() |
There was a problem hiding this comment.
We're using this pretty frequently. I am almost thinking it would make sense to add a singleton here?!
| func getSimulatorsCommand() { | ||
| if let launchPath = Constants.FilePaths.Bash.simulators { | ||
| func getDevicesCommand(ofType type: Constants.DeviceType) { | ||
| var path: String? |
There was a problem hiding this comment.
This can be a let path: String
| let outputStream = CommandTextOutputStream() | ||
| outputStream.textHandler = { text in | ||
| let filderedText = text.components(separatedBy: "\n").filter { $0.contains("Simulator") } | ||
| let filteredText = text.components(separatedBy: "\n").filter { !RegexHandler().matches(for: "\\(([^()])*\\) \\[(.*?)\\]", in: $0).isEmpty } |
There was a problem hiding this comment.
I would maybe do a == false in the filter instead of the ! at the beginning. Especially since you have the matches (but are actually checking the isEmpty), this is pretty easily misread.
| @@ -15,9 +15,9 @@ | |||
| <key>CFBundlePackageType</key> | |||
| <string>APPL</string> | |||
| <key>CFBundleShortVersionString</key> | |||
| <string>1.0</string> | |||
| <string>1.1</string> | |||
There was a problem hiding this comment.
Please do this in a separate PR next time! 😬
No description provided.