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

change file-browser to directory script and bump min mpv version to v0.33 #100

Merged
merged 28 commits into from
Jan 8, 2024

Conversation

CogentRedTester
Copy link
Owner

@CogentRedTester CogentRedTester commented Nov 28, 2023

This PR aims to restructure file-browser into a directory script (see this section of the mpv manual). The main advantage of this change is to break up the code into smaller, more manageable files. Currently file-browser is exactly 2300 lines long, and it is becoming increasingly difficult to navigate the code.

This is a major restructuring that will result in the relocation of the vast majority of the code.

Changing file-browser to a directory script will also require bumping the minimum supported version of mpv to v0.33. I have been hesitant to increase the minimum version in the past, but v0.33 released just over three years ago now, so I think I've given people enough time to update. I'd be willing to bump the version number higher, but I don't see any new features that are really worth using in the base script except for the user-data property, and that was only added in v0.36 six months ago, which I consider a little too recent.

To do list:

  • Move options to module
  • Move global variables to module
  • Move utility functions to module
  • Move ass drawing code to module
  • Move caching code to module
  • Move in-built parsers to modules
  • Move directory navigation to module
  • Move directory parsing to module
  • Move addon API and setup to module
  • Move keybind setup and execution to module
  • Move playlist appending to module
  • Try and leave only environment setup, setup function calls, and mpv callbacks in main file
  • Update documentation with new install instructions and bumped version requirement
  • Make and document unsupported branch that contains the last version of the v0.31 compatible code

All I've done here is change the name of file-browser.lua to main.lua.
Future commits will split the script into multiple files.
I'll need to add some headers to these files at some point
The parser section of the environment section can probably be moved
into an addon module later.
The `highlight_entry` function will probably be moved to a different
module at some point.
This should reduce bugs by requiring uses of the global state to be
explicitly defined.
@CogentRedTester
Copy link
Owner Author

CogentRedTester commented Jan 8, 2024

With the latest commit all of the code has now been moved into modules, and the main.lua file has been made significantly tidier. There are still some refactoring and stylistic changes I would like to do, however this PR is getting very beefy, and I have other changes I'd like to make that are waiting on this.

The important thing is that all of the code has been moved over to modules with relatively few changes, and everything is (hopefully) working. As such I think it is satisfactory to merge with the code in the current state. The only thing to do first is to update the README with the new install instructions.

One thing to note is that this PR will actually break a couple of addons using undocumented features of the API, specifically it will likely break windows-sort and filter-recyclebin. On the other hand the new modules may make it easier to replicate the behaviour.

This includes the new minimum mpv version and the new installation
instructions. A link to the last
version of file-browser that supports mpv v0.31 and v0.32 has also been
linked.
@CogentRedTester
Copy link
Owner Author

@dyphire perhaps you'd like to take it for a spin before I merge? Might be nice to have an extra person confirm I haven't fundamentally broken something. 😅

@dyphire
Copy link
Contributor

dyphire commented Jan 8, 2024

@dyphire perhaps you'd like to take it for a spin before I merge? Might be nice to have an extra person confirm I haven't fundamentally broken something. 😅

Except for the script mentioned in #99 (comment) which is no longer available, everything else LGTM.

@CogentRedTester
Copy link
Owner Author

@dyphire I believe that changing that addon to the following should work. Unfortunately I'm not on windows so I can't confirm, can you try running it:

local fb_utils = require 'modules.utils'

-- this code is based on https://github.com/mpvnet-player/mpv.net/issues/575#issuecomment-1817413401
local ffi = require "ffi"
local winapi = {
        ffi = ffi,
        C = ffi.C,
        CP_UTF8 = 65001,
        shlwapi = ffi.load("shlwapi"),
    }

-- ffi code from https://github.com/po5/thumbfast, Mozilla Public License Version 2.0
ffi.cdef[[
    int __stdcall MultiByteToWideChar(unsigned int CodePage, unsigned long dwFlags, const char *lpMultiByteStr,
    int cbMultiByte, wchar_t *lpWideCharStr, int cchWideChar);
    int __stdcall StrCmpLogicalW(wchar_t *psz1, wchar_t *psz2);
]]

winapi.utf8_to_wide = function(utf8_str)
    if utf8_str then
        local utf16_len = winapi.C.MultiByteToWideChar(winapi.CP_UTF8, 0, utf8_str, -1, nil, 0)

        if utf16_len > 0 then
            local utf16_str = winapi.ffi.new("wchar_t[?]", utf16_len)

            if winapi.C.MultiByteToWideChar(winapi.CP_UTF8, 0, utf8_str, -1, utf16_str, utf16_len) > 0 then
                return utf16_str
            end
        end
    end

    return ""
end

fb_utils.sort = function (t)
    table.sort(t, function(a, b)
        local a_wide = winapi.utf8_to_wide(a.type:sub(1, 1) .. (a.label or a.name))
        local b_wide = winapi.utf8_to_wide(b.type:sub(1, 1) .. (b.label or b.name))
        return winapi.shlwapi.StrCmpLogicalW(a_wide, b_wide) == -1
    end)

    return t
end

return { version = '1.1.0' }

If you can confirm it's working then I'll merge and update those two addons. This is what you get when I write people addons using internal undocumented features 😅 (still better than adding all this bloat directly, however).

@dyphire
Copy link
Contributor

dyphire commented Jan 8, 2024

If you can confirm it's working then I'll merge and update those two addons. This is what you get when I write people addons using internal undocumented features 😅 (still better than adding all this bloat directly, however).

Yes, it works.

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

Successfully merging this pull request may close these issues.

2 participants