Feature/file previewing #114
Conversation
|
|
Codecov Report
@@ Coverage Diff @@
## master #114 +/- ##
=========================================
- Coverage 28.85% 27.3% -1.56%
=========================================
Files 65 71 +6
Lines 5854 6278 +424
=========================================
+ Hits 1689 1714 +25
- Misses 4165 4564 +399
Continue to review full report at Codecov.
|
pablocarmu
force-pushed
the
feature/filePreviewing
branch
from
8b3914a
to
8f0bd36
Compare
|
|
||
| [[OCCoreManager sharedCoreManager] returnCoreForBookmark:_bookmark completionHandler:nil]; | ||
|
|
||
| _core = nil; |
There was a problem hiding this comment.
@pablocarmu without knowing details, don't you have to wait until completionHandler() is called before you reset _core to nil?
|
|
||
| if (error != nil) | ||
| { | ||
| // TODO: Report error as NSFileProviderErrorServerUnreachable or NSFileProviderErrorNotAuthenticated, depending on what the underlying error is |
| { | ||
| queryPath = item.path; | ||
| } | ||
| // |
There was a problem hiding this comment.
@pablocarmu I would not leave dead code in the project without justification
| } | ||
| } | ||
|
|
||
| /* TODO: |
There was a problem hiding this comment.
@pablocarmu Can we fix this TODO? Or is it something which is not critical right now?
|
|
||
| // - (void)enumerateChangesForObserver:(id<NSFileProviderChangeObserver>)observer fromSyncAnchor:(NSFileProviderSyncAnchor)anchor | ||
| // { | ||
| /* TODO: |
There was a problem hiding this comment.
@pablocarmu Same as before, I think we shall try not to leave unfixed TODOs if possible.
| import UIKit | ||
| import ownCloudUI | ||
|
|
||
| class ThemeCertificateViewController: OCCertificateViewController, Themeable { |
There was a problem hiding this comment.
@pablocarmu Shall not OCCertificateViewController be themeable? Does this subclass make sense?
| NSLayoutConstraint.activate([ | ||
| iconImageView.centerXAnchor.constraint(equalTo: view.safeAreaLayoutGuide.centerXAnchor), | ||
| iconImageView.centerYAnchor.constraint(equalTo: view.safeAreaLayoutGuide.centerYAnchor), | ||
| iconImageView.heightAnchor.constraint(equalToConstant: 200), |
There was a problem hiding this comment.
@pablocarmu avoid using magic numbers
|
|
||
| override func viewDidLoad() { | ||
| super.viewDidLoad() | ||
| iconImageView.image = item.icon(fitInSize:CGSize(width: 200.0, height: 200.0)) |
There was a problem hiding this comment.
@pablocarmu avoid using magic numbers
| static var supportedMimeTypes: [String] {get} | ||
| } | ||
|
|
||
| struct FeatureKeys { |
There was a problem hiding this comment.
@pablocarmu why not use enum?
|
|
||
| var matchPriority: OCExtensionPriority = .noMatch | ||
|
|
||
| if context.location.type == self.type { |
There was a problem hiding this comment.
@pablocarmu For me the below code is not self-explaining without comments
There was a problem hiding this comment.
it's a Swift translation of this.
It does the same but also in the location comparison it compares the mime-type of the file you want to present and if it matches, the context match and it's returned as a possible matched value.
it goes through the location, the features and the priority.
I think the only part that it differ from the original one is:
guard let displayLocation = location as? OCDisplayExtensionLocation,
let contextDisplayLocation = context.location as? OCDisplayExtensionLocation else {
return OCExtensionPriority.noMatch
}
for mimeType in displayLocation.supportedMimeTypes {
if contextDisplayLocation.supportedMimeTypes.contains(mimeType) {
matchedLocation = true
matchPriority = .locationMatch
break
}
}
There was a problem hiding this comment.
If you put the MIME-Type into the locationIdentifier, there's no longer any need to override matchesContext.
As far as I can see, the locationIdentifier is not really in use for anything right now, so that's what I'd propose.
pablocarmu
force-pushed
the
feature/filePreviewing
branch
from
8f0bd36
to
baee6de
Compare
|
This branch had previously inside all the commits from the As a side effect on this clean, every time the app try to download an element to preview it the app crashes because there is no file provider inside. This could seem like there is a problem in the branch but it isn't. @mneuwert I reviewed all the changes you proposed and made all but the 2 I commented. Please check again the code and feel free to propose more changes. |
|
|
||
| class OCDisplayExtension: OCExtension { | ||
|
|
||
| init(identifier: OCExtensionIdentifier!, type: OCExtensionType!, location: OCExtensionLocation!, features: [String : Any]!, objectProvider: @escaping OCExtensionObjectProvider) { |
There was a problem hiding this comment.
I was about to say that there's +[OCExtension extensionWithIdentifier:type:location:features:objectProvider:] already, but then noticed, that doesn't seem to get translated to Swift properly. 😬
I just added a pair of init-methods to the SDK (feature/filemgmt) now, so that this can be removed/replaced by them.
| let requirement = context.preferences[preferencekey.key] as AnyObject | ||
| if feature.isEqual(requirement) { | ||
| // TODO : Talk with felix because this is not allowed in Swift. | ||
| //matchPriority += Float(OCExtensionPriority.featureMatchPlus.rawValue) |
There was a problem hiding this comment.
You can achieve this by working solely with raw values, like f.ex.:
matchPriority = OCExtensionPriority.init(rawValue: matchPriority.rawValue + OCExtensionPriority.featureMatchPlus.rawValue)!|
|
||
| var matchPriority: OCExtensionPriority = .noMatch | ||
|
|
||
| if context.location.type == self.type { |
There was a problem hiding this comment.
If you put the MIME-Type into the locationIdentifier, there's no longer any need to override matchesContext.
As far as I can see, the locationIdentifier is not really in use for anything right now, so that's what I'd propose.
| struct FeatureKeys { | ||
| static let canEdit: String = "featureKeyCanEdit" | ||
| static let canSave: String = "featureKeyCanSave" | ||
| static let showPDF: String = "featureKeyShowPDF" |
There was a problem hiding this comment.
As far as I can see, we don't need showPDF as a feature. Instead, PDF viewer(s) can be found by querying� OCExtensionManager with the PDF MIME-Type application/pdf.
|
|
||
| extension OCDisplayExtension { | ||
|
|
||
| static func normalPDFExtension(identifier: String) -> OCDisplayExtension { |
There was a problem hiding this comment.
This static/class method could be moved to the PDFViewerViewController class method/property extension to make it more self-contained.
| return normalPDFExtension | ||
| } | ||
|
|
||
| static func webViewExtension(identifier: String) -> OCDisplayExtension { |
There was a problem hiding this comment.
This static/class method could be moved to the WebViewDisplayViewController class method/property extension to make it more self-contained.
| @@ -18,16 +18,21 @@ | |||
|
|
|||
| import UIKit | |||
| import ownCloudSDK | |||
| import QuickLook | |||
| import ObjectiveC | |||
There was a problem hiding this comment.
These two imports are probably a leftover from an experiment. They're not needed and can be removed.
| @@ -18,6 +18,7 @@ | |||
|
|
|||
| import UIKit | |||
| import ownCloudSDK | |||
| import QuickLook | |||
There was a problem hiding this comment.
This import also is probably a leftover from an experiment. It's not needed and can be removed.
|
|
||
| struct FeatureKeys { | ||
| static let canEdit: String = "featureKeyCanEdit" | ||
| static let canSave: String = "featureKeyCanSave" |
There was a problem hiding this comment.
Thinking about it, editing only makes sense if one can also save the results. So I think the canSave feature can be removed.
|
|
||
| static var features: [String : Any]? {get} | ||
| static var supportedMimeTypes: [String] {get} | ||
| } |
There was a problem hiding this comment.
You could also add displayExtensionIdentifier here, so all info to build the OCExtension can be provided by protocol-adopting classes.
| import UIKit | ||
| import ownCloudSDK | ||
|
|
||
| protocol DisplayViewProtocol where Self: DisplayViewController { |
There was a problem hiding this comment.
For increased clarity, I'd like to suggest naming this DisplayExtension, since Display and View are two words for almost the same thing and it's unusual to name a protocol Protocol.
|
|
||
| static var features: [String : Any]? = [FeatureKeys.canEdit : true] | ||
|
|
||
| static func webViewExtension(identifier: String) -> OCExtension { |
There was a problem hiding this comment.
Seeing how this method and normalPDFExtension are almost identical, I can see how this could be unified and simplified, f.ex. with an extension to DisplayViewProtocol like this:
extension DisplayViewProtocol {
static var displayExtension: OCExtension {
// Code generating OCExtension from `supportedMIMETypes`, `features` and `displayExtensionIdentifier` goes here.
}
}Then, adding the extension to the extension manager can be done with one line
OCExtensionManager.shared.addExtension(PDFViewerViewController.displayExtension)and everything related to the respective viewer (incl. ID) is isolated in its own class.
| for mimeType in supportedMimeTypes { | ||
| let locationIdentifier: OCExtensionLocationIdentifier = OCExtensionLocationIdentifier(rawValue: mimeType) | ||
| locationIdentifiers.append(locationIdentifier) | ||
| } |
There was a problem hiding this comment.
Given that OCExtensionLocationIdentifiers are also just Strings… do you know of a more direct way to cast this? I couldn't immediately find any.
There was a problem hiding this comment.
I've been looking for some other option and I found some info about how NS_TYPED_EXTENSIBLE_ENUM is imported into Swift: Declare Typed Extensible Enumerations
The solution they propose is to create an extension with a static computed variable for each new case you want to add. Something like:
extension OCExtensionLocationIdentifier {
static var pdf: OCExtensionLocationIdentifier {
return OCExtensionLocationIdentifier(rawValue: "application/pdf")
}
}I think this is not extensible for our use case because we could have a lot of mime types.
I coded the OCExtension creation in a DisplayExtension extension that provides the default implementation, so this code is only written once. It could be overridden in the future by any class that implements this protocol.
static var displayExtension: OCExtension {
let rawIdentifier: OCExtensionIdentifier = OCExtensionIdentifier(rawValue: displayExtensionIdentifier)
var locationIdentifiers: [OCExtensionLocationIdentifier] = []
for mimeType in supportedMimeTypes {
let locationIdentifier: OCExtensionLocationIdentifier = OCExtensionLocationIdentifier(rawValue: mimeType)
locationIdentifiers.append(locationIdentifier)
}
let displayExtension = OCExtension(identifier: rawIdentifier, type: .viewer, locations: locationIdentifiers, features: features) { (_ rawExtension, _ context, _ error) -> Any? in
return Self()
}
return displayExtension!
}
}|
|
||
| static var displayExtensionIdentifier: String = "normalPDFViewer" | ||
| static var supportedMimeTypes: [String] = ["application/pdf"] | ||
| static var features: [String : Any]? = [FeatureKeys.canEdit : true] |
There was a problem hiding this comment.
This should be false, since it can't perform any editing.
| "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet", | ||
| "application/vnd.openxmlformats-officedocument.wordprocessingml.document"] | ||
|
|
||
| static var features: [String : Any]? = [FeatureKeys.canEdit : true] |
There was a problem hiding this comment.
This should be false, since it can't perform any editing.
|
|
||
| class PDFViewerViewController: DisplayViewController, DisplayExtension { | ||
|
|
||
| static var displayExtensionIdentifier: String = "normalPDFViewer" |
There was a problem hiding this comment.
Let's use reverse-domain-name-style identifiers here, like f.ex. org.owncloud.pdfViewer.default (Michael's PDF viewer could then be added as org.owncloud.pdfViewer.extended, and something else as com.othercorp.pdfViewer).
|
|
||
| class WebViewDisplayViewController: DisplayViewController, DisplayExtension { | ||
|
|
||
| static var displayExtensionIdentifier: String = "imageViewer" |
There was a problem hiding this comment.
Analogous to PDFViewerViewController, a reverse-domain-name-style identifier here could be org.owncloud.webview.
| static var supportedMimeTypes: [String] = | ||
| ["image/jpeg", | ||
| "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet", | ||
| "application/vnd.openxmlformats-officedocument.wordprocessingml.document"] |
There was a problem hiding this comment.
Please also add MIME Types for other supported formats. As far as I can tell, https://github.com/owncloud/core/blob/master/core/js/mimetypelist.js should be an exhaustive list of what to expect from the OC server.
Thinking about it, this isn't as future-proof as it could be.
So in the latest commit I added support for a customMatcher to OCExtension. That block gets the OCExtensionContext and the OCExtensionPriority computed by the default implementation. To add wild-card matching for f.ex. image/*, video/*, audio/* and text/*, all one would need to do is add a customMatcher block to the OCExtension instance representing WebViewDisplayViewController.
In that block, you'd need to check if the provided OCExtensionPriority is OCExtensionPriorityNoMatch. And if it is, check if the context's location.locationIdentifier begins with image/, video/, audio/ or text/. If it is, the block could return OCExtensionPriorityLocationMatch, otherwise return OCExtensionPriorityNoMatch.
That block could be provided to DisplayExtension.displayExtension by adding another property to return a customMatcher.
That said, adding those MIME Types from https://github.com/owncloud/core/blob/master/core/js/mimetypelist.js that do not match one of the "wildcards" should still be done, especially for Office formats like PowerPoint.
pablocarmu
force-pushed
the
feature/filePreviewing
branch
from
84af6c7
to
5788427
Compare
|
I've made a few minor changes that I thought were quicker to write in code than in text:
Using it for a bit, I also noticed a few other things that still need attention:
|
Right now the item it's downloaded and the thumbnail or icon (if any) is presented. Like when you are trying to open a supported file but without opening the file. There are some possibilities here: A. Show the thumbnail with a "non-supported file" message below the thumbnail, without download the file. IMO the option A should be the one to go, what do you guys think? @felix-schwarz @mneuwert @jesmrec @javiergonzper @michaelstingl |
|
@pablocarmu I would vote against C at least. I think that the decision if the file has to be downloaded shall depend on what actions can be performed with the downloaded file (share, copy into Files app, print or whatever). Besides, even if we can’t show file preview we could still try to show some interesting information along with the thumbnail icon: e.g. file size, meta-data etc since we have more room for that in the detail view. Sent with GitHawk |
|
I'd also go with A. If a file is tapped for preview and can't be shown, there's no benefit in downloading it. Regarding further actions to act on a local copy of the file: I think that once the user triggers such an action would be the right time to trigger the download. Conveniently, you can always just call @pablocarmu @mneuwert @jesmrec @javiergonzper @michaelstingl |
|
A is the option i'd prefer. |
pablocarmu
force-pushed
the
feature/filePreviewing
branch
2 times, most recently
from
ec2a94e
to
c175ce5
Compare
There was a problem hiding this comment.
Looks good! 👍
Only (really tiny) thing I noticed was the use of "Ok". On Apple's platforms this should always be "OK". Fixed with a tiny commit & approved!
P.S.: one final thing before merging this PR: when feature/filemgmt has been merged into the SDK master, please update the ios-sdk git subrepo to the latest commit on the SDK master.
|
Lest's QA a bit (1) [FIXED]What do we expect in the following scenario?
Current: Download view static, no notification to the user |
(2) [FIXED]Formats that are not opened:
|
|
Pics are shown alligned to the top on the screen. We should improve such behaviour when we develop #85 |
These formats are not opened by default by the web view so I'd propose to make a new viewer using UITextView for these formats. (we can write the details in a separate ticket)
These formats are not supported by the server based on what I can see here https://github.com/owncloud/core/blob/master/core/js/mimetypelist.js |
| scrollView!.leftAnchor.constraint(equalTo: view.safeAreaLayoutGuide.leftAnchor), | ||
| scrollView!.rightAnchor.constraint(equalTo: view.safeAreaLayoutGuide.rightAnchor), | ||
| scrollView!.bottomAnchor.constraint(equalTo: view.safeAreaLayoutGuide.bottomAnchor), | ||
| scrollView!.topAnchor.constraint(equalTo: view.safeAreaLayoutGuide.topAnchor) |
There was a problem hiding this comment.
By using the safeAreaLayoutGuide, the scroll view does only fill part of the available space and is "cut off" on the bottom on iPhone X:
| imageView!.leftAnchor.constraint(equalTo: scrollView!.leftAnchor), | ||
| imageView!.rightAnchor.constraint(equalTo: scrollView!.rightAnchor), | ||
| imageView!.bottomAnchor.constraint(equalTo: scrollView!.bottomAnchor), | ||
| imageView!.topAnchor.constraint(equalTo: scrollView!.topAnchor) |
There was a problem hiding this comment.
Using constraints to center the imageView inside the scrollview works initially, but the zoom will also scale up the blank space around the image later. Scrolling around while zoomed into the image, then, can give you this result:
You can use UIScrollView.contentInsets and adapt these in UIScrollViewDelegate's scrollViewDidZoom(_ scrollView: UIScrollView) as zoom levels change to increase or decrease insets value. This Stackoverflow question gives a few examples on how that could be done.
| // Created by Pablo Carrascal on 17/10/2018. | ||
| // Copyright © 2018 ownCloud GmbH. All rights reserved. | ||
| // | ||
|
|
There was a problem hiding this comment.
Please add the standard copyright notice.
ok, the point is that such formats (but .avi) try to be opened in the app, pointing to a white screen:
same behaviour in case of .odt files xml, tex, avi -> correct behaviour for non-supported formats |
…features and supported mimetypes. - Created a new class called DisplayViewController to share the thumbnail code when the image it's not downloaded yet. - Refactored the WebViewDisplayViewController and PDFDisplayViewController to make them DisplayViewController's subclasses.
- Remove unneeded main thread calls in DisplayHostViewController.. - Perform a better view controller selection in DisplayHostViewController.
…playViewController.
- Removed unneeded code from old implementation about QLPreview. - Removed commented code for downloads.
…oller - Update SDK
- Delete app's own mechanism for extensions. - Make use of the new extension APIs supporting multiple location identifiers. - Place the static init methods for each extension in its own class instead of an OCExtension extension.
- Removed one unneded feature key (canSave). - Renamed DisplayViewProtocol to DisplayExtension. - Make the default initialization of previews as a protocol extension.
- Coded the extensionIdentifierst to a more standard system like com.owncloud.foo.bar - SDK updated. - Created custom matchers for webview supported mimetypes.
- Added progress bar and cancel button to DisplayViewController - Added theming support to DisplayViewController (for button, progress bar) - Extended NSObject+ThemeApplication with support for theming UIProgressViews - Extended WebCViewDisplayViewController regex to also match audio and video MIME types - Added missing copyright texts and other minor cleanups
- Fixing newly introduced retain cycle in DisplayHostViewController - Updated ios-sdk with important memory-related fixes
mimy type is supported by the extensions mechanism. - Manage the errors when there is some failure in the download.
- Coded some metadata of the item if you cannot preview it.
- Make the preview available again when the user cancels it.
retention cycle. - Localize "Show Preview" string.
… process. - Make the progressView tracked progress to 0 when hided to avoid UI glitches. - Change "Show Preview" to "Open File".
- Coded new KVO for core's reachability in DisplayViewController to be able to manage non-network states. - New UI for non-network states in DisplayViewController.
- Removed the abailability for preview images in WebViewDisplayViewController.
…pport them. (wmv, ogg, avi, flv) - Made some image mimetypes to open inside WKWebView (svg, gif)
pablocarmu
force-pushed
the
feature/filePreviewing
branch
from
37a9365
to
a6f2421
Compare
Description
Make the app able to automatically select the view controller to present based on the mime-type of the item to open.
Related Issue
#112
Motivation and Context
This PR takes advantage of the Extension mechanism in the SDK so we can change between implementations easily.
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist:
BUGS & IMPROVEMENTS