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

Feat/mpv profiles #108

Merged
merged 16 commits into from
Nov 19, 2023
Merged

Feat/mpv profiles #108

merged 16 commits into from
Nov 19, 2023

Conversation

DanSM-5
Copy link
Contributor

@DanSM-5 DanSM-5 commented Oct 18, 2023

Feature: mpv profiles

Add a set of command line arguments into its own profile to be called in a new entry in the context menu of ff2mpv.

Issue

I've had the need to call mpv with different command line arguments due to the different resolutions of my monitors and to avoid some issues with hls in youtube yt-dlp/yt-dlp#7824 (I can't open the issue anymore but it is listed here yt-dlp/yt-dlp#3766).

Motivation

I found having to change mpv.conf or ff2mpv.py every time I needed to change some command line arguments a bit annoying. While using mpv directly from the command line is fine, I wanted to have the convenience of ff2mpv's right click play. I think I found a good solution and I think other users my benefit from this.

Solution

Adding new entries in the context menu for mpv per each profile created by the user. In order to create a profile you need to add it manually in the addon/extension options. A profile is simply a name and a set of command line arguments.
If the given profile is clicked, it will send the options to the native client which just forward the arguments to the mpv command.
Current behavior is preserved with the default profile "Play in MPV" which doesn't add any additional argument.

Demo

https://drive.google.com/file/d/1WOV7gpezys_B4Ox43H4lChlM0mbdjim3/view?usp=share_link

@DanSM-5
Copy link
Contributor Author

DanSM-5 commented Oct 18, 2023

@woodruffw Let me know what you think.

@woodruffw woodruffw self-requested a review October 18, 2023 23:45
@woodruffw
Copy link
Owner

Thanks @DanSM-5! I really like the demo.

I'll give the code itself a review tomorrow.

@DanSM-5
Copy link
Contributor Author

DanSM-5 commented Oct 20, 2023

I realized I forgot to update the ruby client. I added what looks correct based on some research I did but I'd appreciate if you could validate the functionality.

@DanSM-5
Copy link
Contributor Author

DanSM-5 commented Oct 20, 2023

I found an issue. In summary, the profile.content array that I was trying to leave in the closure was causing a reference to dead object. I found that it happened because the profile object is disposed at the moment you close the options menu of the extension.

I did that on purpose to avoid an extra lookup in the storage but that seems like the best alternative to avoid the issue. I've pushed the commit.

Image for reference of the dead object:
image

@DanSM-5
Copy link
Contributor Author

DanSM-5 commented Oct 28, 2023

@woodruffw did you get a chance to review the PR?

@woodruffw
Copy link
Owner

Not yet, sorry. It's on my list of things to do tomorrow.

Copy link
Owner

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

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

Sorry for the delay here @DanSM-5!

This looks really good overall; I've left a few nitpicks and thoughts.

options/options.js Outdated Show resolved Hide resolved
options/options.html Outdated Show resolved Hide resolved
options/options.js Outdated Show resolved Hide resolved
Copy link
Owner

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for your hard work here, @DanSM-5!

@woodruffw woodruffw merged commit 7f0eb17 into woodruffw:master Nov 19, 2023
8 checks passed
@woodruffw
Copy link
Owner

Merged. Let me know if you'd like a release here, otherwise I'll probably do it sometime next week.

@DanSM-5
Copy link
Contributor Author

DanSM-5 commented Nov 19, 2023

@woodruffw feel free to do the release next week.

@eNV25 eNV25 mentioned this pull request Dec 6, 2023
@woodruffw
Copy link
Owner

I completely forgot to release this. I'll try and remember to do this tonight...

@DanSM-5
Copy link
Contributor Author

DanSM-5 commented Jan 20, 2024

No worries. Originally I wanted the feature before going out in a trip but I ended up with almost no time to watch youtube, so it didn't affect me.

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.

2 participants