Repeat single file #4242

MikeWang000000 · 2023-03-01T18:32:57Z

I have read CONTRIBUTING.md
This implements/fixes issue #.

Description:
New feature: Repeat single file
Cherry-picks CarterLi@490b335
Authored by @CarterLi

…op file` button

according to `CONTRIBUTING.md`

MikeWang000000 · 2023-03-01T18:54:13Z

I think this feature is very useful, so I cherry-picked it here.
However I am not macOS App developer, please help me check the changes of iina/Base.lproj/PlaylistViewController.xib, many thanks.

low-batt · 2023-03-01T20:18:59Z

From a very quick look I spotted one issue that will need to be addressed. I noticed this because I've had to fix this in some of my PRs.

From CONTRIBUTING - Localizations:

We use Crowdin for localization. When there is a UI text change, only add/update the strings in Base.lproj and en.lproj. Please do not include any localization changes for other languages in the pull request. New strings on the develop branch from the en.lproj folder will be synced automatically to Crowdin, and we will fetch the latest translations from Crowdin before releasing the next version.

Changes to localization files in directories other than Base.lproj and en.lproj can cause merge conflicts when merging translations from Crowdin.

That means the changes to the file zh-Hans.lproj/Localizable.strings in this PR must instead be made in Crowdin once this PR has been merged into the develop branch and Crowdin has been updated with the changes.

MikeWang000000 · 2023-03-02T00:16:44Z

Thanks. I have noticed that and reverted the change of zh-Hans.lproj/Localizable.strings in my second commit, so there should me no change in this file after squashing this PR into single commit 🤔

low-batt · 2023-03-02T01:14:07Z

My bad! Missed that the commit was undoing the changes. I forgot to use GitHub's Show all changes view. In that view it is clear the file will not be changed.

CarterLi · 2023-03-02T02:02:07Z

Ref: #3704

The author @lhc70000 promised that he would add it sometime, lets make it happen

CarterLi · 2023-03-02T02:26:17Z

It's not duplicate. The loop file part was removed in my old PR.

MikeWang000000 · 2023-03-02T02:29:15Z

I see. Reopened ; )

low-batt

In testing I found the file loop button was not displayed in the enabled (blue) state in the playlist when IINA was started with file loop enabled. I traced this to the method PlaylistViewController.viewDidAppear which is only initializing one of the buttons. See the proposed code fix to that class.

The code level design of the changes deviates from the existing method for handling IINA preferences that are tied to mpv options. This is normally done in MPVController.mpvInit by calling setUserOption. That method configures mpv initially and establishes a listener for the IINA preference changing that will sync mpv if the IINA preference is changed. We'd want to add something like this at line 218 in MPVController right after the call to setUserOption for playlistAutoPlayNext, BUT DO NOT MAKE THIS CHANGE NOW:

let trueIsInfiniteHandler: OptionObserverInfo.Transformer = { key in
      return Preference.bool(for: key) ? "inf" : nil
}
setUserOption(PK.enablePlaylistLoop, type: .other, forName: MPVOption.PlaybackControl.loopPlaylist,
              transformer: trueIsInfiniteHandler)
setUserOption(PK.enableFileLoop, type: .other, forName: MPVOption.PlaybackControl.loopFile,
              transformer: trueIsInfiniteHandler)

Using this system will allow some of the other code the PR is adding to be simplified. BUT WE DO NOT WANT TO ADDRESS THIS NOW.

I checked with @uiryuu and we want to get this change into the current release. So for now we only need to fix PlaylistViewController.viewDidAppear. We can refactor and simplify the code after this next release is cut.

low-batt · 2023-04-03T04:19:32Z

iina/PlaylistViewController.swift

@@ -149,7 +151,7 @@ class PlaylistViewController: NSViewController, NSTableViewDataSource, NSTableVi
    reloadData(playlist: true, chapters: true)

    let loopStatus = player.mpv.getString(MPVOption.PlaybackControl.loopPlaylist)
-    loopBtn.state = (loopStatus == "inf" || loopStatus == "force") ? .on : .off
+    loopPlaylistBtn.state = (loopStatus == "inf" || loopStatus == "force") ? .on : .off
  }


Suggested change

}

let loopFileStatus = player.mpv.getString(MPVOption.PlaybackControl.loopFile)

loopFileBtn.state = loopFileStatus == "inf" ? .on : .off

}

uiryuu · 2023-04-03T07:25:43Z

Some visual suggestions:

I compared the new icon you provided with the SF Pro Symbols provided by Apple, IMO the Apple's repeat.1 is visually better. We may consider using that.
The spaces between the three icons are not identical.
The highlight style of the new button is not the same as the pre-existing loop button. And this highlight style itself has erroneous behavior: when the window loses focus, the highlight disappears.

I can help you to fix these kind of things if you are not able to or not willing to do.

A code review will be arriving shortly...

* 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>

* Repeat single file (#4242) * 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> * Use image from SF; merge two buttons into 1 * Use LoopMode; implement remote methods * Fixed a bug where the icon is not correctly shown Change the cycle to: no loop -> file loop -> playlist loop * Fix issues about OSD message * Fix some autolayout issues 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. * Use the proposed fix to reflect the loop status ... when the property is a number --------- Co-authored-by: Mike Wang <mikewang000000@gmail.com> Co-authored-by: 李通洲 <carter.li@eoitek.com>

CarterLi and others added 2 commits March 2, 2023 02:22

PlaylistView: Store preference for playlist loop status and add a `lo…

574c556

…op file` button

Update lproj

3e49cb5

according to `CONTRIBUTING.md`

This comment was marked as off-topic.

Sign in to view

MikeWang000000 closed this Mar 2, 2023

MikeWang000000 reopened this Mar 2, 2023

low-batt requested changes Apr 3, 2023

View reviewed changes

low-batt requested a review from uiryuu April 3, 2023 04:43

uiryuu self-assigned this Apr 4, 2023

uiryuu changed the base branch from develop to repeat-file April 6, 2023 01:01

uiryuu merged commit d941523 into iina:repeat-file Apr 6, 2023

uiryuu mentioned this pull request Apr 13, 2023

Implement repeat file #4350

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Repeat single file #4242

Repeat single file #4242

Uh oh!

MikeWang000000 commented Mar 1, 2023 •

edited

Loading

Uh oh!

MikeWang000000 commented Mar 1, 2023

Uh oh!

low-batt commented Mar 1, 2023

Uh oh!

MikeWang000000 commented Mar 2, 2023

Uh oh!

low-batt commented Mar 2, 2023

Uh oh!

CarterLi commented Mar 2, 2023 •

edited

Loading

Uh oh!

This comment was marked as off-topic.

CarterLi commented Mar 2, 2023

Uh oh!

MikeWang000000 commented Mar 2, 2023

Uh oh!

low-batt left a comment •

edited by uiryuu

Loading

Uh oh!

low-batt Apr 3, 2023

Uh oh!

uiryuu commented Apr 3, 2023

Uh oh!

Uh oh!

-  }
+    let loopFileStatus = player.mpv.getString(MPVOption.PlaybackControl.loopFile)
+    loopFileBtn.state = loopFileStatus == "inf" ? .on : .off
+  }

Uh oh!

Repeat single file #4242

Repeat single file #4242

Uh oh!

Conversation

MikeWang000000 commented Mar 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MikeWang000000 commented Mar 1, 2023

Uh oh!

low-batt commented Mar 1, 2023

Uh oh!

MikeWang000000 commented Mar 2, 2023

Uh oh!

low-batt commented Mar 2, 2023

Uh oh!

CarterLi commented Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as off-topic.

CarterLi commented Mar 2, 2023

Uh oh!

MikeWang000000 commented Mar 2, 2023

Uh oh!

low-batt left a comment • edited by uiryuu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

low-batt Apr 3, 2023

Choose a reason for hiding this comment

Uh oh!

uiryuu commented Apr 3, 2023

Uh oh!

Uh oh!

MikeWang000000 commented Mar 1, 2023 •

edited

Loading

CarterLi commented Mar 2, 2023 •

edited

Loading

low-batt left a comment •

edited by uiryuu

Loading