New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hide main window when entering Picture in Picture #2192

Merged
merged 7 commits into from Jan 23, 2019

Conversation

3 participants
@LEoREo2247
Copy link
Contributor

LEoREo2247 commented Jan 4, 2019

  • This change has been discussed with the author.
  • It implements / fixes issue #2147.

Description:

This PR will add ability to hide main window when entering Picture in Picture if window is not in fullscreen mode.
I looked in to it and I noticed that implementation is quite simple.
It doesn't require specification to re-show window when going back from PiP as PiP seems to be doing that already.
I also added option to enable/disable this behavior, but this may not needed. It depends on how you all thinks.

@saagarjha
Copy link
Member

saagarjha left a comment

I'm putting a hold on this pull request, not because I'm opposed to the changes, but because I wanted to discuss extending it. I've received multiple requests for similar features when entering PIP, such as minimizing the window, raising it, whatever, so I'd like to consider whether these cases fit in here or not (I have been meaning to file an issue for this for a while–sorry). Another thing to consider is whether this can be implemented as a plugin; @lhc70000 thoughts?

@anohren

This comment has been minimized.

Copy link
Contributor

anohren commented Jan 7, 2019

So, should the discussion be held here? If yes: what were the reasonings for minimizing the window, raising it or whatever? I can see the point of minimizing the window since that's effectively hiding the window, but that's pretty much it.

If those suggestions are alternatives to this one, then they fit quite naturally in here.

Sounds kind of weird to move core functionality like PiP to a plugin though.

@saagarjha

This comment has been minimized.

Copy link
Member

saagarjha commented Jan 7, 2019

The core PIP functionality (e.g. actually entering/exiting PIP) likely cannot be a plugin, and will remain as-is. The question is allowing users to customize "reactions" to these events, so they they can e.g. minimize the main window when entering picture-in-picture, or close it, etc. So my question is 1. whether we should have this customization point, 2. whether we should say have a preference that lists some commonly requested actions and performs that when PIP is invoked, or 3. not have a preference at all, but allow plugins to customize this behavior.

@anohren

This comment has been minimized.

Copy link
Contributor

anohren commented Jan 7, 2019

I suspected that that was your intention. However, since, as you say, PiP will remain a core functionality it needs to have a sensible default behavior regardless of whether this will be made a customization point and regardless of whether any such plugins are installed.

If I'm not mistaken, this PR is mostly about implementing the most sensible default behavior. Perhaps it should be split up so that the discussion about if/how to make this customizable can continue independently.

Unless of course that your suggestion is to implement default behavior like this as an official plugin. Sounds a bit overkill, but perhaps that's been the plan all along.

@anohren
Copy link
Contributor

anohren left a comment

I tested this and unfortunately the changes don't seem to be compatible with the option to quit the application when all windows are closed; entering PiP in such a situation with only one open window will quit the application.

I would suggest minimizing the window instead.

Another issue is that if the PiP is dismissed by using the 'X' button the window stays closed instead of being restored.

@anohren

This comment has been minimized.

Copy link
Contributor

anohren commented Jan 10, 2019

The app doesn't quit anymore which is much better :)

There's one problem I think needs fixing for minimizing to work. Dismissing the PiP window with the X button unminimizes it on top of your other windows (a separate problem from the one mentionend before where the window wasn't recreated when using the X button).

The X button is used when you want to stop playback and dismiss the video in order to focus on other things. What happens now is that the video window is unminimized above the other application windows that you had just decided to focus on.

I think that the best option is to let the video window stay minimized in this case. This will keep distraction to a minimum.

@saagarjha

This comment has been minimized.

Copy link
Member

saagarjha commented Jan 20, 2019

Sorry for leaving this hanging for a while, but I just got a chance to look at this change and I'm thinking of taking this in a different direction based on my comments above. In particular, we could have a set of options for what we would like to do when entering picture-in-picture mode, and allow users to pick which one they want. Currently, I'm thinking of providing these options, based on what people have asked:

  • Allow for hiding or minimizing the window when entering picture-in-picture.
  • Allow for entering picture-in-picture when minimizing the window.
  • Allow for pausing on-top behavior for windows when in picture-in-picture mode.

If someone comes up with a script to emulate these, we can close this pull request or remove these options in the future, otherwise, @LEoREo2247, if you're up for it, you can try implementing these; I'm anticipating adding a new "Picture in Picture" section in the UI tab of Preferences to accommodate these. Otherwise I'll add them in myself at some point. Please try to keep the defaults to be what they are currently, as I feel they are the clearest; if users want their window to be modified when entering picture-in-picture they can tell IINA to do this.

Edit: Wrong person

@LEoREo2247

This comment has been minimized.

Copy link
Contributor Author

LEoREo2247 commented Jan 20, 2019

Alright, I think that is pretty neat. I can try implementing that if anyone else wouldn't.

LEoREo2247 added some commits Jan 22, 2019

LEoREo2247
Added various Picture-in-Picture options
- Hide or minimize the window when entering PiP
- Pause playback when entering PiP
- Toggle PiP by minimizing/un-minimizing the window
LEoREo2247
@LEoREo2247

This comment has been minimized.

Copy link
Contributor Author

LEoREo2247 commented Jan 22, 2019

So I've added the options that saagarjha listed. I need someone to check if there is any problem or a flaw.

@saagarjha

This comment has been minimized.

Copy link
Member

saagarjha commented Jan 23, 2019

Great job, @LEoREo2247! There's still some jankiness with the transition, but that's my problem, not yours; I'll look into whether this can be fixed later.

@saagarjha saagarjha merged commit 6bf9aa2 into iina:develop Jan 23, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@alejx alejx referenced this pull request Feb 9, 2019

Open

Localization Update Request for 1.0.2 Release #2285

13 of 20 tasks complete

Aikyuichi added a commit to Aikyuichi/iina that referenced this pull request Feb 12, 2019

spanich tranlations
spanich tranlations for:
- Preferences > UI > Picture-in-Picture (iina#2192)
- "pl_menu.browser", "pl_menu.copy_url", "pl_menu.copy_url_multi" in localizable.strings (iina#2271)
- "touchbar.toggle_pip" in localizable.strings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment