Skip to content

Commit

Permalink
Fix #2326
Browse files Browse the repository at this point in the history
  • Loading branch information
@saagarjha
saagarjha committed May 29, 2019
1 parent 818b436 commit e602a57
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 9 deletions.
31 changes: 22 additions & 9 deletions iina/Base.lproj/PrefUIViewController.xib
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
<?xml version="1.0" encoding="UTF-8"?>
<document type="com.apple.InterfaceBuilder3.Cocoa.XIB" version="3.0" toolsVersion="14490.55" targetRuntime="MacOSX.Cocoa" propertyAccessControl="none" useAutolayout="YES" customObjectInstantitationMethod="direct">
<document type="com.apple.InterfaceBuilder3.Cocoa.XIB" version="3.0" toolsVersion="14490.70" targetRuntime="MacOSX.Cocoa" propertyAccessControl="none" useAutolayout="YES" customObjectInstantitationMethod="direct">
<dependencies>
<deployment identifier="macosx"/>
<plugIn identifier="com.apple.InterfaceBuilder.CocoaPlugin" version="14490.55"/>
<plugIn identifier="com.apple.InterfaceBuilder.CocoaPlugin" version="14490.70"/>
<capability name="documents saved in the Xcode 8 format" minToolsVersion="8.0"/>
</dependencies>
<objects>
Expand Down Expand Up @@ -1045,11 +1045,11 @@
<point key="canvasLocation" x="19" y="-265"/>
</customView>
<customView id="1fz-oP-RhZ">
<rect key="frame" x="0.0" y="0.0" width="588" height="119"/>
<rect key="frame" x="0.0" y="0.0" width="588" height="141"/>
<autoresizingMask key="autoresizingMask" flexibleMaxX="YES" flexibleMinY="YES"/>
<subviews>
<textField identifier="SectionTitlePictureInPicture" horizontalHuggingPriority="251" verticalHuggingPriority="750" allowsCharacterPickerTouchBarItem="YES" translatesAutoresizingMaskIntoConstraints="NO" id="ghC-br-cK9">
<rect key="frame" x="-2" y="77" width="124" height="34"/>
<rect key="frame" x="-2" y="99" width="124" height="34"/>
<textFieldCell key="cell" sendsActionOnEndEditing="YES" alignment="left" id="G8w-J7-1q1">
<font key="font" metaFont="systemBold"/>
<string key="title">Picture-in-
Expand All @@ -1059,7 +1059,7 @@ Picture:</string>
</textFieldCell>
</textField>
<button verticalHuggingPriority="750" translatesAutoresizingMaskIntoConstraints="NO" id="A7f-Up-23R">
<rect key="frame" x="118" y="6" width="425" height="18"/>
<rect key="frame" x="118" y="28" width="425" height="18"/>
<buttonCell key="cell" type="check" title="Toggle Picture-in-Picture by minimizing/un-minimizing the window" bezelStyle="regularSquare" imagePosition="left" state="on" inset="2" id="baY-O6-fcB">
<behavior key="behavior" changeContents="YES" doesNotDimImage="YES" lightByContents="YES"/>
<font key="font" metaFont="system"/>
Expand All @@ -1069,15 +1069,15 @@ Picture:</string>
</connections>
</button>
<textField horizontalHuggingPriority="251" verticalHuggingPriority="750" translatesAutoresizingMaskIntoConstraints="NO" id="l21-mq-zWd">
<rect key="frame" x="118" y="94" width="209" height="17"/>
<rect key="frame" x="118" y="116" width="209" height="17"/>
<textFieldCell key="cell" scrollable="YES" lineBreakMode="clipping" sendsActionOnEndEditing="YES" title="When entering Picture-in-Picture:" id="EUd-RD-g0T">
<font key="font" metaFont="system"/>
<color key="textColor" name="labelColor" catalog="System" colorSpace="catalog"/>
<color key="backgroundColor" name="controlColor" catalog="System" colorSpace="catalog"/>
</textFieldCell>
</textField>
<box title="Box" titlePosition="noTitle" translatesAutoresizingMaskIntoConstraints="NO" id="eTQ-fy-fNJ">
<rect key="frame" x="117" y="34" width="466" height="56"/>
<rect key="frame" x="117" y="56" width="466" height="56"/>
<view key="contentView" id="ucO-5V-y9Y">
<rect key="frame" x="3" y="3" width="460" height="50"/>
<autoresizingMask key="autoresizingMask" widthSizable="YES" heightSizable="YES"/>
Expand Down Expand Up @@ -1148,23 +1148,36 @@ Picture:</string>
</constraints>
</view>
</box>
<button verticalHuggingPriority="750" translatesAutoresizingMaskIntoConstraints="NO" id="6jT-ao-ep5">
<rect key="frame" x="118" y="6" width="310" height="18"/>
<buttonCell key="cell" type="check" title="Enter Picture-in-Picture when switching spaces" bezelStyle="regularSquare" imagePosition="left" state="on" inset="2" id="7gM-dr-eqv">
<behavior key="behavior" changeContents="YES" doesNotDimImage="YES" lightByContents="YES"/>
<font key="font" metaFont="system"/>
</buttonCell>
<connections>
<binding destination="lH7-Vv-0M1" name="value" keyPath="values.enterPipWhenSwitchingSpaces" id="mmR-8o-i9k"/>
</connections>
</button>
</subviews>
<constraints>
<constraint firstItem="ghC-br-cK9" firstAttribute="top" secondItem="1fz-oP-RhZ" secondAttribute="top" constant="8" id="5nx-h9-8G4"/>
<constraint firstAttribute="trailing" relation="greaterThanOrEqual" secondItem="A7f-Up-23R" secondAttribute="trailing" id="6vX-hX-s1z"/>
<constraint firstItem="ghC-br-cK9" firstAttribute="leading" secondItem="1fz-oP-RhZ" secondAttribute="leading" id="A1p-op-Aj1"/>
<constraint firstAttribute="trailing" relation="greaterThanOrEqual" secondItem="6jT-ao-ep5" secondAttribute="trailing" id="F0Z-2c-GBi"/>
<constraint firstItem="eTQ-fy-fNJ" firstAttribute="top" secondItem="l21-mq-zWd" secondAttribute="bottom" constant="6" id="IVY-I6-WXc"/>
<constraint firstAttribute="bottom" secondItem="A7f-Up-23R" secondAttribute="bottom" constant="8" id="KI3-bx-05m"/>
<constraint firstAttribute="trailing" secondItem="eTQ-fy-fNJ" secondAttribute="trailing" constant="8" id="KZx-Lw-Sfh"/>
<constraint firstItem="6jT-ao-ep5" firstAttribute="top" secondItem="A7f-Up-23R" secondAttribute="bottom" constant="8" id="LDS-9q-sdX"/>
<constraint firstItem="l21-mq-zWd" firstAttribute="top" secondItem="ghC-br-cK9" secondAttribute="top" id="N7b-nm-9lF"/>
<constraint firstItem="l21-mq-zWd" firstAttribute="leading" secondItem="1fz-oP-RhZ" secondAttribute="leading" constant="120" id="Soe-cB-SYk"/>
<constraint firstItem="A7f-Up-23R" firstAttribute="leading" secondItem="eTQ-fy-fNJ" secondAttribute="leading" id="XVe-n6-qzG"/>
<constraint firstItem="A7f-Up-23R" firstAttribute="top" secondItem="eTQ-fy-fNJ" secondAttribute="bottom" constant="16" id="Xso-UC-S3D"/>
<constraint firstAttribute="bottom" secondItem="6jT-ao-ep5" secondAttribute="bottom" constant="8" id="eWS-76-O76"/>
<constraint firstAttribute="trailing" relation="greaterThanOrEqual" secondItem="l21-mq-zWd" secondAttribute="trailing" id="l5H-rO-WnB"/>
<constraint firstItem="ghC-br-cK9" firstAttribute="trailing" relation="greaterThanOrEqual" secondItem="1fz-oP-RhZ" secondAttribute="leading" constant="120" id="pTG-PR-Adh"/>
<constraint firstItem="6jT-ao-ep5" firstAttribute="leading" secondItem="A7f-Up-23R" secondAttribute="leading" id="rQn-cR-JDk"/>
<constraint firstItem="eTQ-fy-fNJ" firstAttribute="leading" secondItem="l21-mq-zWd" secondAttribute="leading" id="xog-r6-yIL"/>
</constraints>
<point key="canvasLocation" x="6" y="977"/>
<point key="canvasLocation" x="6" y="987.5"/>
</customView>
</objects>
<resources>
Expand Down
8 changes: 8 additions & 0 deletions iina/MainWindowController.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -709,6 +709,14 @@ class MainWindowController: NSWindowController, NSWindowDelegate {
self.player.togglePause(true)
}
})

if #available(macOS 10.12, *) {
NSWorkspace.shared.notificationCenter.addObserver(forName: NSWorkspace.activeSpaceDidChangeNotification, object: nil, queue: nil, using: { [unowned self] _ in
if Preference.bool(for: .enterPipWhenSwitchingSpaces) {
self.enterPIP()
}
})
}
}

deinit {
Expand Down
1 change: 1 addition & 0 deletions iina/Preference.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ struct Preference {
static let windowBehaviorWhenPip = Key("windowBehaviorWhenPip")
static let pauseWhenPip = Key("pauseWhenPip")
static let togglePipByMinimizingWindow = Key("togglePipByMinimizingWindow")
static let enterPipWhenSwitchingSpaces = Key("enterPipWhenSwitchingSpaces")

// Codec

Expand Down

8 comments on commit e602a57

@anohren
Copy link
Contributor

@anohren anohren commented on e602a57 May 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to see you gave this idea a go!

I don't have any experience with that notification and I couldn't decipher its documentation, but just from looking at this commit I can't see any logic checking whether the space that was switched to is actually the one containing the window in question (in which case a switch to PiP is undesirable). Is such a check not necessary?

Also, was this tested with more than one monitor?

Edit: I also imagined a check to see whether the space that was switched from is actually the one containing the window. Otherwise a PiP window would just magically appear if you e.g. resumed a non-visible video with your media keys and then proceeded to navigate to it by switching spaces. I.e. the desire to switch to PiP should, in my opinion, only be inferred if the window starts out being visible at the point of switching spaces.

@uiryuu
Copy link
Member

@uiryuu uiryuu commented on e602a57 May 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit contains another issue: entering full screen is also considered as changing space, so it will enter pip mode.

@saagarjha
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You bring up some good points. When I wrote this I didn't think these checks were necessary, as I had thought that the only way to get this notification to fire would be to exit the current space (otherwise you'd already have the PiP window floating around), but there are ways to make this condition break down so the checks are actually needed. Let me fix that.

Also, was this tested with more than one monitor?

Nope, I don't have an external monitor to test on :(

@anohren
Copy link
Contributor

@anohren anohren commented on e602a57 May 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit contains another issue: entering full screen is also considered as changing space, so it will enter pip mode.

Yeah, that does sound like a problem.

While searching for a better description of the notification I came across someone saying that entering mission control was also considered a switch. It sounds pretty messy.

@uiryuu
Copy link
Member

@uiryuu uiryuu commented on e602a57 May 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a NSWindow's property called isVisible, which might be useful for our case, tho I haven't tried it. Documentation says:

A Boolean value that indicates whether the window is visible onscreen (even when it’s obscured by other windows).

@saagarjha
Copy link
Member Author

@saagarjha saagarjha commented on e602a57 May 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I should actually revert this commit, honestly: I was a bit too hasty in pushing it out. The API that something like this would need to behave correctly (namely, being able to observe the current space) is not public, at least based on what I could see in in Apple's documentation. I'll come back around to this once I can poke into how Dock and Skylight work.

@saagarjha
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reverted this commit: cf0d271.

@anohren
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget to reopen #2326 :)

Please sign in to comment.