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

Put the plugin into the script folder #102

Closed
dyphire opened this issue Jan 9, 2024 · 4 comments
Closed

Put the plugin into the script folder #102

dyphire opened this issue Jan 9, 2024 · 4 comments

Comments

@dyphire
Copy link
Contributor

dyphire commented Jan 9, 2024

As commented in #100, it doesn't seem to make much sense to separate plugins after refactoring the script into a directory script. Putting it in the script folder seems more reasonable, but it will once again disrupt downward compatibility.

@CogentRedTester
Copy link
Owner

What exactly are you suggesting? Moving where addons are placed?

@dyphire
Copy link
Contributor Author

dyphire commented Jan 9, 2024

What exactly are you suggesting? Moving where addons are placed?

Yes, quoting my previous review suggestions:

according to the installation instructions, maybe it should be changed to:

addon_directory = "~~/scripts/file-browser/addons"

I prefer to remove this option and apply the following patch in addon.lua:

- local addon_dir = mp.command_native({"expand-path", o.addon_directory..'/'})
+ local addon_dir = mp.get_script_directory() .. '/addons/'

@dyphire
Copy link
Contributor Author

dyphire commented Jan 9, 2024

If you choose to maintain the current logic, please consider changing the folder structure of the project to the following more reasonable composition:

├───addons
├───docs
├───file-browser
└───screenshots

@CogentRedTester
Copy link
Owner

Yes, quoting my previous review suggestions:

Sorry, I don't see any review suggestions on that PR, are you sure that you published them? If you had any suggested changes feel free to make a new PR that contains them, I can't say I'll follow them but I'd value the feedback.


If you choose to maintain the current logic, please consider changing the folder structure of the project to the following more reasonable composition:

├───addons
├───docs
├───file-browser
└───screenshots

The problem with this file structure is that this repository would not longer be easily clonable into the scripts directory as the main.lua file would no longer be at the top level. I really like that I can now have a direct clone of this repository in the script directory, and this is actually the suggested way to structure directory scripts in the mpv manual:

Using a script directory is the recommended way to package a script that consists of multiple source files, or requires other files (you can use mp.get_script_directory() to get the location and e.g. load data files).

Making a script a git repository, basically a repository which contains a main.lua file in the root directory, makes scripts easily updateable (without the dangers of auto-updates). Another suggestion is to use git submodules to share common files or libraries.

Changing the name of modules to file-browser or some equivalent might be a good idea though, less ambiguous.
Alternatively, the addons could be moved to a separate repository. I have considered it in the past but it never really seemed worth the effort.


according to the installation instructions, maybe it should be changed to:

addon_directory = "~~/scripts/file-browser/addons"

I prefer to remove this option and apply the following patch in addon.lua:

- local addon_dir = mp.command_native({"expand-path", o.addon_directory..'/'})
+ local addon_dir = mp.get_script_directory() .. '/addons/'

I did consider that while working on the PR, but I think there are a number of problems with the idea.

Firstly, I think there are some problems that come with conflating the directory where we store the addons, with the directory they are loaded from. If we made this change, then when users enable addons all of the addons maintained in the repository will be run by default. This is not ideal as many of the addons in that list designed for entirely different operating systems, and some will throw errors if required files and programs have not been configured or installed. We could tell people to remove the addons they don't want, but in my experience that doesn't work; there are several issues reports where people have posted file-browser-keybinds.json files that still have the example bindings inside despite telling people to remove them in the README. Like it or not, if we set that directory to default then people would treat the files in there as the recommended default addons to enable, and I don't those addons are stable or supported enough to be the case.

This also creates an issue when it comes to updating file-browser. If people do remove addons from that folder, then each update would replace them again by default. Additional, addons added manually might cause conflict with future addons added to this repository. I'd also generally like to avoid asking people to pollute a clone of this repository when enabling addons.

So for those reasons I think it is best to keep the current default. However, it is of course possible for anyone to manually switch their addon directory to ~~/scripts/file-browser/addons/.

@dyphire dyphire closed this as completed Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants