occivink / mpv-gallery-view

Gallery-view scripts for mpv
The Unlicense
189 stars 20 forks source link

Package this script so it can be run out of a directory inside mpv's script folder #38

Closed microraptor closed 2 years ago

microraptor commented 2 years ago

Thank you for thiese awesome mpv scripts. I have a suggestion how to tidy up the folder structure and make the lib.disabled directory unnecessary (I almost deleted it, because the name suggests it isn't needed).

Since mpv build v0.33.1 (Apr 05, 2021) it is possible for scripts to be run out a folder inside the script directory. This is recommended by the mpv manual (second last §): https://mpv.io/manual/master/#script-location This makes installation very easy and elegant via git clone right into scripts folder. Since that release v0.33.1 includes security fixes, people should upgrade to it anyways.

All files would be neatly packed into this one folder and the gallery-library would be just alongside them. This import code of contact-sheet.lua and gallery-view could be deleted, because the library would be already in the package path:

local lib = mp.find_config_file('scripts/lib.disable')
if not lib then
    return
end
-- lib can be nil if the folder does not exist or we're in --no-config mode
package.path = package.path .. ';' .. lib .. '/?.lua;'

For it the scripts to load a main.lua is required which would optimally look something like this:

--[[
mpv-gallery-view | https://github.com/occivink/mpv-gallery-view
Playlist view and contact sheet scripts for mpv.
Assigns the script-bindings 'g' and 'c' by default.
]]

local gallery_thumbgen = require 'gallery-thumbgen'
local playlist_view = require 'playlist-view'
local contact_sheet = require 'contact-sheet'

-- Comment out functionalities, you don't want
gallery_thumbgen.init(0)
gallery_thumbgen.init(1)
gallery_thumbgen.init(2)
gallery_thumbgen.init(3)
contact_sheet.init()
playlist_view.init()

However for this these three files have to be changed to lua modules with an init function like that.

I tried a very simple main.lua like this, but just couldn't get it to work:

require 'gallery-thumbgen'
require 'contact-sheet'

The .conf files could be just in a subdirectory of the repo called conf with a readme.txt stating: If you want to use these config files, place them in the "script-opts" directory inside mpv's configuration directory and edit them. Create the directory if not already present.

For an example of a mpv script which is installed with a directory see: https://github.com/ctlaltdefeat/mpv-open-imdb-page

occivink commented 2 years ago

Hello,

Thanks for your interest, I wasn't aware of this 'directory-script' feature of mpv Fundamentally, the scripts are split that way because

Point 1 is non-negotiable at the moment, you cannot run the the thumbgen and gallery from the same 'script', especially because the thumbgen scripts hijack the event loop. Point 2 is a maybe. Off the top of my head, there is nothing preventing you from loading both the contact-sheet and playlist-view from the same main.lua, since they're both good mpv citizens. However, some functionality is duplicated between them so you might run into issues. Merging both into one script could actually be a good way to cut this duplication. Point 3 will also probably stay that way. It's unfortunate that it makes the installation slightly more complicated, but I think it's a good way to enforce a 'clean' API.

I'll think about it all some more, but I think there is indeed potential for organizing this repository in a cleaner way.

microraptor commented 2 years ago

Thank you for taking your time and explaining your thoughts on it. You make some good points I haven't considered and it sounds like it is best solution not to use a script directory for now.

Personally my issue with the current structure is the confusing lib.disable folder and that there are 4 (+ extra workers) items inside the auto-loading scripts directory, which I like to keep clean and organized, for only two scripts.

https://github.com/CogentRedTester/ made shared modules (mpv-read-file, mpv-scroll-list, mpv-user-input), which are used by multiple script's they made. Their solution is to put the modules into a directory called script-modules at the root of the mpv's config path.

I made a branch where I moved the gallery.lua and it works without issues: https://github.com/microraptor/mpv-gallery-view/commit/cff8ef478b1baed90ccb06f61e73cf01899c06fa

I used CogentRedTester's method of expanding the path for the modules in that branch. I am not sure if it is actually better.

Then maybe sometime in the future the thumbgen script could be integrated into the scripts or module and the amount of workers configured in .conf files.

To make things more organized I would suggest adding a comment-header to every file, so people know what script/repo it belongs to. Something like this at the start of every .lua file:

--[[
mpv-gallery-view | https://github.com/occivink/mpv-gallery-view

This mpv script generates and displays a contact sheet of a video.

File placement: scripts/contact-sheet.lua
Settings: script-opts/contact-sheet.conf
Requires: script-modules/gallery-module.lua
Default keybinding: c script-binding contact-sheet-toggle
]]
occivink commented 2 years ago

I think these are good ideas yes. If you want to make a PR I'd accept it.

microraptor commented 2 years ago

Done