Fix: honor "exact" seek in user bindings #3732
Conversation
|
I'm going to defer to @lhc70000 on the code changes as it has been proven that I don't know this part of the code well enough to comment. But I do want to comment on this:
Indeed I remember reading that one large company specifies that use of the swift forced unwrap operator is a code smell. I have a different view. I think of force unwrapping as a sanity check and like most sanity checks it will result in a fatal error, in this case a crash. Therefore in the case where an optional must be non-nil for the code to function as intended, I think of using ? instead of ! as removing a sanity check. That can turn a crash that might be noticed during development into subtle malfunctioning behavior that isn't noticed until some user encounters the obscure problem. So now a user has reported the problem. If the application had crashed and the user provided the crash report there is a good chance that points to the source of the problem. But it didn't crash and we were not able to reproduce the malfunction. We suspect something was nil when it shouldn't have been and are desk checking the code. Think of every ? as an if statement. The code is now filled with many different execution paths. Which property was nil when it shouldn't be? For this reason I don't see avoiding the force unwrapping operator as a black and white issue. I don't think it is appropriate to always consider it a code smell. In primary features that persist data you might prefer the application crashes rather than corrupt a database. In secondary features of the application you might prefer the feature malfunctions rather than take down the application. My impression is that mine is a minority viewpoint. The "fear of crash" philosophy is winning. I have been reducing my use of the force unwrap operator. With respects to IINA, we need to hear what @lhc70000 thinks about use of ! and follow his guidance. |
|
Not @lhc70000 but +1 to the use of force unwrapping when appropriate |
|
Re: use of force-unwrapping issue, yeah, I can understand the need to fail fast, but bringing down the whole program seems like harsh punishment! Maybe adding an "else" and writing an error to the log would be good? Although that does tend to clutter up the code... anyway I'll go with the official decree whatever that may be. |
|
My comment is about the push for an absolute ban on use of forced unwrapping which seems to be taking hold in some parts of the industry. Not an opposition to use of optional chaining. Just trying to bring awareness to the downsides of excessive use of optional chaining. |
|
Rebranding this one so it doesn't get lost. (Excited by the 1.3.0 release by the way! It will be a huge help). Yes, this isn't a crasher but I'd like to push for it more than anything else, since it's very high visibility, it's not very complicated, and it's only been partially fixed. The original issue was that forward/backward seeks which qualified for menu items were being hard-coded to Let's say there is a video with a keyframe only every 10s. This is how Point A is at I don't like keyframe seeks. The MPV team seemed to make this the default because they were worried about performance. And they may have been a valid concern 10 or 15 years ago, but IINA has the luxury of assuming relatively modern Mac hardware. I've been using |
|
Yes, there are still lots of interesting proposed changes outstanding. Merging will continue. |
089e485 to
6a50430
Compare
6a50430 to
dc3aabb
Compare
|
Didn't realize this had fallen into conflict. Fixed and rebased. Really hope this can make it into develop soon. |
…ct" and exact numbers as specified
dc3aabb to
74282bc
Compare
@low-batt do you have the ability to commit your changes? I don't know if I will have time in the next few days. |
|
No need to commit those changes. This PR does not have conflicts preventing merge. Those changes are only needed if you want to build and test this PR. The changes are quite simple. Anyway, don't worry, I can take care of it should the other reviewers want changes. |
There was a problem hiding this comment.
I have a problem compiling on this PR after rebasing to develop. On line 901 of MenuController.swift, Xcode doesn't let me compile
let (_, value) = sameKeyAction(actions, actions, normalizeLastNum, numRange)The error message is "Cannot convert value of type '(Bool, Double?, Any?)' to specified type '(_, _)'". So I have to change it to:
let value = sameKeyAction(actions, actions, normalizeLastNum, numRange).1| if let args = sender.representedObject as? (Double, Preference.SeekOption) { | ||
| player.seek(relativeSecond: args.0, option: args.1) | ||
| } else { | ||
| player.seek(relativeSecond: 5, option: Preference.SeekOption.defaultValue) |
There was a problem hiding this comment.
You forgot to consider sender.tag, if sender.tag == 1 the relativeSecound should be -5. When the menu item is not matched by keybindings, we will have forward and backward menu items by default.
| } else { | ||
| // matching values are one of: "relative", "keyframes", "exact", "relative+keyframes", "relative+exact" | ||
| let splitArray = last.split(whereSeparator: { $0 == "+" }) | ||
| if splitArray.count == 2 && splitArray[0] == "relative" { |
There was a problem hiding this comment.
Here you assumed "relative" always appears first, however it seems things like "keyframes+relative" are also valid mpv flags. I tried to add seek -5 keyframes+relative and seek 5 relative+keyframes, both shortcuts can be used in IINA, but only the latter one is recognized as the same action as in the default menu.
|
As @svobs is busy I will attempt to address the feedback. |
seek exactare silently overridden toseek keyframes#3710.Description:
I rewrote the fix to this issue to minimally impact the existing code and make it more robust (e.g. using ? instead of !)