From f91e377c42a2812cd5d05aa92d108c7389f3e17b Mon Sep 17 00:00:00 2001 From: Sindre Sorhus Date: Wed, 10 Apr 2019 13:43:04 +0700 Subject: [PATCH] Improve error message for audio-only files --- Gifski/AppDelegate.swift | 5 +- Gifski/Base.lproj/MainMenu.xib | 11 ++- Gifski/Gifski.swift | 19 +++++ Gifski/MainWindowController.swift | 38 +++++++-- Gifski/util.swift | 137 ++++++++++++++++++++---------- 5 files changed, 151 insertions(+), 59 deletions(-) diff --git a/Gifski/AppDelegate.swift b/Gifski/AppDelegate.swift index 9e4bc32a..4412316b 100644 --- a/Gifski/AppDelegate.swift +++ b/Gifski/AppDelegate.swift @@ -38,8 +38,7 @@ final class AppDelegate: NSObject, NSApplicationDelegate { guard urls.count == 1 else { NSAlert.showModal( for: mainWindowController.window, - title: "Max one file", - message: "You can only convert a single file at the time" + message: "Gifski can only convert a single file at the time." ) return } @@ -61,7 +60,7 @@ final class AppDelegate: NSObject, NSApplicationDelegate { } func application(_ application: NSApplication, willPresentError error: Error) -> Error { - Crashlytics.sharedInstance().recordErrorBetter(error) + Crashlytics.recordNonFatalError(error: error) return error } } diff --git a/Gifski/Base.lproj/MainMenu.xib b/Gifski/Base.lproj/MainMenu.xib index d186dfdb..f4dc0d47 100644 --- a/Gifski/Base.lproj/MainMenu.xib +++ b/Gifski/Base.lproj/MainMenu.xib @@ -1,7 +1,8 @@ - + - + + @@ -141,24 +142,26 @@ - + - + + + diff --git a/Gifski/Gifski.swift b/Gifski/Gifski.swift index 202af2f5..56fd6b60 100644 --- a/Gifski/Gifski.swift +++ b/Gifski/Gifski.swift @@ -1,5 +1,6 @@ import Foundation import AVFoundation +import Crashlytics final class Gifski { enum Error: LocalizedError { @@ -87,6 +88,24 @@ final class Gifski { DispatchQueue.global(qos: .utility).async { let asset = AVURLAsset(url: conversion.input, options: nil) + + Crashlytics.record( + key: "Does input file exist", + value: conversion.input.exists + ) + Crashlytics.record( + key: "Is input file reachable", + value: try? conversion.input.checkResourceIsReachable() + ) + Crashlytics.record( + key: "Is input file readable", + value: conversion.input.isReadable + ) + Crashlytics.record( + key: "AVAsset debug info 2", + value: asset.debugInfo + ) + let generator = AVAssetImageGenerator(asset: asset) generator.requestedTimeToleranceAfter = .zero generator.requestedTimeToleranceBefore = .zero diff --git a/Gifski/MainWindowController.swift b/Gifski/MainWindowController.swift index b8e68793..ea0cb1f8 100644 --- a/Gifski/MainWindowController.swift +++ b/Gifski/MainWindowController.swift @@ -105,11 +105,27 @@ final class MainWindowController: NSWindowController { func convert(_ inputUrl: URL) { let asset = AVURLAsset(url: inputUrl) + Crashlytics.record(key: "AVAsset debug info", value: asset.debugInfo) + guard asset.videoCodec != "rle" else { NSAlert.showModal( for: window, - title: "QuickTime Animation format not supported", - message: "Re-export or convert your video to ProRes 4444 XQ instead. It's more efficient, more widely supported, and like QuickTime Animation, it also supports alpha channel. To convert an existing video, just open it in QuickTime Player (which will convert it) and then save it." + message: "The QuickTime Animation format is not supported.", + informativeText: "Re-export or convert your video to ProRes 4444 XQ instead. It's more efficient, more widely supported, and like QuickTime Animation, it also supports alpha channel. To convert an existing video, open it in QuickTime Player, which will automatically convert it, and then save it." + ) + return + } + + if asset.hasAudio && !asset.hasVideo { + NSAlert.showModal( + for: window, + message: "Audio files are not supported.", + informativeText: "Gifski converts video files but the provided file is audio-only. Please provide a file that contains video." + ) + + Crashlytics.recordNonFatalError( + title: "Audio files are not supported.", + message: asset.debugInfo ) return } @@ -118,22 +134,28 @@ final class MainWindowController: NSWindowController { guard asset.isVideoDecodable else { NSAlert.showModal( for: window, - title: "Video file not supported", - message: "The video file you tried to convert could not be read. Please open an issue on https://github.com/sindresorhus/gifski-app. ZIP the video and attach it to the issue.\n\nInclude this info:\n\(asset.debugInfo)" + message: "The video file is not supported.", + informativeText: "Please open an issue on https://github.com/sindresorhus/gifski-app. ZIP the video and attach it to the issue.\n\nInclude this info:\n\(asset.debugInfo)" ) - Crashlytics.sharedInstance().recordErrorMessage("Video file not supported: \(asset.debugInfo)") + Crashlytics.recordNonFatalError( + title: "The video file is not supported.", + message: asset.debugInfo + ) return } guard let videoMetadata = asset.videoMetadata else { NSAlert.showModal( for: window, - title: "Video metadata not readable", - message: "The metadata of the video could not be read. Please open an issue on https://github.com/sindresorhus/gifski-app. ZIP the video and attach it to the issue.\n\nInclude this info:\n\(asset.debugInfo)" + message: "The video metadata is not readable.", + informativeText: "Please open an issue on https://github.com/sindresorhus/gifski-app. ZIP the video and attach it to the issue.\n\nInclude this info:\n\(asset.debugInfo)" ) - Crashlytics.sharedInstance().recordErrorMessage("Video metadata not readable: \(asset.debugInfo)") + Crashlytics.recordNonFatalError( + title: "The video metadata is not readable.", + message: asset.debugInfo + ) return } diff --git a/Gifski/util.swift b/Gifski/util.swift index 4f59b58d..9ea1e885 100644 --- a/Gifski/util.swift +++ b/Gifski/util.swift @@ -237,59 +237,59 @@ extension NSWindowController: NSWindowDelegate { extension NSAlert { - /// Show a modal alert sheet on a window - /// If the window is nil, it will be a app-modal alert + /// Show a modal alert sheet on a window. + /// If the window is nil, it will be a app-modal alert. @discardableResult static func showModal( for window: NSWindow?, - title: String, - message: String? = nil, - style: NSAlert.Style = .critical + message: String, + informativeText: String? = nil, + style: NSAlert.Style = .warning ) -> NSApplication.ModalResponse { guard let window = window else { return NSAlert( - title: title, message: message, + informativeText: informativeText, style: style ).runModal() } return NSAlert( - title: title, message: message, + informativeText: informativeText, style: style ).runModal(for: window) } - /// Show a app-modal (window indepedendent) alert + /// Show a app-modal (window indepedendent) alert. @discardableResult static func showModal( - title: String, - message: String? = nil, - style: NSAlert.Style = .critical + message: String, + informativeText: String? = nil, + style: NSAlert.Style = .warning ) -> NSApplication.ModalResponse { return NSAlert( - title: title, message: message, + informativeText: informativeText, style: style ).runModal() } convenience init( - title: String, - message: String? = nil, - style: NSAlert.Style = .critical + message: String, + informativeText: String? = nil, + style: NSAlert.Style = .warning ) { self.init() - self.messageText = title + self.messageText = message self.alertStyle = style - if let message = message { - self.informativeText = message + if let informativeText = informativeText { + self.informativeText = informativeText } } - /// Runs the alert as a window-modal sheel + /// Runs the alert as a window-modal sheet. @discardableResult func runModal(for window: NSWindow) -> NSApplication.ModalResponse { beginSheetModal(for: window) { returnCode in @@ -614,6 +614,16 @@ extension AVAsset { return firstVideoTrack.isDecodable } + /// Returns a boolean of whether there are any video tracks. + var hasVideo: Bool { + return !tracks(withMediaType: .video).isEmpty + } + + /// Returns a boolean of whether there are any audio tracks. + var hasAudio: Bool { + return !tracks(withMediaType: .audio).isEmpty + } + /// Returns the first video track if any. var firstVideoTrack: AVAssetTrack? { return tracks(withMediaType: .video).first @@ -1275,6 +1285,14 @@ extension URL { return values.allValues[key] as? T } + private func boolResourceValue(forKey key: URLResourceKey, defaultValue: Bool = false) -> Bool { + guard let values = try? resourceValues(forKeys: [key]) else { + return defaultValue + } + + return values.allValues[key] as? Bool ?? defaultValue + } + /// File UTI var typeIdentifier: String? { return resourceValue(forKey: .typeIdentifierKey) @@ -1288,6 +1306,14 @@ extension URL { var fileSizeFormatted: String { return ByteCountFormatter.string(fromByteCount: Int64(fileSize), countStyle: .file) } + + var exists: Bool { + return FileManager.default.fileExists(atPath: path) + } + + var isReadable: Bool { + return boolResourceValue(forKey: .isReadableKey) + } } extension CGSize { @@ -1693,42 +1719,64 @@ extension Error { extension NSError { // TODO: Return `Self` here in Swift 5.1 class func from(error: Error, userInfo: [String: Any] = [:]) -> NSError { + let nsError = error as NSError + // Since Error and NSError are often bridged between each other, we check if it was originally an NSError and then return that. guard !error.isNsError else { guard !userInfo.isEmpty else { - return error as NSError + return nsError } // TODO: Use `Self` instead of `NSError` here in Swift 5.1 - return (error as NSError).appending(userInfo: userInfo) + return nsError.appending(userInfo: userInfo) } var userInfo = userInfo userInfo[NSLocalizedDescriptionKey] = error.localizedDescription + // This is needed as `localizedDescription` often lacks important information, for example, when an NSError is wrapped in a Swift.Error. + userInfo["Swift.Error"] = "\(nsError.domain).\(error)" + return self.init( - domain: "\(App.id) - \((error as NSError).domain)", - code: 0, + domain: "\(App.id) - \(nsError.domain)", + code: nsError.code, userInfo: userInfo ) } - class func appError(message: String, userInfo: [String: Any] = [:]) -> Self { + /** + - Parameter domainPostfix: String to append to the `domain`. + */ + class func appError(message: String, userInfo: [String: Any] = [:], domainPostfix: String? = nil) -> Self { return self.init( - domain: App.id, + domain: domainPostfix != nil ? "\(App.id) - \(domainPostfix!)" : App.id, code: 0, userInfo: [NSLocalizedDescriptionKey: message] ) } - /// Returns a new error with the user info appended - func appending(userInfo: [String: Any]) -> Self { - var currentUserInfo = userInfo - for (key, value) in userInfo { - currentUserInfo[key] = value - } + /// Returns a new error with the user info appended. + func appending(userInfo newUserInfo: [String: Any]) -> Self { // TODO: Use `Self` here in Swift 5.1 - return type(of: self).init(domain: domain, code: code, userInfo: currentUserInfo) + return type(of: self).init( + domain: domain, + code: code, + userInfo: userInfo.appending(newUserInfo) + ) + } +} + +extension Dictionary { + /// Adds the elements of the given dictionary to a copy of self and returns that. + /// Identical keys in the given dictionary overwrites keys in the copy of self. + func appending(_ dictionary: [Key: Value]) -> [Key: Value] { + var newDictionary = self + + for (key, value) in dictionary { + newDictionary[key] = value + } + + return newDictionary } } @@ -1737,24 +1785,25 @@ extension NSError { extension Crashlytics { /// A better error recording method. Captures more debug info. - func recordErrorBetter(_ error: Error) { + static func recordNonFatalError(error: Error, userInfo: [String: Any] = [:]) { #if !DEBUG // This forces Crashlytics to actually provide some useful info for Swift errors - let nsError = NSError.from(error: error) - - recordError( - nsError, - withAdditionalUserInfo: [ - "userInfo": nsError.userInfo, - "localizedDescription": nsError.localizedDescription - ] - ) + let nsError = NSError.from(error: error, userInfo: userInfo) + + sharedInstance().recordError(nsError) + #endif + } + + static func recordNonFatalError(title: String? = nil, message: String) { + #if !DEBUG + sharedInstance().recordError(NSError.appError(message: message, domainPostfix: title)) #endif } - func recordErrorMessage(_ message: String) { + /// Set a value for a for a key to be associated with your crash data which will be visible in Crashlytics. + static func record(key: String, value: Any?) { #if !DEBUG - recordError(NSError.appError(message: message)) + sharedInstance().setObjectValue(value, forKey: key) #endif } }