Close add A-B Loop indicator on progress bar, #548 #3529

low-batt · 2021-10-30T17:38:05Z

This commit adds two additional thumbs to the progress bar representing
the A and B loop points when the A-B loop feature is active. This allows
the user to adjust the loop points.

This commit will:

Add a new class PlaySliderLoopKnob that adds an additional thumb
Add a new class PlaySlider that customizes NSSlider to add the thumbs
Change MainWindowController.xib to use PlaySlider for the OSC
Change PlayerCore to allow mpv loop points to be updated
Change MainWindowController to observe the thumbs and update mpv
Change PlayerWindowController to synchronize the thumbs with mpv
Change MainMenuActions to call the window controller for A-B Loop
Change PlaybackInfo to use an enum class for abLoopStatus
Add a new message to OSDMessage
This change has been discussed with the author.
[ x] It implements / fixes issue Adding A-B Loop indicator on progress bar #548.

Description:
See the associated issue for screenshots of what the OSC looks like with these changes.

CarterLi · 2021-11-15T03:00:27Z

I got these kind of errors:

/Users/carter/iina/iina/PlaySliderLoopKnob.swift:162:59: error: 'darkKnobColor' is unavailable in macOS
      return slider.window!.effectiveAppearance.isDark ? .darkKnobColor : .lightKnobColor
                                                          ^~~~~~~~~~~~~
/Users/carter/iina/iina/PlaySliderLoopKnob.swift:15:14: note: 'darkKnobColor' was obsoleted in macOS 10.14
  static let darkKnobColor = NSColor(calibratedRed: 0.59, green: 0.59, blue: 0.59, alpha: 1)
             ^
/Users/carter/iina/iina/PlaySliderLoopKnob.swift:162:76: error: 'lightKnobColor' is unavailable in macOS
      return slider.window!.effectiveAppearance.isDark ? .darkKnobColor : .lightKnobColor
                                                                           ^~~~~~~~~~~~~~
/Users/carter/iina/iina/PlaySliderLoopKnob.swift:16:14: note: 'lightKnobColor' was obsoleted in macOS 10.14
  static let lightKnobColor = NSColor(calibratedRed: 0.35, green: 0.35, blue: 0.35, alpha: 1)

Any ideas?

low-batt · 2021-11-15T03:45:31Z

Hi!

The offending code from PlaySliderLoopKnob.swift is:

// These colors are for 10.13- only
@available(macOS, obsoleted: 10.14)
fileprivate extension NSColor {
  // Use different colors to distinguish loop knobs from the primary knob.
  static let darkKnobColor = NSColor(calibratedRed: 0.59, green: 0.59, blue: 0.59, alpha: 1)
  static let lightKnobColor = NSColor(calibratedRed: 0.35, green: 0.35, blue: 0.35, alpha: 1)
}

    // Starting with macOS Mojave 10.14 colors can be configured to automatically adjust for the
    // current appearance.
    if #available(macOS 10.14, *) {
      return NSColor(named: .mainSliderLoopKnob)!
    } else {
      return slider.window!.effectiveAppearance.isDark ? .darkKnobColor : .lightKnobColor
    }

I patterned that code after this existing code from PlaySliderCell.swift:

// These colors are for 10.13- only
@available(macOS, obsoleted: 10.14)
fileprivate extension NSColor {


    if #available(macOS 10.14, *) {
      return NSColor(named: .mainSliderKnobActive)!
    } else {
      return .darkKnobColor
    }

I'm not spotting a typo in the new code that would trigger the errors shown.
Do you spot something I've missed?

This relates to the "macOS Deployment Target" build setting in the top level IINA project.
It should be set to "10.11" as IINA still supports that release.

Any local changes to your Xcode project file?
What Xcode version?
I was testing with Xcode 12.5.

Late, so my brain may not be working. I'm coming up empty. I'll think on this some more.

CarterLi · 2021-11-15T05:34:27Z

I was using the latest Xcode 13.1

Strange enough. I didi find a similar usage:

https://github.com/iina/iina/blob/develop/iina/PlaySliderCell.swift#L12

But I didn't get errors there. And If I changed obsoleted to deprecated, the error disappeared.

Don't know if it's a xcode bug or not.

low-batt · 2021-11-15T18:55:00Z

All the work on these changes was done on an Intel Mac in a VM running macOS 11.4 using Xcode 12.5. I've since moved my development system to be native on an Apple silicon Mac running macOS 12.0.1 and Xcode Version 13.1 (13A1030d). I have these changes in my current local branch. Xcode is successfully compiling PlaySliderLoopKnob for me. The mystery continues!

This from the Xcode 13 Release Notes is interesting:

Availability checks in iPhone and iPad apps on a Mac with Apple silicon always return true. This causes iOS apps running in macOS 11 Big Sur to see iOS 15 APIs as available, resulting in crashes. This only affects apps available in the Mac App Store built with the “My Mac (Designed for iPhone)” or “My Mac (Designed for iPad)” run destination. It doesn’t affect Mac Catalyst apps. (83378814)

The description of the problem indicates the failure only affects iPhone and iPad apps, but maybe the problem is worse than they knew when the release notes were published?

Are you building on an Apple silicon Mac?
If so, are you building for Apple silicon or for Intel (with Rosetta)?
I'm building for Apple silicon.

The release notes under Swift / Resolved Issues lists this fix:

The compiler used to erroneously accept @available annotations on enum cases with associated values that were newer than the deployment target. (80238318)

Maybe that "fix" introduced a regression in the availability support?

The Xcode 13.2 Beta Release Notes contains this:

Fixed an issue where availability checks in iPhone and iPad apps running on a Mac with Apple silicon always returned true, which caused iOS apps running in macOS Big Sur to see iOS 15 APIs as available and crash when trying to use those APIs. These availability checks now return the correct result for apps compiled with Xcode 13.2. (83378814)

So the issue with availability checks is definitely still a problem in Xcode 13.1.

None of this provides a definitive answer for the problem at hand. It just makes more suspicious that the issue is due to Xcode 13.

By the way, IINA built under Xcode 13 crashed out for me when I bought up Preferences / General. Telegram user xjc pointed me to a fix in this commit ed796c6 that upgrades the Sparkle framework to 2.0.0-beta.3. That fix worked for me.

iina/PlaySlider.swift

svobs · 2023-03-04T08:26:04Z

Played around with this and it looks good to me. Although I didn't notice until now that there is some screwiness with the position of the knob on the play slider. If you try clicking on the slider near the beginning of the video, the slider bar is drawn about too far to the right by ~ 1x its full width. When clicking near the end, it's drawn to the left by ~ half its width. I should probably file a bug report for it sometime. Doesn't seem to be the fault of your PR (I checked and now see it on develop branch and in older versions of IINA), but it's noticeable because the play slider seems to go past the "B" indicator before looping. And for some reason it seems more noticeable with a Floating OSC.

uiryuu

When the playback position is on A, the main knob will surpass knob A by 1(?)px. Don't know whether it's a intended design?

uiryuu · 2023-04-02T05:47:39Z

iina/MainWindowController.swift

@@ -630,9 +630,41 @@ class MainWindowController: PlayerWindowController {
      setWindowFrameForLegacyFullScreen()
    }

+    // Observe the loop knobs on the progress bar and update mpv when the knobs move.
+    addObserver(to: .default, forName: .iinaPlaySliderLoopKnobChanged, object: playSlider.abLoopA) { [weak self] _ in
+      guard let strongSelf = self else { return }


guard let self = self else { return }

This is supported starting from Swift 4.2

uiryuu · 2023-04-02T05:47:53Z

iina/MainWindowController.swift

+      strongSelf.player.sendOSD(.abLoopUpdate(.aSet, VideoTime(seconds).stringRepresentation))
+    }
+    addObserver(to: .default, forName: .iinaPlaySliderLoopKnobChanged, object: playSlider.abLoopB) { [weak self] _ in
+      guard let strongSelf = self else { return }


uiryuu · 2023-04-02T06:06:22Z

iina/PlaySlider.swift

+
+  // MARK:- Private Properties
+
+  private var abLoopAKnob: PlaySliderLoopKnob!


I think we can use private(set) var abLoopAKnob instead of creating var abLoopA.

uiryuu · 2023-04-02T06:40:23Z

iina/PlaySliderLoopKnob.swift

+
+  private var slider: PlaySlider!
+
+  private var value: Double = 0


Is it necessary to use a separate value here? I would recommend to only use doubleValue with didSet if there's no other special consideration.

Fixing this. Removing value and changing doubleValue to use didSet.

uiryuu · 2023-04-02T06:58:19Z

iina/PlaySliderLoopKnob.swift

+final class PlaySliderLoopKnob: NSView {
+
+  // Must accept first responder for help tags to work.
+  override var acceptsFirstResponder: Bool { true }


Found a bug with this line:

Play a video

Create an AB-loop

Drag the B knob to adjust the loop

Make the cursor be in the OSC so that the controller doesn't disappear

Try to pause the video by pressing space

The video is temporary stopped and then resume

Removing this line seems to solve this problem, so I'm guessing this maybe the culprit.

I remember I had to add this to get tool tips to work. And yet I commented it out and tool tips still seemed to work. I will test some more, but yes it is looking like the fix is to remove this.

low-batt · 2023-04-04T00:23:25Z

I'm also looking into your comment about handlingmouseDown in PlaySliderLoopKnob. I think that will work and simplify the code. Still working on an update.

low-batt · 2023-04-04T01:55:25Z

I decided to go ahead and explore the behavior of the main slider. Changing knobWidth in PlaySliderCell from 3 to 30 makes the primary knob issues obvious. This is the knob set as far to the left as it can go:

If I move the knob all the way to the right it disappears completely. In this screen shot it is not moved as far right as it can go:

My expectation is that knobs should never move outside of the width of the bar. This is a loop knob set all the way to the left:

And all the way to the right:

This behavior is because the code in PlaySliderLoopKnob takes into account the width of the knob and does not use the full width of the bar:

    // The knob is not allowed to slide past the end of the bar, so the usable width is reduced.
    let effectiveWidth = bar.width - cell.knobWidth

I will see if I can make the main knob behave.

low-batt · 2023-04-04T02:22:37Z

Here is another part of the problem. From NSView.isFlipped:

The default value of this property is false, which results in a non-flipped coordinate system. In a non-flipped coordinate system, the origin is in the lower-left corner of the view and positive y-values extend upward. In a flipped coordinate system, the origin is in the upper-left corner of the view and y-values extend downward. X-values always extend to the right.

So isFlipped controls the y-axis and has no effect on the x-axis. However knobRect in PlaySliderCell considers it when calculating the x position of the knob:

    return NSMakeRect(pos - flippedMultiplier * 0.5 * knobWidth, rect.origin.y, knobWidth, height)

This is what is preventing the knob from going all the way to the right.

At least I think I am properly understanding this…

svobs · 2023-04-04T05:57:03Z

So isFlipped controls the y-axis and has no effect on the x-axis. However knobRect in PlaySliderCell considers it when calculating the x position of the knob[...]

Yeah I saw this when I dug into the code a while back. I too thought that was the smoking gun at first, but it turned out to be a red herring, or at best only part of the problem. Unfortunately something like that usually hints that the code around it should not be trusted, but the code around it is doing all kinds of unusual things too. Hopefully you were able to map out part of this 🐇🕳️ already and might have better odds than me!

low-batt · 2023-04-04T06:25:49Z

And that is why I decided to not touch the main slider code back when I originally added this PR. It is not just one problem in one part of the code. I have found a few more things that seem wrong. I have the knobs matching up now. But I still have more work to do. Might not get done tomorrow, but will have an update soon.

low-batt · 2023-04-06T01:09:17Z

I'm still having "fun" with this one. Here is the latest problem I tracked down in the current code. This is where the slider knob ends up when you start playing a video with IINA configured to pause at the start:

You would expect the slider knob to be at the very start of the slider. But notice the gap. That is as far left as the knob will go. Why is part of the slider visible? In PlaySliderCell we override barRect:

  override func barRect(flipped: Bool) -> NSRect {
    let rect = super.barRect(flipped: flipped)
    return NSMakeRect(0, rect.origin.y, rect.width + rect.origin.x * 2, rect.height)
  }

I wish there was a comment about the thinking behind this. I still need to check Catalina, but on Ventura the slider's bar is located at 2. So we are overriding this method to stretch the size of the bar, moving it to 0 and increasing the width by 4. I don't see a problem with that.

But the method knobRect contains this line:

let barRect = super.barRect(flipped: flipped)

Because it specified super it by passed the local overridden method is therefore using the wrong bar size.

This commit adds two additional thumbs to the progress bar representing the A and B loop points when the A-B loop feature is active. This allows the user to adjust the loop points. This commit will: - Add a new class PlaySliderLoopKnob that adds an additional thumb - Add a new class PlaySlider that customizes NSSlider to add the thumbs - Change MainWindowController.xib to use PlaySlider for the OSC - Change PlayerCore to allow mpv loop points to be updated - Change MainWindowController to observe the thumbs and update mpv - Change PlayerWindowController to synchronize the thumbs with mpv - Change MainMenuActions to call the window controller for A-B Loop - Change PlaybackInfo to use an enum class for abLoopStatus - Add a new message to OSDMessage Rebased with develop to resolve conflicts with Xcode project. Rebased with develop to resolve merge conflicts. Rebased with develop to resolve conflicts with plugin system. Rebased with develop and corrected merge conflicts.

This commit adds two additional thumbs to the progress bar representing the A and B loop points when the A-B loop feature is active. This allows the user to adjust the loop points. This commit will: - Add a new class PlaySliderLoopKnob that adds an additional thumb - Add a new class PlaySlider that customizes NSSlider to add the thumbs - Change MainWindowController.xib to use PlaySlider for the OSC - Change PlayerCore to allow mpv loop points to be updated - Change MainWindowController to observe the thumbs and update mpv - Change PlayerWindowController to synchronize the thumbs with mpv - Change MainMenuActions to call the window controller for A-B Loop - Change PlaybackInfo to use an enum class for abLoopStatus - Add a new message to OSDMessage Corrected merge error in Xcode project. Rebased with develop to resolve conflicts with Xcode project. Rebased with develop to resolve merge conflicts. Rebased with develop to resolve conflicts with plugin system. Rebased with develop and corrected merge conflicts. Rebased with develop and resolved Xcode project merge conflicts.

This commit adds two additional thumbs to the progress bar representing the A and B loop points when the A-B loop feature is active. This allows the user to adjust the loop points. This commit will: - Add a new class PlaySliderLoopKnob that adds an additional thumb - Add a new class PlaySlider that customizes NSSlider to add the thumbs - Change MainWindowController.xib to use PlaySlider for the OSC - Change PlayerCore to allow mpv loop points to be updated - Change MainWindowController to observe the thumbs and update mpv - Change PlayerWindowController to synchronize the thumbs with mpv - Change MainMenuActions to call the window controller for A-B Loop - Change PlaybackInfo to use an enum class for abLoopStatus - Add a new message to OSDMessage Rebased with develop to resolve conflicts with Xcode project. Rebased with develop to resolve merge conflicts. Rebased with develop to resolve conflicts with plugin system. Corrected typo found in review.

This commit adds two additional thumbs to the progress bar representing the A and B loop points when the A-B loop feature is active. This allows the user to adjust the loop points. This commit will: - Add a new class PlaySliderLoopKnob that adds an additional thumb - Add a new class PlaySlider that customizes NSSlider to add the thumbs - Change MainWindowController.xib to use PlaySlider for the OSC - Change PlayerCore to allow mpv loop points to be updated - Change MainWindowController to observe the thumbs and update mpv - Change PlayerWindowController to synchronize the thumbs with mpv - Change MainMenuActions to call the window controller for A-B Loop - Change PlaybackInfo to use an enum class for abLoopStatus - Add a new message to OSDMessage - Move changing of the slider control size from PlaySliderCell to PlaySlider - Change PlaySliderCell.knobRect to keep thumb within bar - Remove PlaySliderCell.barRect that increased the width of the bar

low-batt · 2023-04-06T22:49:47Z

Based on the excellent review comments I reworked the new code to simplify it. I also updated the existing PlaySliderCell.knobRect method to correct problems with determining the x coordinate of the thumb that controls the playback position. These changes correct the problems with the primary knob being unable to move all the way to the start of the bar and not lining up with the loop knobs.

I also removed the method PlaySliderCell.barRect that increased the width of the slider's bar. We can put that back in if reviewer's object. It makes it very hard to grab a knob that is at the end of the bar as the bar is running right up against the time label. A lot of the time you will end up changing what the time label is reporting while trying to click on the thumb.

uiryuu

LGTM

lhc70000 force-pushed the develop branch from c9964a4 to 856f3fd Compare April 24, 2022 03:46

low-batt force-pushed the issue-548 branch from a10f12c to 35009a9 Compare May 17, 2022 01:58

low-batt mentioned this pull request Aug 18, 2022

Top of progress bar not behaving as such #3911

Closed

low-batt force-pushed the issue-548 branch from 35009a9 to ee5f639 Compare September 25, 2022 19:47

low-batt linked an issue Nov 7, 2022 that may be closed by this pull request

Adding A-B Loop indicator on progress bar #548

Closed

1 task

low-batt force-pushed the issue-548 branch from ee5f639 to a0a7fbb Compare November 7, 2022 03:06

low-batt force-pushed the issue-548 branch from a0a7fbb to 3b95b51 Compare November 22, 2022 00:51

low-batt mentioned this pull request Feb 25, 2023

Refactor MainWindow/OSC layout handling to fix bugs, improve consistency #4180

Draft

2 tasks

svobs reviewed Feb 28, 2023

View reviewed changes

iina/PlaySlider.swift Outdated Show resolved Hide resolved

low-batt force-pushed the issue-548 branch from d43ccee to 6e933b2 Compare March 25, 2023 22:49

uiryuu requested changes Apr 2, 2023

View reviewed changes

low-batt added 4 commits April 5, 2023 23:05

low-batt force-pushed the issue-548 branch from 6e933b2 to 2c8f8f4 Compare April 6, 2023 22:36

uiryuu approved these changes Apr 7, 2023

View reviewed changes

uiryuu merged commit 24630f1 into iina:develop Apr 11, 2023


		// MARK:- Private Properties

		private var abLoopAKnob: PlaySliderLoopKnob!


		private var slider: PlaySlider!

		private var value: Double = 0

Uh oh!

Close add A-B Loop indicator on progress bar, #548 #3529

Close add A-B Loop indicator on progress bar, #548 #3529

Uh oh!

Conversation

low-batt commented Oct 30, 2021

Uh oh!

CarterLi commented Nov 15, 2021

Uh oh!

low-batt commented Nov 15, 2021

Uh oh!

CarterLi commented Nov 15, 2021

Uh oh!

low-batt commented Nov 15, 2021

Uh oh!

Uh oh!

svobs commented Mar 4, 2023

Uh oh!

uiryuu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

low-batt commented Apr 4, 2023

Uh oh!

low-batt commented Apr 4, 2023

Uh oh!

low-batt commented Apr 4, 2023

Uh oh!

svobs commented Apr 4, 2023

Uh oh!

low-batt commented Apr 4, 2023

Uh oh!

low-batt commented Apr 6, 2023

Uh oh!

low-batt commented Apr 6, 2023

Uh oh!

uiryuu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!