Esri / data-collection-ios

Mobile data collection app using the iOS Runtime SDK.
https://developers.arcgis.com/
Apache License 2.0
25 stars 26 forks source link

Crash when trying to add image as attachment (iPad) #209

Closed mhdostal closed 4 years ago

mhdostal commented 5 years ago

When attempting to add an attachment to a new tree, tapping the "Add Attachments" row in the "Attachments" view crashes the app. The error shown in the log is:

2019-09-18 11:17:26.072695-0500 data-collection[3197:356042] *** Terminating app due to uncaught exception 'NSGenericException', reason: 'Your application has presented a UIAlertController (<UIAlertController: 0x11d272000>) of style UIAlertControllerStyleActionSheet from data_collection.AppContextAwareNavigationController (<data_collection.AppContextAwareNavigationController: 0x11d95ea00>). The modalPresentationStyle of a UIAlertController with this style is UIModalPresentationPopover. You must provide location information for this popover through the alert controller's popoverPresentationController. You must provide either a sourceView and sourceRect or a barButtonItem.  If this information is not known when you present the alert controller, you may provide it in the UIPopoverPresentationControllerDelegate method -prepareForPopoverPresentation.'

The cause is the UIAlertController needs a rectangle to display the action sheet from on an iPad. This can happen either by the controller's popoverPresentationController?.barButtonItem or a combination of popoverPresentationController?.sourceView and popoverPresentationController?.sourceRect properties.

The solution is to provide a source Rect that the popover would display from (the cell's frame maybe?) or to switch to a different type of alert VC (maybe).

This is iPad-only; does not crash on an iPhone 8.

iPad Pro 10.5" iOS 12.4.1

mhdostal commented 5 years ago

The same crash will occur when attempting to add a new feature and there is more than one feature layer in the map. When there is more than one FL, an AlertController is displayed allowing the user to select which FL to add the feature two. The default configuration in the app has only one FL, so this crash would not happen in that case.

mhdostal commented 5 years ago

There are two separate instances of the crash with the UIAlertViewController: image source selection and Feature Layer selection.

The first, image source selection can be solved by changing the type of the UIAlertController to .alert from .actionSheet. There is no barButtonItem in this case as it's triggered by tapping on the AddAttachment row in a table. Also, the display of the action sheet is many layers removed, code-wise, from the table view so that passing in a source view and rect is not practical. So in this case, .alert is the best bet. It will look like this:

image

The second, feature layer selection, can be handled either by making the UIAlertController an .alert OR by passing in a UIBarButtonItem (which the actionSheet would be presented from on an iPad). The bar button item case would involve passing the sender from the @IBAction for the "add feature" button to the func userRequestsAddNewFeature(_ sender: Any) method in MapViewController+LocationSelection extension. The two options look like this:

Alert (both iPhone and iPad look the same):

image

BarButtonItem:

iPhone (this is unchanged from the original code):

image

iPad:

image

@esreli @mikewilburn Do either of you have a preference between using .alert vs. UIBarButtonItem in how the UI is presented?

mikewilburn commented 5 years ago

Thanks for the consideration @mhdostal, but my recommendation here would mean very little. I defer to @esreli!

esreli commented 5 years ago

From a UI perspective, it would be my preference to keep the alert controller configured as an .actionSheet, specifying the UIBarButtonItem's view geometry as the source rect.

@mhdostal what do you think?

mhdostal commented 5 years ago

@esreli I'm cool with keeping the .actionSheet for the layer selection and passing in the UiBarButtonItem.

For the image source selection alert controller it's not so easy. There is no UIBarButtonItem, only the "Add Attachments" table view cell. In the call stack below, ImagePickerPermissions.request(options:) is where the UIAlertController is presented. The action is initiated by a tap on the "Add Attachments" row, so that's would require passing a sourceView and sourceRect down more than one level deep.

#0  0x0000000104d304e0 in ImagePickerPermissions.request(options:) at /Users/mark1113/Development/OpenSourceApps/data-collection-ios/data-collection/data-collection/Utilities/ImagePickerPermissions.swift:181
#1  0x0000000104db58e0 in RichPopupViewController.attachmentsViewControllerDidRequestAddAttachment(_:) at /Users/mark1113/Development/OpenSourceApps/data-collection-ios/data-collection/data-collection/View Controllers/Rich Popup View Controllers/Rich Popup View Controller/RichPopupViewController+RichPopupAttachmentsViewControllerDelegate.swift:41
#2  0x0000000104db60b8 in protocol witness for RichPopupAttachmentsViewControllerDelegate.attachmentsViewControllerDidRequestAddAttachment(_:) in conformance RichPopupViewController ()
#3  0x0000000104de6af8 in RichPopupAttachmentsViewController.tableView(_:didSelectRowAt:) at /Users/mark1113/Development/OpenSourceApps/data-collection-ios/data-collection/data-collection/View Controllers/Rich Popup View Controllers/Rich Popup View Controller/RichPopupAttachmentsViewController/RichPopupAttachmentsViewController+UITableViewDelegate.swift:74

So for the image source selection alert controller, I'd say we switch to the .alert option.

esreli commented 5 years ago

The action is initiated by a tap on the "Add Attachments" row, so that's would require passing a sourceView and sourceRect down more than one level deep.

I see now what you're saying.

So for the image source selection alert controller, I'd say we switch to the .alert option.

I thought we might be able to avoid presenting the user with the notion of an alert, which (to me, at least) implies the user has reached a barrier, in favor of prompting them to take an action. It also appears we have way to present a modal .actionSheet in any other style than .popover. Without some sort of UI anchor, it looks like UIKit has forced our hand on this one.

Admittedly, amidst the other tasks i'm attending to simultaneously, I didn't investigate the issue as thoroughly as I would have needed to. I'm sorry & i'll do better next time.

mhdostal commented 4 years ago

In iOS 13 presenting the image source selection alert controller no longer crashes when using .actionSheet without a UIBarButtonItem or setting a sourceView/sourceRect.

mhdostal commented 4 years ago

Verified in the v1.1.2 release.