Skip to content
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

cli.output: path-only --player argument #5310

Merged

Conversation

bastimeyer
Copy link
Member

@bastimeyer bastimeyer commented Apr 25, 2023

Resolves #5305

Breaking change... Opening this as a draft for now, since I'm aiming for the next major version release when py37 support gets dropped in two months.

Please read #5305 for the full details about the motivation of these changes.

Changes

  1. Rewrite find_default_player()
  2. Turn --player into a path-only argument and remove support for specifying a whole player command-line with additional arguments. Player arguments now must be set via the --player-args argument.
    As a result, subprocess.Popen() and subprocess.call() now receive an argv with a properly defined and safe player path with additional player arguments. The player detection also doesn't rely on a weird path basename workaround anymore.
  3. Remove the old --player-args {filename} fallback variable. Only {playerinput} is now supported.
  4. Refactor the PlayerOutput's player detection and the player arguments builder for better code quality and easier extensibility.
  5. Actually check/verify the player path via shutil.which(), so a better error can be raised when the path is incorrect, or when the player path is not executable, or when the single executable name can't be resolved from the system's PATH env var. Additionally raise a warning with more info when the player path begins with a single or double quotation mark, as this is now unsupported. Detecting player args in the --player value is not possible, so a warning can't be raised for those cases.
  6. Add {playertitleargs} to --player-args, so the order of automatically generated player titles can be customized. By default, player title args come in front of the player args set by the user, so user data can override (if the player supports overriding previous args).

More details in the commit message bodies...

Notes

  • Since custom player arguments now can only be set via --player-args, this makes the addition of player title arguments more strict. Previously it was possible to set arguments in front of the automated player title arguments, but this is not possible anymore because the order is always path -> title-args -> custom-args, where custom-args has support for the {playerinput} variable, so the input argument can be set freely here. I was thinking about adding another variable ({playertitle}) that replaces the title-args segment and that automatically gets added to the --player-args if it's missing, similar to {playerinput}. Then the player title args can also be set freely, like the player input arg. Thoughts?
  • The updated default player lookup doesn't have tests. I've tested it manually though...
  • No changes to the PlayerOutput implementation itself (which I'm still unhappy about)
  • The Streamlink config files will all be outdated with these changes. This is especially bad on Windows where the installer only writes the config file if it doesn't exist yet. This will require a special note, or maybe even a new option in the installer which lets it overwrite it with the default config.

Please give this branch a thorough test. It would be pretty annoying if something was missed in these changes.

@bastimeyer
Copy link
Member Author

Small refactor, added/improved some test cases and added {playertitleargs} var.

@bastimeyer bastimeyer force-pushed the cli/output/player-path-only branch 3 times, most recently from e534593 to 1b7d6e1 Compare April 27, 2023 18:42
- Rewrite in preparation for the following --player CLI argument changes
- Resolve player paths via `shutil.which()`
- Use `pathlib.Path` (change return type from `str` to `Path` later)
- Add tests
- Remove support for custom player arguments from `--player`
- Set the `--player` argument's `type` to `pathlib.Path`
- Set return type of `find_default_player()` to `pathlib.Path`
- Update `--player` and `--player-args` help texts accordingly
- Update `PlayerOutput`:
  - Update constructor signature and add typing information
  - Rename `cmd` to `path` and remove support for player arguments
  - Always build list of arguments and remove Windows-specific logic
  - Properly tokenize player input value in `--player-args`
  - Log player argv instead of the player command-line string
- Fix now unneeded passthrough URL quotation in `streamlink_cli.main`
- Update docs and fix argparse docs extension
- Update tests and remove Windows-specific player argument tests
- Create dedicated `PlayerArgs` classes for player-specific arguments
- Create subclasses with support for VLC, MPV and PotPlayer
- Move player detection logic to `playerargsfactory()` classmethod
- Fix player executable detection by using regexes
  with optional file extensions only on Windows
- Remove `SUPPORTED_PLAYERS` from `streamlink_cli.constants`
  and fix references in `streamlink_cli.argparser`
- Rewrite and update tests, rename test modules and remove outdated ones
- Resolve player executable before launching it, update argv
  and raise a `FileNotFoundError` on error
- Raise a descriptive `StreamlinkWarning` if the player path
  starts with a quotation mark
- Add and fix tests
@bastimeyer
Copy link
Member Author

Rebased to master (#5317) and updated tests

@bastimeyer bastimeyer mentioned this pull request Jun 27, 2023
8 tasks
@bastimeyer bastimeyer marked this pull request as ready for review July 2, 2023 19:04
@bastimeyer
Copy link
Member Author

I will add more documentation to the deprecations page once this got merged, among all the other breaking changes.

Not sure yet about the Windows installer and its config file. I'll have to figure this out later.

A bit sad that nobody bothered to give this branch a try and give feedback. Will merge now anyway...

@bastimeyer bastimeyer merged commit 4a7df18 into streamlink:master Jul 2, 2023
23 checks passed
@bastimeyer bastimeyer deleted the cli/output/player-path-only branch July 2, 2023 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

proposal: remove optional player args from --player and only allow player paths (breaking change)
1 participant