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.

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

Add toggle to suppress sending additional args to external players #4515

Merged
merged 4 commits into from
Jan 13, 2024
Merged

Add toggle to suppress sending additional args to external players #4515

merged 4 commits into from
Jan 13, 2024

Conversation

trostboot
Copy link
Contributor

Title

Add toggle to suppress sending additional args to external players

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

closes #4493

Description

Adds a toggle setting in the external players sections that lets users suppress any args being sent to the external player aside from the video URL.
Also adds corresponding logic to skip processing of args if this toggle is enabled (this made the diff a mess even though it's a minimal change, tried different orders but git no likey).

Screenshots

ignoreArgs

Testing

Tested on Windows using SMPlayer and mpv with the toggle on and off. Seems to work as expected.
Testing with more combinations is always appreciated.
Testing build.

Desktop

  • OS: Windows
  • OS Version: 11 22H2
  • FreeTube version: 0.19.1

Additional context

I'm open to a better name for the setting if anyone thinks it's not clear enough.

@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Jan 3, 2024
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) January 3, 2024 19:12
@trostboot trostboot mentioned this pull request Jan 3, 2024
Closed
3 tasks
auto-merge was automatically disabled January 4, 2024 16:11

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) January 4, 2024 16:11
PikachuEXE
PikachuEXE previously approved these changes Jan 6, 2024
View reviewed changes
Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

I have no external player, can only review code

@absidue
Copy link
Member

absidue commented Jan 9, 2024

Not sure about the naming, as it sounds like it will also ignore the users custom arguments, which is probably undesirable.

@trostboot
Copy link
Contributor Author

That's exactly what it does/what was requested in the linked issue. It also disables the custom args field if enabled to make that clear.

@absidue
Copy link
Member

absidue commented Jan 9, 2024

If that is the case, then you should add a visual indicator to the custom arguments field, so that it is clear that it is disabled/ignored too. That being said, from a user perspective I would expect that to just disable passing any extra information. If I have gone out of my way to add custom arguments, then I would want them to be used.

@trostboot
Copy link
Contributor Author

trostboot commented Jan 9, 2024 •
edited
Loading

It will grey out when disabled, sorry, I haven't updated the screenshot in the initial post, I added the logic to disable it afterwards.

I can see where you're coming from. What would you suggest, then? Pass on custom arguments anyway and rely on the user to clear the custom argument field if they don't want them passed on for whatever reason, temporarily or otherwise?

auto-merge was automatically disabled January 9, 2024 17:59

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) January 9, 2024 18:00
@trostboot
Copy link
Contributor Author

Changed it so custom arguments are still passed on if there are any and changed the name and tooltip accordingly.

ignoreArgs2

absidue
absidue approved these changes Jan 12, 2024
View reviewed changes
Copy link
Member

@absidue absidue left a comment

Choose a reason for hiding this comment

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

Hint for other reviewers: Turning on the "Hide Whitespace" option makes the diff significantly more readable.

@trostboot trostboot requested a review from PikachuEXE January 12, 2024 17:03
@FreeTubeBot FreeTubeBot merged commit 40ff63a into FreeTubeApp:development Jan 13, 2024
6 checks passed
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Jan 13, 2024
@trostboot trostboot deleted the ext-skip-args branch January 13, 2024 06:56
PikachuEXE added a commit to PikachuEXE/FreeTube that referenced this pull request Jan 15, 2024
@PikachuEXE
Merge branch 'development' into feature/playlist-2023-05-w-quick-book…
8d5f3b2 
…mark

* development: (53 commits)
  Add dearrow support for thumbnails (FreeTubeApp#4520)
  Translated using Weblate (French)
  Fix hardware acceleration flag for Linux (FreeTubeApp#4532)
  Translated using Weblate (Bengali)
  Translated using Weblate (Czech)
  Translated using Weblate (Hungarian)
  Translated using Weblate (Turkish)
  Translated using Weblate (Spanish)
  Translated using Weblate (Arabic)
  Translated using Weblate (Italian)
  Translated using Weblate (Polish)
  Translated using Weblate (Russian)
  Translated using Weblate (French)
  Translated using Weblate (Chinese (Simplified))
  Add toggle to suppress sending additional args to external players (FreeTubeApp#4515)
  Translated using Weblate (English (United Kingdom))
  Translated using Weblate (Italian)
  Translated using Weblate (French)
  Translated using Weblate (Finnish)
  Translated using Weblate (Polish)
  ...

# Conflicts:
#	src/renderer/store/modules/settings.js
PikachuEXE added a commit to PikachuEXE/FreeTube that referenced this pull request Jan 15, 2024
@PikachuEXE
Merge branch 'feature/consistent-sharable-video-url-local' into custo…
346d6a1 
…m-builds/current

* feature/consistent-sharable-video-url-local: (30 commits)
  * Update places generating sharable YT video URLs to always return prefix https://youtu.be/
  Translated using Weblate (Chinese (Traditional))
  Translated using Weblate (Chinese (Simplified))
  Add dearrow support for thumbnails (FreeTubeApp#4520)
  Translated using Weblate (French)
  Fix hardware acceleration flag for Linux (FreeTubeApp#4532)
  Translated using Weblate (Bengali)
  Translated using Weblate (Czech)
  Translated using Weblate (Hungarian)
  Translated using Weblate (Turkish)
  Translated using Weblate (Spanish)
  Translated using Weblate (Arabic)
  Translated using Weblate (Italian)
  Translated using Weblate (Polish)
  Translated using Weblate (Russian)
  Translated using Weblate (French)
  Translated using Weblate (Chinese (Simplified))
  Add toggle to suppress sending additional args to external players (FreeTubeApp#4515)
  Translated using Weblate (English (United Kingdom))
  Translated using Weblate (Italian)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PikachuEXE PikachuEXE PikachuEXE approved these changes

@absidue absidue absidue approved these changes

@ChunkyProgrammer ChunkyProgrammer ChunkyProgrammer approved these changes

@kommunarr kommunarr Awaiting requested review from kommunarr

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Prevent options being passed to external player's command
5 participants
@trostboot @absidue @PikachuEXE @ChunkyProgrammer @FreeTubeBot