Skip to content

Close option to start iina-cli in music-mode, #3651 #3926

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
May 4, 2023
Merged

Close option to start iina-cli in music-mode, #3651 #3926

merged 2 commits into from
May 4, 2023

Conversation

low-batt
Copy link
Contributor

The commit in the pull request will:

  • Add a new enterMusicMode property to the CommandLineStatus struct

  • Add support for a new --music-mode option to the
    AppDelegate.parseArguments method

  • Add code to applicationDidFinishLaunching to switch to the mini player
    if the music-mode option is specified

  • Add the new --music-mode option to the output of the --help option

  • This change has been discussed with the author.

  • It implements / fixes issue Missing option to start iina-cli in music-mode #3651.

Description:

@uiryuu uiryuu added this to the 1.4.0 milestone Aug 25, 2022
uiryuu
uiryuu reviewed Aug 25, 2022
View reviewed changes
iina/AppDelegate.swift Outdated
if #available(macOS 10.12, *), let pc = lastPlayerCore, commandLineStatus.enterPIP {
pc.mainWindow.enterPIP()
if let pc = lastPlayerCore {
// PiP is not supported in music mode. Ignore the PiP option if music mode was specified.
Copy link
Member

Choose a reason for hiding this comment

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

Can you add this to the help message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to update the help. Something like this?

    --music-mode:
            Enter music mode after opening the media.
            If --music-mode is specified the --pip option will be ignored.

What I thought was really needed was option validation where if you use both then IINA fails with an error. The typical style error message would be:

Cannot specify both --music-mode and --pip

I did not add that because validation is currently a missing feature of the IINA CLI. For example there is no attempt to reject mpv options that IINA does not support like the ones that configure mpv's basic controller. IINA ignores the options.

Should I just update the help, or also add validation and treat this as a fatal error like most command line utilities would do?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to failing with a useful error message. Otherwise it's not obvious why --music-mode should be favored over --pip

@uiryuu uiryuu removed this from the 1.4.0 milestone Nov 6, 2022
low-batt added 2 commits April 20, 2023 18:05
@low-batt
Close option to start iina-cli in music-mode, iina#3651
7abcf16 
The commit in the pull request will:
- Add a new enterMusicMode property to the CommandLineStatus struct
- Add support for a new --music-mode option to the
  AppDelegate.parseArguments method
- Add code to applicationDidFinishLaunching to switch to the mini player
  if the music-mode option is specified
- Add the new --music-mode option to the output of the --help option
@low-batt
Close option to start iina-cli in music-mode, iina#3651
f31f518 
The commit in the pull request will:
- Add a new enterMusicMode property to the CommandLineStatus struct
- Add support for a new --music-mode option to the
  AppDelegate.parseArguments method
- Add code to applicationDidFinishLaunching to switch to the mini player
  if the music-mode option is specified
- Add the new --music-mode option to the output of the --help option
- Update the help for --pip to indicate it can not be used with music mode
- Terminate with a command line usage error if both --music-mode and --pip
  are specified
@low-batt
Copy link
Contributor Author

I updated the PR so that both iina-cli and the IINA executable will reject combining --music-mode with --pip and terminate an error as seen here:

low-batt@gag MacOS$ ./iina-cli --music-mode --pip ~/Movies/resume-failure.mp4 
Cannot specify both --music-mode and --pip
low-batt@gag MacOS$

The help for --pip now indicates that music mode does not support Picture-in-Picture.

Help output: 
low-batt@gag MacOS$ ./iina-cli --help
Usage: iina-cli [arguments] [files] [-- mpv_option [...]]

Arguments:
--mpv-*:
        All mpv options are supported here, except those starting with "--no-".
        Example: --mpv-volume=20 --mpv-resume-playback=no
--separate-windows | -w:
        Open all files in separate windows.
--stdin, --no-stdin:
        You may also pipe to stdin directly. Sometimes iina-cli can detect whether
        stdin has file, but sometimes not. Therefore it's recommended to always
        supply --stdin when piping to iina, and --no-stdin when you are not intend
        to use stdin.
--keep-running:
        Normally iina-cli launches IINA and quits immediately. Supply this option
        if you would like to keep it running until the main application exits.
--music-mode:
        Enter music mode after opening the media.
--pip:
        Enter Picture-in-Picture after opening the media. Music mode does not
        support Picture-in-Picture.
--help | -h:
        Print this message.

mpv Option:
Raw mpv options without --mpv- prefix. All mpv options are supported here.
Example: --volume=20 --no-resume-playback
low-batt@gag MacOS$

@low-batt low-batt linked an issue Apr 20, 2023 that may be closed by this pull request
Missing option to start iina-cli in music-mode #3651
Closed
@uiryuu uiryuu merged commit d1fec00 into iina:develop May 4, 2023
@low-batt low-batt deleted the close-3651 branch July 10, 2023 04:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uiryuu uiryuu uiryuu approved these changes

+1 more reviewer

@svobs svobs svobs left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
feature: cli feature: music mode
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Missing option to start iina-cli in music-mode
3 participants
@low-batt @svobs @uiryuu