search

luosheng / OpenSim

OpenSim is an open source alternative to SimPholders, written in Swift.
MIT License
1.28k stars 130 forks source link

Implemented additional features for Simulator reset/delete. #56

Closed CraigPeebles closed 4 years ago

CraigPeebles commented 4 years ago

Hello. This is my first PR to an Open Source project, so please let me know if any of the content or description is unclear, or if you'd prefer any further changes.

I found your project via a work colleague, after I'd been investigating command-line simctl operation of Simulators. These changes are to implement useful new features for my workflow, that I thought might be useful for others as well.

This PR Adds the following features:

Bunn commented 4 years ago

Hey @CraigPeebles thanks for your contribution! I'm really busy with some other things that's why I'm taking some time to review your changes (and there's a lot of them), but I'll try to do it over this weekend.

thanks again :)

CraigPeebles commented 4 years ago

Hi Fernando,

Thanks for your reply. This is my first contribution to OS, so I’m still learning the processes. Would you prefer if I split this into some smaller PR’s, specific to each feature? That might make it easier to review.

Cheers, Craig.

Sent from my iPhone

On 20 Feb 2020, at 7:26 am, Fernando Bunn notifications@github.com wrote:

Hey @CraigPeebles thanks for your contribution! I'm really busy with some other things that's why I'm taking some time to review your changes (and there's a lot of them), but I'll try to do it over this weekend.

thanks again :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

Bunn commented 4 years ago

@CraigPeebles ideally it would be good to have one PR per feature, but there's no need to redo anything now, let me first check what you've pushed and I'll share my thoughts about it, let's avoid unnecessary work until we think it's required, sounds good? :)

CraigPeebles commented 4 years ago

Hi Fernando,

My apologies for not replying to this sooner. The emails were received, but I didn’t filter through my unread properly to see them.

I appreciate your comments and suggestions, and will reply/action to them individually. Just wanted to let you know why I hadn’t replied earlier.

Cheers, Craig Peebles.

On 23 Feb 2020, at 12:41 am, Fernando Bunn notifications@github.com wrote:

@Bunn requested changes on this pull request.

I just had 2 comments on the PR. I really liked all the new features you added, thanks a lot for your contribution :)

In OpenSim/MenuManager.swift https://github.com/luosheng/OpenSim/pull/56#discussion_r382913333:

@@ -169,5 +210,47 @@ protocol MenuManagerDelegate { sender.state = (wasOn ? .off : .on) setLaunchAtLogin(itemUrl: Bundle.main.bundleURL, enabled: !wasOn) } +

  • private func eraseAllSimulators(omitBootedSimulators: Bool) { I think we can improve the readability of this by removing the flag. Methods with flags are a bit harder to read and maintain, my suggestion: Delete this eraseAllSimulators function, and add 3 new functions:

private func resetAllSimulators() { }

private func resetShutdownSimulators() { }

private func resetSimulators(_ devices: [Device]) { } resetSimulators is a generic function that receives an array of devices and resets them, it's agnostic to state or condition, it just resets whatever it receives. resetShutdownSimulators and resetAllSimulators will just filter the simulators you want and then send them to resetSimulators

By doing this resetSimulators function doesn't need conditional checking and will look cleaner, all the logic to get the correct simulators is defined on resetShutdownSimulators and resetAllSimulators which gives each function a single goal.

That way we have a clear understanding of what each method does, and it will make the callstack easier to read from: eraseAllSimulators(omitBootedSimulators: false) to resetAllSimulators

and eraseAllSimulators(omitBootedSimulators: true) to resetShutdownSimulators

In OpenSim/MenuManager.swift https://github.com/luosheng/OpenSim/pull/56#discussion_r382913370:

DeviceManager.defaultManager.reload { (runtimes) in

  • runtimes.forEach { (runtime) in
  • let devices = runtime.devices.filter { $0.applications?.count ?? 0 > 0 }
  • var sortedList = [Runtime]()
  • _ = Dictionary(grouping: runtimes, by: { (runtime: Runtime) in
  • return runtime.platform
  • }).values.map({ (runtimeList: [Runtime]) -> [Runtime] in
  • return runtimeList.sorted { $0.version ?? 0.0 > $1.version ?? 0.0 }
  • }).forEach({ (list) in
  • sortedList.append(contentsOf: list)
  • })
  • sortedList.forEach { (runtime) in
  • var devices = runtime.devices
  • if self.focusedMode {
  • devices = devices.filter { $0.applications?.count ?? 0 > 0 } When I was using this, I missed the booted devices to also show up when the focused mode was active. I had a booted simulator without any apps on it and I wanted to shut it down from the app but couldn't without turning focused mode off. I personally think that we should also filter by booted devices, what do you think?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/luosheng/OpenSim/pull/56?email_source=notifications&email_token=AAW4WXDH2NILDOROOSDGGQDREETRNA5CNFSM4KHDNBWKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCWRWHIQ#pullrequestreview-363029410, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAW4WXEMN3YGEZDRFIBLGA3REETRNANCNFSM4KHDNBWA.

CraigPeebles commented 4 years ago

Thanks again for your review and the helpful comments. Let me know if you have any further thoughts on this.