[feature/open_in] Open in another app (via UIDocumentInteractionController) #132
Conversation
| @@ -51,6 +51,8 @@ class ClientQueryViewController: UITableViewController, Themeable { | |||
| private var observerContext : UnsafeMutableRawPointer | |||
| var refreshController: UIRefreshControl? | |||
|
|
|||
| var interactionController: UIDocumentInteractionController? | |||
|
|
|||
| // MARK: - Init & Deinit | |||
| public init(core inCore: OCCore, query inQuery: OCQuery) { | |||
| observerContext = UnsafeMutableRawPointer(&observerContextValue) | |||
There was a problem hiding this comment.
@pablocarmu I checked the code and I think observerContext is not really required, since it is used when e.g. you observe two objects for the change of same keypath and want to differentiate between them when the observer iis called.
| func openInRow(_ item: OCItem, viewDidAppearHandler: ClientActionVieDidAppearHandler? = nil, completionHandler: ClientActionCompletionHandler? = nil) { | ||
|
|
||
| if let progress = self.core?.downloadItem(item, options: nil, resultHandler: { (error, _, _, file) in | ||
| if error != nil { |
There was a problem hiding this comment.
@pablocarmu If I remember correctly completion handler of downloadItem() is not guaranteed to be called on the main thread, so are we missing dispatch on main thingy here?
There was a problem hiding this comment.
@pablocarmu I guess I had to wait until you remove [WIP] to start reviewing..
Codecov Report
@@ Coverage Diff @@
## master #132 +/- ##
===========================================
- Coverage 48.84% 24.16% -24.68%
===========================================
Files 91 96 +5
Lines 7657 7854 +197
===========================================
- Hits 3740 1898 -1842
- Misses 3917 5956 +2039
Continue to review full report at Codecov.
|
|
|
There was a problem hiding this comment.
Did a quick (but not complete) test and code review. Commented about a few findings.
Other than that I also found:
- the SDK wasn't using the latest master (fixed that with a commit)
- trying to save an image makes iOS kill the app because
NSPhotoLibraryAddUsageDescriptionis missing from theInfo.plist(kind of unexpected for me too, TBH, but… ok):
2018-10-30 13:52:19.694273+0100 ownCloud[33446:4280224] [access] This app has crashed because it attempted to access privacy-sensitive data without a usage description. The app's Info.plist must contain an NSPhotoLibraryAddUsageDescription key with a string value explaining to the user how the app uses this data.
|
|
||
| let controller = DownloadFileProgressHUDViewController() | ||
|
|
||
| if core!.reachabilityMonitor.available { |
There was a problem hiding this comment.
core.downloadItem will work offline and return the file if the file is already downloaded, so the lack of connectivity shouldn't be a blocker.
Of course, the user should be getting feedback if a file can't be downloaded right now. However, there's no efficient way in the app to do this right now. The approach I'd like to take there is adding an option to OCCore.downloadItem whereby the app can tell the SDK that the download should be cancelled and an appropriate error be returned if network isn't available at the time of the method call.
There was a problem hiding this comment.
So I'll remove the check and leave a TODO and when the SDK has support for this, implement it.
I'll open an issue with the info to track this if you agree with the approach.
| Log.log("Error \(String(describing: error)) downloading \(String(describing: item.path)) in openIn function") | ||
| completionHandler?(false) | ||
| } else { | ||
| controller.dismiss(animated: true, completion: { |
There was a problem hiding this comment.
The resultHandler isn't guaranteed to be called on the main thread. So this UI call can (and will) be called on another thread.
Also, there's no guarantee that the resultHandler is called only after OCCore.downloadItem finishes, so this could be called even before controller.present(on: self) further down below.
| controller.dismiss(animated: true, completion: { | ||
| self.interactionController = UIDocumentInteractionController(url: file!.url) | ||
| self.interactionController?.delegate = self | ||
| OnMainThread { |
There was a problem hiding this comment.
UIViewController.dismiss will return on the main thread, so this shouldn't be necessary.
There was a problem hiding this comment.
I'll remove the main thread call.
| }) | ||
| } | ||
| }) { | ||
| OnMainThread { |
There was a problem hiding this comment.
This code should already run on the main thread, so this shouldn't be needed.
There's no guarantee that OCCore.downloadItem finishes and returns before the resultHandler is called, so controller.dismiss may already have been called before controller.present.
There was a problem hiding this comment.
I'll take the following approach then:
- Present the controller.
- Call the
if let progress = self.core.downloadItem... - Attach the progress if any.
- If the completion handler is called before the return, no strange behavior should appear. The controller should be dismissed.
What do you think?
| } | ||
| } | ||
| } else { | ||
| OnMainThread { |
There was a problem hiding this comment.
This code should already run on the main thread, so this shouldn't be needed.
There was a problem hiding this comment.
I'll remove the main thread call.
|
|
||
| class DownloadFileProgressHUDViewController: UIViewController { | ||
|
|
||
| private let progressViewSidesConstraintConstant: CGFloat = 20 |
There was a problem hiding this comment.
This should look better with more spacing…
| private let progressViewSidesConstraintConstant: CGFloat = 20 | |
| private let progressViewSidesConstraintConstant: CGFloat = 40 |
… or, considering iPads, a maximum width.
There was a problem hiding this comment.
I'll test this with an iPad and see how it looks. In case it looks bad, I'll attach new constraints for iPad layouts.
| func openInRow(_ item: OCItem, button: UIBarButtonItem? = nil) { | ||
|
|
||
| if source == nil { | ||
| if !core.reachabilityMonitor.available { |
There was a problem hiding this comment.
Please find a good way to fuse this code together with the code in ClientQueryViewController, so it's not as repetitive.
Quick idea: move this (and – in the future – all other actions) code into its own class that takes a core, an item and a viewController to initialize and then allows to perform either actions or presenting an "More" card for the item on another viewController.
At the end, this should allow to also get rid of other "repeatish" code in this file (like optionsBarButtonPressed and openDocumentInteractionController).
There was a problem hiding this comment.
I've been thinking about this and tried a couple of solutions, but I've faced a lot of issues trying to make it work easily.
What I think could be abstracted it's the creation of the card with all the actions and but not the action themselves (or at least not all).
I've tried to make something like this
typealias ActionCompletion = ((_ item: OCItem, _ core: OCCore, _ vcToPresent: UIViewController) -> Void)?
enum ActionType {
case destructive
case regular
}
struct Action {
var name: String
var type: ActionType
var completion: ActionCompletion
init(with name: String, completion: ActionCompletion, type: ActionType) {
self.name = name
self.completion = completion
self.type = type
}
}To be able to abstract the actions into an struct, then I created a card using the more style and those actions
func actionsViewController(with actions: [Action], for item: OCItem, core: OCCore) -> MoreViewController {
let header = MoreViewHeader(for: item, with: core)
let tableViewController = MoreStaticTableViewController(style: .grouped)
let moreViewController: MoreViewController = MoreViewController(item: item, core: core, header: header, viewController: tableViewController)
var rows: [StaticTableViewRow] = []
for action in actions {
var style: StaticTableViewRowButtonStyle
switch action.type {
case .destructive:
style = .destructive
default:
style = .plainNonOpaque
}
let row: StaticTableViewRow = StaticTableViewRow(buttonWithAction: { (_, _) in
moreViewController.dismiss(animated: true, completion: {
action.completion?(item, core, self.vcToPresentIn!)
})
}, title: action.name, style: style)
rows.append(row)
}
let title = NSAttributedString(string: "Actions".localized, attributes: [NSAttributedStringKey.font: UIFont.systemFont(ofSize: 20, weight: .heavy)])
let section = MoreStaticTableViewSection(headerAttributedTitle: title, identifier: "actions-section", rows: rows)
tableViewController.addSection(section)
return moreViewController
}This works fine for Move and Duplicate because then you can create functions like this one:
func move(completion: (() -> Void)? = nil) -> Action {
let action = Action(with: "Move".localized, completion: { (item, core, viewController) in
guard let viewController = viewController as? ClientQueryViewController else {
return
}
let directoryPickerVC = ClientDirectoryPickerViewController(core: core, path: "/", completion: { (selectedDirectory) in
if let progress = core.move(item, to: selectedDirectory, withName: item.name, options: nil, resultHandler: { (error, _, _, _) in
if error != nil {
Log.log("Error \(String(describing: error)) moving \(String(describing: item.path))")
}
completion?()
}) {
viewController.progressSummarizer?.startTracking(progress: progress)
}
})
let pickerNavigationController = ThemeNavigationController(rootViewController: directoryPickerVC)
viewController.navigationController?.present(pickerNavigationController, animated: true)
}, type: .regular)
return action
}But this adds a problem when each view controller that wants the action also wants a custom implementation of what happens on every momment, maybe they want a custom UI or something that with this implementation it's not posible.
So another solution came to my mind, make all the abstraction returns a Result:
enum Result<T> {
case failure(Error)
case success(T)
}and make the call something like:
func move(progress: @escaping (Progress) -> Void, result: (Result<Bool>) -> Void) {}But this also has the problem of the custom implementation, so I think the best solution pass through abstract the way the card controller is created and create/attach each action inside the view controller it's going to use it. This way we avoid stupid unneded castings inside a common method and gives us the flexibility to insert the actions we want in all the possible scenarios.
I've uploaded a gist with the complete implementation of the first solution I explained here so you can take a look and judge it.
There was a problem hiding this comment.
@pablocarmu Finally coming around to answer. Thanks a ton for the patience, extensive feedback and thinking on this!
That gist already comes pretty close to what I had in mind last week. I didn't think of actions as their own objects or structs, though. Instead, I'd have tried to simply move over the methods we already have in ClientQueryViewController.
The idea of having actions as fully independent building blocks is appealing, though. And it immediately makes me think of putting them into OCExtensions:
- a new
OCExtensionsubclassOCActionExtension(or justActionExtension) could add infos like name and actionType - control over where an action is present (i.e. only in the action sheet, or also in the table row actions) could leverage the
locationsfeature of extensions (through differentOCExtensionLocationIdentifiers) - control over whether an action should be presented for an item could leverage the
customMatcherfeature of extensions. The same feature could also be used to establish an order of appearance (even taking into account the location of appearance, so f.ex. cards could have a different sorting as table row actions) via the matching score (and sorting by that score). - the identifier of the action extension in combination with
OCClassSettingsand custom matching could be used to allow MDM-side configuration of actions (especially to remove actions). Blocked actions, f.ex. would just not match. - a new
OCExtensionContextsubclassOCActionContext(or justActionContext) could be used to pass along information required by the actions like items, core, query, viewController, etc. to make matching decisions, but also to present custom UI - that context subclass could be extended as needed to accommodate for any special requirements, including completion handlers and results
- building this on
OCExtensions of course also would allow the addition of customer-specific actions without requiring any changes in the app code itself
What do you think?
There was a problem hiding this comment.
I made a quick PoC of this in a gist
I didn't had time to check if it works inside the app, but I think this almost matches with what you wrote, I'm still need to get better the idea but I think this could be a base to start a discussion about the implementation.
Even if this PR gets more time of what was initially though, I think we should address a correct implementation of the actions.
What do you think about the gist?
There was a problem hiding this comment.
Thanks for putting the gist together @pablocarmu!
I've forked and extended it (gist here) to include everything I wrote (except OCClassSettings integration, which is just a comment in the spot I'd put it) and test a few ideas.
One of these ideas was to let Action instances directly create StaticTableViewRows and UIContextualActions for use in the UI. The main idea for this was to have convenient access to these through default "builders" in the base Action class, but to also have the flexibility to customize them in the respective Action subclass as needed.
michaelstingl
added
p2-high
|
Added test plan here: https://github.com/owncloud/QA/blob/master/Mobile/iOS-app/Open%20In.md |
pablocarmu
force-pushed
the
feature/open_in
branch
from
4f4ab08 to
a54e798
Compare
|
|
||
| let alertController = UIAlertController( | ||
| with: item.name!, | ||
| message: "Are you sure you want to delete this item from the server?".localized, |
There was a problem hiding this comment.
These two messages, too, would need to be adapted for multiple files.
| beforeRunHandler?() | ||
|
|
||
| let item = context.items[0] | ||
| let rootItem = context.items[1] |
There was a problem hiding this comment.
items should only contain items to perform the action on, and might have different parent items. Therefore, the parentItem should be retrieved from OCCore dynamically.
I wrote an OCItem category that does this for you:
extension OCItem {
func parentItem(from core: OCCore, completionHandler: ((_ error: Error?, _ parentItem: OCItem?) -> Void)? = nil) -> OCItem? {
var parentItem : OCItem?
if let parentItemIdentifier = self.parentFileID {
var waitGroup : DispatchGroup?
if completionHandler == nil {
waitGroup = DispatchGroup()
waitGroup?.enter()
}
core.retrieveItemFromDatabase(forFileID: parentItemIdentifier) { (error, _, item) in
if completionHandler == nil {
parentItem = item
waitGroup?.leave()
} else {
completionHandler?(error, item)
}
}
waitGroup?.wait()
}
return parentItem
}
}It can be used both synchronous and asynchronously:
// Synchronous (please avoid if possible)
let parentItem = item.parentItem(from: core)
// Asynchronous (and preferred)
item.parentItem(from: core) { (_ error, _ parenItem) -> Void in
OnMainThread {
// work with parentItem
}
}Same for RenameAction.
There was a problem hiding this comment.
I made this changed in both places, thanks for the help!
| self.completionHandler?(nil) | ||
| self.interactionController = UIDocumentInteractionController(url: file!.url) | ||
| self.interactionController?.delegate = self | ||
| OnMainThread { |
There was a problem hiding this comment.
This OnMainThread should not be needed as UI completion handler are generally executed on the main thread.
| }) | ||
| } | ||
| }) { | ||
| OnMainThread { |
There was a problem hiding this comment.
This OnMainThread should not be needed as UI completion handler are generally executed on the main thread.
|
|
||
| let controller = DownloadFileProgressHUDViewController() | ||
|
|
||
| OnMainThread { |
There was a problem hiding this comment.
Maybe we should make it a convention that Action.run() may only be called from the main thread (and log a warning in preRun() if it's called on any other thread, so we catch that early). You can then get rid of OnMainThread here.
| override class var name : String { return "Open in".localized } | ||
|
|
||
| override class func applicablePosition(forContext: ActionContext) -> ActionPosition { | ||
| if forContext.items[0].type == .collection { |
There was a problem hiding this comment.
As mentioned in a previous comment, to make Actions truly universally usable throughout the app, it should look at all items. In this case, if any of the items is a .collection, that'd mean this action isn't available.
There was a problem hiding this comment.
I think it depends on the action itself. Of course, a Delete action should delete as many items as you want, but for open in, this action should not be available in multiselection environments because of UIDocumentInteractionController can be only created with one URL and I think we should not open more than one UIDocumentInteractionController. Same for the Rename action.
| @@ -307,11 +312,45 @@ class DisplayViewController: UIViewController { | |||
| self.noNetworkLabel?.isHidden = true | |||
| self.showPreviewButton?.isHidden = false | |||
| } | |||
|
|
|||
| @objc func optionsBarButtonPressed() { | |||
There was a problem hiding this comment.
Would be great to see this and moreButtonTapped merged into a single method, f.ex. into a class-level method of the Action class.
- Made one Action subclass per action. - Register the actions in the AppDelegate - Ask for extensions in ClientQueryViewController.
- Reversed the extensions array because the pction's priority mechanism needs more work.
- Code new Rename Action.
- Made the actions inside the preview to use the extensions mechanism.
- Use the correct API for get the actions.
OCItem extension. - Make sure the actions uses all the items when it makes sense. - Removed some of main thread calls.
- Removed some unused code.
- Removed empty white line to fix the linter warning.
…ext for OpenInAction Co-Authored-By: pablocarmu <[email protected]>
Co-Authored-By: pablocarmu <[email protected]>
Co-Authored-By: pablocarmu <[email protected]>
downloading the item.
- Removed left-over code from DisplayViewController.
nil) as suggested by @felix-schwarz - Replace old calls to completionHandlers with these two new functions.
- Removed unneeded code (fixed in the SDK).
- Added code removed while rebasing against.
pablocarmu
force-pushed
the
feature/open_in
branch
from
9d465d6 to
7edde84
Compare
Description
I coded a new action in the card view controller that is launched through the 3 dots in the file list to be able to open in a file in another app.
Related Issue
#130
Motivation and Context
With this PR you're able to open one item in another app
Types of changes
Checklist:
BUGS & IMPROVEMENTS