Fix screenshot crash with long video title, #3334 #3469
Conversation
iina/MPVController.swift
Outdated
| @@ -664,6 +664,18 @@ class MPVController: NSObject { | |||
| case MPV_EVENT_COMMAND_REPLY: | |||
| let reply = event.pointee.reply_userdata | |||
| if reply == MPVController.UserData.screenshot { | |||
| let code = event.pointee.error | |||
| guard 0 <= code else { | |||
There was a problem hiding this comment.
I think I would prefer the condition to be flipped
| guard 0 <= code else { | |
| guard code >= 0 else { |
iina/MPVController.swift
Outdated
| @@ -664,6 +664,18 @@ class MPVController: NSObject { | |||
| case MPV_EVENT_COMMAND_REPLY: | |||
| let reply = event.pointee.reply_userdata | |||
| if reply == MPVController.UserData.screenshot { | |||
| let code = event.pointee.error | |||
| guard 0 <= code else { | |||
| let error = String.init(cString: mpv_error_string(code)) | |||
There was a problem hiding this comment.
| let error = String.init(cString: mpv_error_string(code)) | |
| let error = String(cString: mpv_error_string(code)) |
There was a problem hiding this comment.
I see my inexperience with Swift is showing. Fixed.
iina/Utility.swift
Outdated
| @@ -461,7 +461,7 @@ class Utility { | |||
| guard let contents = try? FileManager.default.contentsOfDirectory( | |||
| at: folder, | |||
| includingPropertiesForKeys: [.creationDateKey], | |||
| options: .skipsSubdirectoryDescendants) else { return nil } | |||
| options: .skipsSubdirectoryDescendants), !contents.isEmpty else { return nil } | |||
There was a problem hiding this comment.
| options: .skipsSubdirectoryDescendants), !contents.isEmpty else { return nil } | |
| options: .skipsSubdirectoryDescendants), | |
| let latestFile = contents.first else { return nil } |
There was a problem hiding this comment.
Fixed. I did have to use var instead of let as the code below this updates latestFile.
There was a problem hiding this comment.
Ah, I see. I would probably replace the code below to use min(by:) with a custom comparator, to be honest, but it's up to you if you want to change that here.
There was a problem hiding this comment.
That was existing code that I thought could be simplified, but I chose to make the minimum change to fix the issue. I have just pushed a commit that simplifies this method. See if that is what you were expecting.
iina/Utility.swift
Outdated
| return latestFile | ||
| options: .skipsSubdirectoryDescendants), | ||
| !contents.isEmpty else { return nil } | ||
| return contents.max { a, b in a.creationDate! < b.creationDate! } |
There was a problem hiding this comment.
| return contents.max { a, b in a.creationDate! < b.creationDate! } | |
| return contents.filter { $0.creationDate != nil }.max { $0.creationDate! < $1.creationDate! } |
Or if you are sure that creationDate would never be nil, you can remove the filter clause.
There was a problem hiding this comment.
Fixed. Rebased with develop. Tested again. I kept the filter. Made me worry that someone could put the screen shot directory on some network device that doesn't provide it.
This commit will:
- Change the method MPVController.handleEvent to raise an alert if
screenshot generation failed
- Change the method Utility.getLatestScreenshot to guard against an empty
screenshot directory
This commit adds new text for the alert that will require localization.
This commit will address review comments on the changes to the methods MPVController.handleEvent and Utility.getLatestScreenshot.
This commit will: - Extend URL with a new optional computed property, creationDate - Change the method Utility.getLatestScreenshot to use the array max method to find the latest screenshot file
Rebased with develop and addressed review comment. This commit will: - Extend URL with a new optional computed property, creationDate - Change the method Utility.getLatestScreenshot to use the array max method to find the latest screenshot file
This commit will:
screenshot generation failed
screenshot directory
This commit adds new text for the alert that will require localization.
Description:
NOTE the need for localization of the added alert message text.