Implement repeat file #4350
Implement repeat file #4350
Conversation
There was a problem hiding this comment.
I am very concerned that we seem to be trying to hide the fact that mpv has two separate options, loop-playlist and loop-file. With the changes in the commit, turning off loop-file changes the value of loop-playlist. That must not be the case. IINA should strive to fully support mpv functionality, but certainly should not interfere with the user's ability to to use mpv features not supported by IINA. Currently IINA falls short in its support for both of these options as they are treated like booleans even though that is not the type of these properties. But what is great about IINA is that mpv features not available in IINA's UI are still available to users. The following sets loop-playlist to force:
low-batt@gag ~$ echo '{ "command": ["set_property", "loop-playlist", "force"] }' | socat - /tmp/mpv-socket | jq
{
"request_id": 0,
"error": "success"
}With IINA 1.3.1 changing loop-file will not overwrite that loop-playlist setting. Fine for IINA to not support the full functionality of these two mpv options, but combining them such that changing one changes the other is inappropriate.
In testing I clicked on the playlist button and an OSD message Playlist Loop: On appeared. I clicked a second time and an OSD message saying Playlist Loop: On appeared. I immediately thought the button is broken. That needs to be corrected. I'm seeing other incorrect OSD behavior as well. Of course I believe this should be corrected by keeping these two mpv settings separate.
|
Firstly, let's postpone this to 1.4.0, so that OSD related issues can also be resolved. I was mainly thinking about the UX part, since most of other players (like iTunes) are just using one button for looping, clicking on it cycles between three states. Introducing two buttons will cause some confusion IMO. When the 'loop-file' option is on, is there any difference whether 'loop-playlist' is on or off? If there is no difference, I will change it back to 2 buttons, and when 'loop-file' is on, I'll disable the 'loop-playlist' button. Does this sound better to you? |
|
Postponing sounds good. The problem is that mpv deviated from the simple off/on settings that other applications provide. Disabling the button troubles me as it means IINA does not reflect the behavior and state of mpv. The loop playlist button tells you whether the playlist will loop or not once you turn off loop file. And of course the user can change that setting at any time using the mpv IPC interface. If we disable the button then if the user uses the IPC interface to change the mvp property there is no feedback in the IINA UI. The menu items should match up with the buttons. So if we disable the loop playlist button I'd expect the loop playlist menu item to also be disabled. Think about what the IINA UI would be like if we fully supported these controls. In other words allowed the user to set loop counts. Gave them a way to set loop playlist to |
|
To make this more understandable to the user we could add a border with arrows similar to the arrows in the button icon. When loop playlist is enabled this border would be around the entire playlist. When loop file is enabled the border would be around the file that is currently playing. If both of these are enabled then only the border around loop file would be shown. When loop file is disabled then the border would switch to the entire playlist. Something like that. |
If we want to fully support all functionalities provided by mpv, then we can accomadate the loop options in a context menu when right clicking on the icon.
I don't quite understand your proposal. Is the "border" you mentioned persisent or just active when changing the loop mode? Can you make a quick mockup for that? |
|
I'm thinking this would be something that mimics the curved arrows in the loop icon. The one for loop-file would always be shown while looping is active. We'd want to make this use small thin lines and be some level of grey. The idea is to make it not be distracting so it is acceptable to always display it while looping is active. The one for loop-playlist would also always be shown while looping is active, unless loop-file is active. When loop-file is deactivated it would be shown again if loop-playlist is active. Something like the following, but with curved lines like in the button and of course more subtle: |
|
I think this kind of indicator is too much for such for looping functionality. Could you find some other media player does something like this? |
|
Yes, my mockup certainly does not look good. I do remember reading a post about a player that did a box kind of like that except it was on the control slider. It was how they showed an a-b loop. VLC has separate menu items The problem here is that mpv decided to offer fancy looping features. I'm critical of the |
|
While I do think to fully support the mpv options (repeat times, information, force) is a good thing, but it's very hard to make UI in such a small area. Maybe we can provide UI elements to set detailed options in other place. To reflect the internal mpv state of |
|
fwiw I don't mind the IINA UI not showing the full gamut of what mpv lets you set, as long as we don't clobber the actual setting under the hood. I think an iTunes like none/loop one/loop playlist is probably fine. |
* PlaylistView: Store preference for playlist loop status and add a `loop file` button * Update lproj according to `CONTRIBUTING.md` --------- Co-authored-by: 李通洲 <carter.li@eoitek.com>
Change the cycle to: no loop -> file loop -> playlist loop
|
I tested the latest commit and things are definitely broken. Looks like there is trouble with presenting the correct OSD message and coordinating with the Clicking the button on the playlist panel the OSD message says I clicked on Even though we are not going to support setting counts in the UI it would be nice if the button and menu item state reflected the feature was active when the user has set a count. This is what I was executing to test that with echo '{ "command": ["set_property", "loop-playlist", "2"] }' | socat - /tmp/mpv-socket | jqWe can postpone dealing with this to a future release if it is not easy to implement. |
There was a problem hiding this comment.
Menus and OSD worked for me. But why not change PlayerCore.getLoopMode to look something like I show in my comment. That way if the user sets a count:
echo '{ "command": ["set_property", "loop-file", "2"] }' | socat - /tmp/mpv-socket | jq
The menus and playlist button properly reflect that looping is enabled.
iina/PlayerCore.swift
Outdated
| func getLoopMode() -> LoopMode { | ||
| let loopPlaylistStatus = mpv.getString(MPVOption.PlaybackControl.loopPlaylist) | ||
| let loopFileStatus = mpv.getString(MPVOption.PlaybackControl.loopFile) | ||
| if loopFileStatus == "inf" { | ||
| return .file | ||
| } else if loopPlaylistStatus == "inf" || loopPlaylistStatus == "force" { | ||
| return .playlist | ||
| } | ||
| return .off | ||
| } |
There was a problem hiding this comment.
func getLoopMode() -> LoopMode {
let loopFileStatus = mpv.getString(MPVOption.PlaybackControl.loopFile)
guard loopFileStatus != "inf" else { return .file }
if let loopFileStatus = loopFileStatus, let count = Int(loopFileStatus), count != 0 {
return .file
}
let loopPlaylistStatus = mpv.getString(MPVOption.PlaybackControl.loopPlaylist)
guard loopPlaylistStatus != "inf", loopPlaylistStatus != "force" else { return .playlist }
guard let loopPlaylistStatus = loopPlaylistStatus, let count = Int(loopPlaylistStatus) else {
return .off
}
return count == 0 ? .off : .playlist
}
|
What's the best way to test this? I'm not super familiar with how to interact with mpv options. |
|
To use mpv's IPC interface I install Once that setup is done you can send commands to
Running the commands looks like: low-batt@gag ~$ echo '{ "command": ["set_property", "loop-playlist", "2"] }' | socat - /tmp/mpv-socket | jq
{
"request_id": 0,
"error": "success"
}
low-batt@gag ~$ |
|
Actually you can also watch the property in real time in the inspector |
This commit: - Changed the resizing mode of (Scroll View - Clip View - Table View) of both Playlist and Chapters tabs in PlaylistViewController from Autoresizing Mask to Inferred - Added constrains to the table view in both Playlist and Chapters tabs, which makes all of the resizing mode of the aforementioned views from Inferred (Autoresizing Mask) to Inferred (Constrains) - Added "Horizontally in the container = 0" and "Vertically in the container = 0" for both of the table views, otherwise Xcode sometimes complains about missing X or Y contrains for the table view. This commit fixes the xib editor always has the "Misplaced Views" warning and it's actually not fixable. When you try to fix it, the view will shrink a bit and the warning still exist. Even you just ignore it, every time the constrains on the canvas is re-calucated, the view will shrink as well.
|
Pushed another commit trying to fix the autolayout error. It's not a UI bug, but really annoying to work in I wanted to fix this issue in develop branch, but I have other changes in |
... when the property is a number
Description:
A continued work of #4242. Changes:
.off->.playlist->.file).playlistand.filemutually exclusiveremoteCommand.changeRepeatModeCommand