search

jamf / PPPC-Utility

Privacy Preferences Policy Control (PPPC) Utility
MIT License
735 stars 46 forks source link

Fix for implicit unwrapped optional - username #91

Closed MarkusBux closed 3 years ago

MarkusBux commented 3 years ago

This change fixes an app crash in case of cancelling an upload action.

Steps to reproduce the crash:

  1. Add an app
  2. Click on Upload
  3. Uncheck save credentials
  4. Click on cancel button
hisaac commented 3 years ago

Thank you for the PR @MarkusBux and bringing attention to the issue.

Looks like there are quite a few places in this file that the username is assumed to be non-nil. If it's possible for it to actually be nil, it could effect the other spots as well. It would be better to mark it as optional and handle that throughout this file.

It's not clear who originally wrote this file from the git blame. @cyrusingraham, @watkyn, @macblazer Do any of you know why all the variables at the top of this file are @objc dynamic and implicitly unwrapped?

macblazer commented 3 years ago

The @objc dynamic is required for the use of KVO to handle property updates. See Apple docs on KVO.

I haven't tried KVO with Optionals but a simple Google search seems to indicate that there may be problems with it. Somebody could give it a try to see what the current state of Swift 5 with KVO and Optionals is.

If KVO does work with Optionals, I highly recommend we move away from the implicitly unwrapped Optional and make it explicit. The code is much safer with that approach, and there are several other places in this file that check for nil as well.

Implicitly unwrapped Optional is probably OK for use with the @IBOutlet tag, but even then if the storyboard is changed and the outlet connection is dropped then the app will crash while trying to access that outlet. Explicit optionals are much better to prevent app crashes, although it's hard to know what to do in the code when a required outlet is not loaded from the storyboard.

hisaac commented 3 years ago

Ah ok. Good to know, I've never worked with KVO before.

It looks like we do have a couple variables set as optional — siteName and siteId — and they are being used in an addObserver method — which I assume is KVO-related. So maybe that means it's OK to use optionals now.

MarkusBux commented 3 years ago

So I will try to change this for this file and give it a try...

MarkusBux commented 3 years ago

KVO with explicit Optionals are working. Updated the code to use explicit instead of implicit Optionals

MarkusBux commented 3 years ago

@hisaac, @macblazer Thanks a lot. This is my first PR in a repo not owned by myself and my every first swift related part. Just started with swift a month ago. So thanks for all the help in getting to the right direction!