Closed teodorciuraru closed 8 years ago
And add a brief comment to the PR regarding the widget behaviour so that it is documented here, in this PR.
Teo, the correct bundler identifier is: group.org.systers.peacecorps.malaria (I noticed a issue when reviewing the PR)
And, in the member center I added a AppGroup named: group.org.systers.peacecorps.malaria.tookpillwidget
Please update the PR accordingly ;) Also, in the widget you have copyright text, please remove them :)
Edit: I will fix this myself and you'll have to pull the changes. Edit2: Just updated: https://github.com/systers/malaria-app-ios/commit/a9212d52b1466f2f88b0b5686dcaec2877aba618
I think this is equivalent (taking profit of swift syntax)
func checkIfWeNeedToShowWidget() -> Bool {
return (currentMedicine?.registriesManager.allRegistriesInPeriod(NSDate()).entries.count == 0) ?? false;
}
Also equivalent (less verbose and simpler to read)
// check is user pressed Did Take Pill in the widget
func checkIfUserPressedButtonInWidget() -> Bool {
return NSUserDefaults(suiteName: Constants.Widget.AppGroupBundleID)!.objectForKey(Constants.Widget.DidTakePillForToday) != nil
}
If those logs are not important (I think they arent) I would make this change: from
guard let didTakePill = didTakePillObject as? Bool else {
Logger.Error("Couldn't add pill entry.")
return
}
guard let currentMedicine = currentMedicine else {
Logger.Error("Couldn't add pill entry.")
return
}
to
if let didTakePill = didTakePillObject as? Bool,
currentMedicine = currentMedicine{
// register the entry from the widget
currentMedicine.registriesManager.addRegistry(NSDate(), tookMedicine: didTakePill)
currentMedicine.notificationManager.reshedule()
}
These three changes are working. While reducing the number of lines makes the code more readable in my opinion (less lines and more straightforward). You can check if everything is working with these suggestions
Widget handler is in the backend folder on supporting files (in the project, in the filesystem it is). It should be at least on the Frontend folder.
I would wrap these two functions:
func showWidget() {
widgetController.setHasContent(true, forWidgetWithBundleIdentifier: Constants.Widget.WidgetBundleID)
}
func hideWidget() {
widgetController.setHasContent(false, forWidgetWithBundleIdentifier: Constants.Widget.WidgetBundleID)
}
into
func setVisibility(value: Bool){
widgetController.setHasContent(value, forWidgetWithBundleIdentifier: Constants.Widget.WidgetBundleID)
}
So that in the AppDelegate we just have
widgetHandler.setVisibility(widgetHandler.checkIfWeNeedToShowWidget())
instead of the ternary operator (or ifs)
Edit: Also change checkIfWeNeedToShowWidget name to CheckUserInputForToday (it is more clear what the method will return)
Therefore it becomes
widgetHandler.setVisibility(!widgetHandler.CheckUserInputForToday())
So now the code documents itself saying that the widget only appears when the user hasn't entered information for today! :D
You have a bug here (tiny one)
let context = CoreDataHelper.sharedInstance.createBackgroundContext()!
var currentMedicine: Medicine? {
return MedicineManager(context: context).getCurrentMedicine()
}
If the user changes his medicine, the variable context
is outdated (see it as a snapshot of the CoreData) and you will be adding entries to a medicine that the user has selected as current.
change to:
var currentMedicine: Medicine? {
return MedicineManager(context: CoreDataHelper.sharedInstance.createBackgroundContext()!).getCurrentMedicine()
}
The function just returns a boolean and does nothing: https://github.com/systers/malaria-app-ios/pull/36/files#diff-d9749f74cfe2d6548e5b37b8e4394e67R37
Also, does that method have a purpose? I think you are handling everything through becomeActive events :)
Remove this info: https://github.com/teo029/malaria-app-ios/blob/widget-app-group/Malaria%20App%20Widget/TodayViewController.swift#L3
I think it is replicated in other files. I will look into the remaining things later ;)