woodruffw / ff2mpv

A Firefox/Chrome add-on for playing URLs in mpv.
https://addons.mozilla.org/en-US/firefox/addon/ff2mpv/
Other
521 stars 52 forks source link

Feature add install script for Windows #77

Closed DanSM-5 closed 2 years ago

DanSM-5 commented 2 years ago

Windows installation script.

This should make installation a smooth process for windows users. I made it to behave as the bash installation scripts.

Example: > .\install.ps1 firefox Will add the registry key for ff2mpv in the NativeMessagingHost for firefox.

Featured browsers

Other features

woodruffw commented 2 years ago

Thanks for the PR!

Is there any functional overlap between this script and check-config-win.py? I believe another Windows user contributed that earlier script to manage the registry settings for ff2mpv.

Assuming there is some overlap, I'm not opposed to deleting that old script and completely replacing it with your work (especially since the old one doesn't support anything but Firefox).

DanSM-5 commented 2 years ago

The python script does 3 things

It mainly overlaps with the installation part but only for firefox as the powershell script supports more browsers and will generate the JSON's for chromium based browsers.

I didn't add a command to just check the configuration (like checking for mpv). I added a check for python mainly because in windows this may vary and the batch script won't work unless it uses the right command in the specific system. It would be simple add a check for MPV and just log a message if it is not found. Would this be desired?

Finally, the powershell script only does installations but it won't add the registry key gain if it is already present or it will update the value if you move the location of the repository. I could later implement a flag to allow removing the key. Other changes are local to the repository (like creating a JSON or modifying the bat script) and follow the same conventions as the sh scrips to keep it consistent.

DanSM-5 commented 2 years ago

I made an update so the powershell script:

I believe that now the python script can be replaced and all its functions should be cover.

woodruffw commented 2 years ago

I believe that now the python script can be replaced and all its functions should be cover.

Thank you! Would you mind deleting the Python script as part of this changeset?

I'll do a final review pass tonight as well, but this LGTM overall.

DanSM-5 commented 2 years ago

Done. While at it I added option to display help or exit the prompt to make it more user friendly.

woodruffw commented 2 years ago

CI failure is caused by the deletion. Feel free to delete that line from the CI if you have a moment, otherwise I'll clean it up post-merge.

DanSM-5 commented 2 years ago

No worries, I found where the configuration was. I removed the check for the script in ci.yml.

woodruffw commented 2 years ago

Thanks again!

DanSM-5 commented 2 years ago

I'll update the wiki over the weekend with usage about the new installation script on Windows.

woodruffw commented 2 years ago

Sounds good, thanks for doing that.

Sent from mobile. Please excuse my brevity.

On Mar 4, 2022, at 8:44 PM, DanSM-5 @.***> wrote:

 I'll update the wiki over the weekend with usage about the new installation script on Windows.

— Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you modified the open/close state.