From ba35d818627a496aaec35fcc48e63bcdd396ebfd Mon Sep 17 00:00:00 2001 From: CogentRedTester Date: Fri, 4 Nov 2022 18:04:11 +1030 Subject: [PATCH 01/14] move mutable properties out of the state table these are properties that we don't really want to be immutable --- file-browser.lua | 49 ++++++++++++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 22 deletions(-) diff --git a/file-browser.lua b/file-browser.lua index 0b6352b..64d5cd6 100644 --- a/file-browser.lua +++ b/file-browser.lua @@ -188,23 +188,28 @@ local style = { } local state = { + -- lvl 1 values list = {}, - selected = 1, - hidden = true, - flag_update = false, - keybinds = nil, - parser = nil, directory = nil, directory_label = nil, prev_directory = "", co = nil, + --lvl 2 values + selected = 1, multiselect_start = nil, initial_selection = nil, selection = {} } +local overlay = { + hidden = true, + flag_update = false +} + +local keybinds = nil + --the parser table actually contains 3 entries for each parser --a numeric entry which represents the priority of the parsers and has the parser object as the value --a string entry representing the id of each parser and with the parser object as the value @@ -739,7 +744,7 @@ end --refreshes the ass text using the contents of the list local function update_ass() - if state.hidden then state.flag_update = true ; return end + if overlay.hidden then overlay.flag_update = true ; return end ass.data = style.global @@ -1170,15 +1175,15 @@ end --opens the browser local function open() - if not state.hidden then return end + if not overlay.hidden then return end - for _,v in ipairs(state.keybinds) do + for _,v in ipairs(keybinds) do mp.add_forced_key_binding(v[1], 'dynamic/'..v[2], v[3], v[4]) end utils.shared_script_property_set("file_browser-open", "yes") if o.toggle_idlescreen then mp.commandv('script-message', 'osc-idlescreen', 'no', 'no_osd') end - state.hidden = false + overlay.hidden = false if state.directory == nil then local path = mp.get_property('path') update_current_directory(nil, path) @@ -1186,28 +1191,28 @@ local function open() return end - if state.flag_update then update_current_directory(nil, mp.get_property('path')) end - if not state.flag_update then ass:update() - else state.flag_update = false ; update_ass() end + if overlay.flag_update then update_current_directory(nil, mp.get_property('path')) end + if not overlay.flag_update then ass:update() + else overlay.flag_update = false ; update_ass() end end --closes the list and sets the hidden flag local function close() - if state.hidden then return end + if overlay.hidden then return end - for _,v in ipairs(state.keybinds) do + for _,v in ipairs(keybinds) do mp.remove_key_binding('dynamic/'..v[2]) end utils.shared_script_property_set("file_browser-open", "no") if o.toggle_idlescreen then mp.commandv('script-message', 'osc-idlescreen', 'yes', 'no_osd') end - state.hidden = true + overlay.hidden = true ass:remove() end --toggles the list local function toggle() - if state.hidden then open() + if overlay.hidden then open() else close() end end @@ -1487,7 +1492,7 @@ end ------------------------------------------------------------------------------------------ ------------------------------------------------------------------------------------------ -state.keybinds = { +keybinds = { {'ENTER', 'play', function() open_file('replace', false) end}, {'Shift+ENTER', 'play_append', function() open_file('append-play', false) end}, {'Alt+ENTER', 'play_autoload',function() open_file('replace', true) end}, @@ -1746,7 +1751,7 @@ local function insert_custom_keybind(keybind) for code in string.gmatch(keybind.condition, KEYBIND_CODE_PATTERN) do keybind.condition_codes[code] = true end end - table.insert(state.keybinds, {keybind.key, keybind.name, function() run_keybind_coroutine(keybind) end, keybind.flags or {}}) + table.insert(keybinds, {keybind.key, keybind.name, function() run_keybind_coroutine(keybind) end, keybind.flags or {}}) top_level_keys[keybind.key] = keybind end @@ -1756,7 +1761,7 @@ local function setup_keybinds() if not o.custom_keybinds and not o.addons then return end --this is to make the default keybinds compatible with passthrough from custom keybinds - for _, keybind in ipairs(state.keybinds) do + for _, keybind in ipairs(keybinds) do top_level_keys[keybind[1]] = { key = keybind[1], name = keybind[2], command = keybind[3], flags = keybind[4] } end @@ -1885,7 +1890,7 @@ function API.get_current_parser() return state.parser:get_id() end function API.get_current_parser_keyname() return state.parser.keybind_name or state.parser.name end function API.get_selected_index() return state.selected end function API.get_selected_item() return API.copy_table(state.list[state.selected]) end -function API.get_open_status() return not state.hidden end +function API.get_open_status() return not overlay.hidden end function API.get_parse_state(co) return parse_states[co or coroutine.running() or ""] end function API.set_empty_text(str) @@ -2259,10 +2264,10 @@ end) --we don't want to add any overhead when the browser isn't open mp.observe_property('path', 'string', function(_,path) - if not state.hidden then + if not overlay.hidden then update_current_directory(_,path) update_ass() - else state.flag_update = true end + else overlay.flag_update = true end end) --updates the dvd_device From 2ddce6205fc05a2960525fea3949e43aa9129969 Mon Sep 17 00:00:00 2001 From: CogentRedTester Date: Fri, 4 Nov 2022 18:05:05 +1030 Subject: [PATCH 02/14] implement API.read_only function this will be what enforces immutability at runtime --- file-browser.lua | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/file-browser.lua b/file-browser.lua index 64d5cd6..e61dbab 100644 --- a/file-browser.lua +++ b/file-browser.lua @@ -626,6 +626,21 @@ function API.copy_table(t, depth) return copy_table_recursive(t, {}, depth or math.huge) end +--handles the read-only table logic +do + local references = setmetatable({}, { __mode = 'k' }) + local newindex = function(t, k, v) error(("attempted to assign %s to key %s in read-only table %s"):format(v, k, t)) end + + --returns a read-only reference to the table t + function API.read_only(t) + if references[t] then return references[t] end + + local ro = setmetatable({}, { __index = t, __newindex = newindex }) + references[t] = ro + return ro + end +end + -------------------------------------------------------------------------------------------------------- From 5991c4bd84c2360b5131d96df92224bb031c9642 Mon Sep 17 00:00:00 2001 From: CogentRedTester Date: Fri, 4 Nov 2022 18:10:06 +1030 Subject: [PATCH 03/14] implement function to handle state changes This is where the magic happens. Allowing for different levels of state changes while ensuring any specific state reference is immutable. Not yet tested for bugs. --- file-browser.lua | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/file-browser.lua b/file-browser.lua index e61dbab..d3411ad 100644 --- a/file-browser.lua +++ b/file-browser.lua @@ -252,6 +252,29 @@ local compatible_file_extensions = { -------------------------------------------------------------------------------------------------------- -------------------------------------------------------------------------------------------------------- +--the values table will be made readonly and set as part of the state - do not use values after passing to this function! +local function update_state(level, values) + local new_mt = { level = level } + setmetatable(values, new_mt) + + if level == 0 then + state = API.read_only(values) + return state + end + + --bypasses the readonly reference and grabs the original table and metatable + local mutable_state = getmetatable(state).__index + local mt = getmetatable(mutable_state) + + --travels up the state chain until it finds a mt of the same level as `level` + --this mt will be pointing to a state table of one level lower, which is what we want to point to + while (mt.level > level ) do mt = getmetatable(mt.__index) end + new_mt.__index = mt.__index + + state = API.read_only(values) + return state +end + --metatable of methods to manage the cache local __cache = {} From 37a7d76fb8ba86387d115f5018445a784260faee Mon Sep 17 00:00:00 2001 From: CogentRedTester Date: Fri, 4 Nov 2022 18:10:38 +1030 Subject: [PATCH 04/14] API.read_only: improve error message --- file-browser.lua | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/file-browser.lua b/file-browser.lua index d3411ad..9b5119f 100644 --- a/file-browser.lua +++ b/file-browser.lua @@ -652,7 +652,7 @@ end --handles the read-only table logic do local references = setmetatable({}, { __mode = 'k' }) - local newindex = function(t, k, v) error(("attempted to assign %s to key %s in read-only table %s"):format(v, k, t)) end + local newindex = function(t, k, v) error(("attempted to assign `%s` to key `%s` in read-only %s"):format(v, k, t)) end --returns a read-only reference to the table t function API.read_only(t) From 78264e6d7f8b5ec6efa9af5ac7991d5684eb889c Mon Sep 17 00:00:00 2001 From: CogentRedTester Date: Fri, 4 Nov 2022 23:35:28 +1030 Subject: [PATCH 05/14] add separate update and set state functions This makes it much easier to make incremental changes to the current state. Also added a print function to maker debugging state values easier. --- file-browser.lua | 70 +++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 61 insertions(+), 9 deletions(-) diff --git a/file-browser.lua b/file-browser.lua index 9b5119f..1e29098 100644 --- a/file-browser.lua +++ b/file-browser.lua @@ -252,8 +252,44 @@ local compatible_file_extensions = { -------------------------------------------------------------------------------------------------------- -------------------------------------------------------------------------------------------------------- +--this value can be used to remove values when updating the current state +local NIL_STATE = {} + +local function get_state(level) + --bypasses the readonly reference and grabs the original table and metatable + local s = getmetatable(state).__index + local mt = getmetatable(s) + + --travels up the state chain until it finds a mt of the same level as `level` + --this mt will be pointing to a state table of one level lower, which is what we want to point to + while mt.level > level do + s = mt.__index + mt = getmetatable(s) + + if not mt or not mt.level then return nil end + end + + return s, mt +end + +--prints the current state values +local function print_state() + local i = 0 + while true do + s, mt = get_state(i) + if not s then error('failed to get state of level '..i) end + if mt.level ~= i then break end + + for k, v in pairs(s) do + print(i, k, v) + end + + i = i + 1 + end +end + --the values table will be made readonly and set as part of the state - do not use values after passing to this function! -local function update_state(level, values) +local function set_state(level, values) local new_mt = { level = level } setmetatable(values, new_mt) @@ -262,19 +298,35 @@ local function update_state(level, values) return state end - --bypasses the readonly reference and grabs the original table and metatable - local mutable_state = getmetatable(state).__index - local mt = getmetatable(mutable_state) - - --travels up the state chain until it finds a mt of the same level as `level` - --this mt will be pointing to a state table of one level lower, which is what we want to point to - while (mt.level > level ) do mt = getmetatable(mt.__index) end - new_mt.__index = mt.__index + local s, mt = get_state(level) + if not mt then error('failed to get state of level '..level) end + new_mt.__index = mt.level == level and mt.__index or s state = API.read_only(values) return state end +--updates the current state values of the particular level +local function update_state(level, values) + local s, mt = get_state(level) + if not mt then error('failed to get state of level '..level) end + + if mt.level ~= level then + set_state(level, values) + return + end + + local new_state = API.copy_table(s, 1) + for k, v in pairs(values) do + if v == NIL_STATE then new_state[k] = nil + else new_state[k] = v end + end + + setmetatable(new_state, mt) + state = API.read_only(new_state) + return state +end + --metatable of methods to manage the cache local __cache = {} From 3a2d67df5afe5d8bfded21bad73b99170eca912b Mon Sep 17 00:00:00 2001 From: CogentRedTester Date: Fri, 4 Nov 2022 23:36:21 +1030 Subject: [PATCH 06/14] API.copy_table: now dereferences readonly table references The copies are not readonly. --- file-browser.lua | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/file-browser.lua b/file-browser.lua index 1e29098..afff3cf 100644 --- a/file-browser.lua +++ b/file-browser.lua @@ -374,6 +374,7 @@ API.coroutine = {} local ABORT_ERROR = { msg = "browser is no longer waiting for list - aborting parse" } +local newindex = function(t, k, v) error(("attempted to assign `%s` to key `%s` in read-only %s"):format(v, k, t), 2) end --implements table.pack if on lua 5.1 if not table.pack then @@ -685,8 +686,13 @@ local function copy_table_recursive(t, references, depth) if type(t) ~= "table" or depth == 0 then return t end if references[t] then return references[t] end + --bypasses the proxy protecting read-only tables + local mt = getmetatable(t) + local original_t + if mt and mt.__newindex == newindex then original_t = mt.__index end + local copy = setmetatable({}, { __original = t }) - references[t] = copy + references[original_t or t] = copy for key, value in pairs(t) do key = copy_table_recursive(key, references, depth - 1) @@ -704,7 +710,6 @@ end --handles the read-only table logic do local references = setmetatable({}, { __mode = 'k' }) - local newindex = function(t, k, v) error(("attempted to assign `%s` to key `%s` in read-only %s"):format(v, k, t)) end --returns a read-only reference to the table t function API.read_only(t) From a7d8a17e1fa1a4ea57440b062c156e85bac180d8 Mon Sep 17 00:00:00 2001 From: CogentRedTester Date: Sat, 5 Nov 2022 00:14:39 +1030 Subject: [PATCH 07/14] API.read_only: prevent changes to subtables and support the # operator --- file-browser.lua | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/file-browser.lua b/file-browser.lua index afff3cf..59641f6 100644 --- a/file-browser.lua +++ b/file-browser.lua @@ -195,6 +195,7 @@ local state = { directory_label = nil, prev_directory = "", co = nil, + empty_text = 'empty directory', --lvl 2 values selected = 1, @@ -257,7 +258,7 @@ local NIL_STATE = {} local function get_state(level) --bypasses the readonly reference and grabs the original table and metatable - local s = getmetatable(state).__index + local s = getmetatable(state).mutable local mt = getmetatable(s) --travels up the state chain until it finds a mt of the same level as `level` @@ -290,17 +291,20 @@ end --the values table will be made readonly and set as part of the state - do not use values after passing to this function! local function set_state(level, values) - local new_mt = { level = level } - setmetatable(values, new_mt) if level == 0 then + setmetatable(values, { level = 0 }) state = API.read_only(values) return state end local s, mt = get_state(level) if not mt then error('failed to get state of level '..level) end - new_mt.__index = mt.level == level and mt.__index or s + if mt.level == level then + setmetatable(values, mt) + else + setmetatable(values, { level = level, __index = s }) + end state = API.read_only(values) return state @@ -712,10 +716,20 @@ do local references = setmetatable({}, { __mode = 'k' }) --returns a read-only reference to the table t + --based on https://stackoverflow.com/a/28315547 function API.read_only(t) + if type(t) ~= 'table' then return t end if references[t] then return references[t] end - local ro = setmetatable({}, { __index = t, __newindex = newindex }) + local ro = setmetatable({}, { + __newindex = newindex, + mutable = t, + __index = function(_, k) + return API.read_only( t[k] ) + end, + __len = function () return #t end + }) + references[t] = ro return ro end From 5b25e8d97b6bf79b294b9a73264cb8e21f955014 Mon Sep 17 00:00:00 2001 From: CogentRedTester Date: Sat, 5 Nov 2022 00:15:22 +1030 Subject: [PATCH 08/14] switch basic file browsing to use immutable states Only the most basic operations have been ported, there is still a LOT of broken functionality. --- file-browser.lua | 175 ++++++++++++++++++++++++++++++----------------- 1 file changed, 114 insertions(+), 61 deletions(-) diff --git a/file-browser.lua b/file-browser.lua index 59641f6..d03778d 100644 --- a/file-browser.lua +++ b/file-browser.lua @@ -473,6 +473,15 @@ function API.coroutine.run(fn, ...) API.coroutine.resume_err(co, ...) end +--schedules a coroutine to run when next idle. +--returns the coroutine object +function API.coroutine.schedule(fn, ...) + local co = coroutine.create(fn) + local args = table.pack(...) + mp.add_timeout(0, function() API.coroutine.resume_err(co, table.unpack(args)) end) + return co +end + --get the full path for the current file function API.get_full_path(item, dir) if item.path then return item.path end @@ -951,38 +960,49 @@ end --disables multiselect local function disable_select_mode() - state.multiselect_start = nil - state.initial_selection = nil + -- state.multiselect_start = nil + -- state.initial_selection = nil + update_state(2, { + multiselect_start = NIL_STATE, + initial_selection = NIL_STATE + }) + print_state() end --enables multiselect local function enable_select_mode() - state.multiselect_start = state.selected - state.initial_selection = API.copy_table(state.selection) + update_state(2, { + multiselect_start = state.selected, + initial_selection = API.copy_table(state.selection) + }) + print_state() end --calculates what drag behaviour is required for that specific movement local function drag_select(original_pos, new_pos) if original_pos == new_pos then return end + local new_selection = API.copy_table(state.selection) local setting = state.selection[state.multiselect_start] for i = original_pos, new_pos, (new_pos > original_pos and 1 or -1) do --if we're moving the cursor away from the starting point then set the selection --otherwise restore the original selection if i > state.multiselect_start then if new_pos > original_pos then - state.selection[i] = setting + new_selection[i] = setting elseif i ~= new_pos then - state.selection[i] = state.initial_selection[i] + new_selection[i] = state.initial_selection[i] end elseif i < state.multiselect_start then if new_pos < original_pos then - state.selection[i] = setting + new_selection[i] = setting elseif i ~= new_pos then - state.selection[i] = state.initial_selection[i] + new_selection[i] = state.initial_selection[i] end end end + + return new_selection end --moves the selector up and down the list by the entered amount @@ -991,31 +1011,43 @@ local function scroll(n, wrap) if num_items == 0 then return end local original_pos = state.selected + local new_pos + local new_multiselect if original_pos + n > num_items then - state.selected = wrap and 1 or num_items + new_pos = wrap and 1 or num_items elseif original_pos + n < 1 then - state.selected = wrap and num_items or 1 + new_pos = wrap and num_items or 1 else - state.selected = original_pos + n + new_pos = original_pos + n + end + + if state.multiselect_start then + new_multiselect = drag_select(original_pos, new_pos) end - if state.multiselect_start then drag_select(original_pos, state.selected) end + update_state(2, { selected = new_pos, selection = new_multiselect }) update_ass() end --toggles the selection local function toggle_selection() if not state.list[state.selected] then return end - state.selection[state.selected] = not state.selection[state.selected] or nil + -- state.selection[state.selected] = not state.selection[state.selected] or nil + local selection = API.copy_table(state.selection) + selection[state.selected] = not selection[state.selected] or nil + update_state(2, { selection = selection }) update_ass() end --select all items in the list local function select_all() + local selection = {} for i,_ in ipairs(state.list) do - state.selection[i] = true + selection[i] = true end + + update_state(2, {selection = selection}) update_ass() end @@ -1133,18 +1165,18 @@ end local function update_list(moving_adjacent) msg.verbose('opening directory: ' .. state.directory) - state.selected = 1 - state.selection = {} + -- state.selected = 1 + -- state.selection = {} --loads the current directry from the cache to save loading time --there will be a way to forcibly reload the current directory at some point --the cache is in the form of a stack, items are taken off the stack when the dir moves up - if cache[1] and cache[#cache].directory == state.directory then - msg.verbose('found directory in cache') - cache:apply() - state.prev_directory = state.directory - return - end + -- if cache[1] and cache[#cache].directory == state.directory then + -- msg.verbose('found directory in cache') + -- cache:apply() + -- state.prev_directory = state.directory + -- return + -- end local directory = state.directory local list, opts = parse_directory(state.directory, { source = "browser" }) @@ -1157,19 +1189,21 @@ local function update_list(moving_adjacent) end --apply fallbacks if the scan failed - if not list and cache[1] then - --switches settings back to the previously opened directory - --to the user it will be like the directory never changed - msg.warn("could not read directory", state.directory) - cache:apply() - return - elseif not list then + -- if not list and cache[1] then + -- --switches settings back to the previously opened directory + -- --to the user it will be like the directory never changed + -- msg.warn("could not read directory", state.directory) + -- cache:apply() + -- return + if not list then msg.warn("could not read directory", state.directory) list, opts = root_parser:parse() end - state.list = list - state.parser = opts.parser + local finished_state = {} + + finished_state.list = list + finished_state.parser = opts.parser --this only matters when displaying the list on the screen, so it doesn't need to be in the scan function if not opts.escaped then @@ -1179,54 +1213,71 @@ local function update_list(moving_adjacent) end --setting custom options from parsers - state.directory_label = opts.directory_label - state.empty_text = opts.empty_text or state.empty_text + finished_state.directory_label = opts.directory_label + finished_state.empty_text = opts.empty_text --we assume that directory is only changed when redirecting to a different location --therefore, the cache should be wiped if opts.directory then - state.directory = opts.directory + finished_state.directory = opts.directory cache:clear() end if opts.selected_index then - state.selected = opts.selected_index or state.selected - if state.selected > #state.list then state.selected = #state.list - elseif state.selected < 1 then state.selected = 1 end + finished_state.selected = opts.selected_index or state.selected + if finished_state.selected > #list then finished_state.selected = #list + elseif finished_state.selected < 1 then finished_state.selected = 1 end end if moving_adjacent then select_prev_directory() else select_playing_item() end - state.prev_directory = state.directory + finished_state.prev_directory = finished_state.directory + + print(utils.to_string(finished_state)) + return finished_state end --rescans the folder and updates the list -local function update(moving_adjacent) +local function update(new_state, moving_adjacent) + if not new_state then new_state = {} end + local directory = new_state.directory or state.directory + cache:clear() + --we can only make assumptions about the directory label when moving from adjacent directories - if not moving_adjacent then - state.directory_label = nil - cache:clear() - end + -- if not moving_adjacent then + -- state.directory_label = nil + -- cache:clear() + -- end - state.empty_text = "~" - state.list = {} - disable_select_mode() - update_ass() + new_state.directory = directory + new_state.empty_text = "~" + new_state.list = {} + + -- set_state(1, new_state) + -- -- disable_select_mode() + -- update_ass() --the directory is always handled within a coroutine to allow addons to --pause execution for asynchronous operations - API.coroutine.run(function() - state.co = coroutine.running() - update_list(moving_adjacent) - state.empty_text = "empty directory" + new_state.co = API.coroutine.schedule(function() + local newer_state = update_list(moving_adjacent) + if not newer_state then error() end + newer_state.empty_text = newer_state.empty_text or NIL_STATE + update_state(1, newer_state) + print_state() + update_ass() end) + + set_state(1, new_state) + print_state() + update_ass() end --the base function for moving to a directory local function goto_directory(directory) - state.directory = directory - update(false) + -- state.directory = directory + update({ directory = directory }, false) end --loads the root list @@ -1251,13 +1302,14 @@ local function up_dir() index = dir:find("[/\\]") end - if index == nil then state.directory = "" - else state.directory = dir:sub(index):reverse() end + if index == nil then dir = "" + else dir = dir:sub(index):reverse() end --we can make some assumptions about the next directory label when moving up or down - if state.directory_label then state.directory_label = state.directory_label:match("^(.+/)[^/]+/$") end + local dir_label = nil + if state.directory_label then dir_label = state.directory_label:match("^(.+/)[^/]+/$") end - update(true) + update({ directory = dir, directory_label = dir_label } ,true) cache:pop() end @@ -1268,11 +1320,11 @@ local function down_dir() cache:push() local directory, redirected = API.get_new_directory(current, state.directory) - state.directory = directory --we can make some assumptions about the next directory label when moving up or down - if state.directory_label then state.directory_label = state.directory_label..(current.label or current.name) end - update(not redirected) + local dir_label + if state.directory_label then dir_label= state.directory_label..(current.label or current.name) end + update({ directory = directory, directory_label = dir_label }, not redirected) end @@ -1330,7 +1382,7 @@ local function escape() --if multiple items are selection cancel the --selection instead of closing the browser if next(state.selection) or state.multiselect_start then - state.selection = {} + update_state(2, { selection = {} }) disable_select_mode() update_ass() return @@ -2247,6 +2299,7 @@ local function setup_root() end end +set_state(0, state) setup_root() setup_parser(file_parser, "file-browser.lua") From 5505d420af4a86784b0116c07b4623b79ead3d1b Mon Sep 17 00:00:00 2001 From: CogentRedTester Date: Sat, 5 Nov 2022 17:25:58 +1030 Subject: [PATCH 09/14] API.copy_table now properly copies read-only tables --- file-browser.lua | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/file-browser.lua b/file-browser.lua index d03778d..11ef9ce 100644 --- a/file-browser.lua +++ b/file-browser.lua @@ -697,15 +697,14 @@ end --uses a structured clone algorithm to maintain cyclic references local function copy_table_recursive(t, references, depth) if type(t) ~= "table" or depth == 0 then return t end - if references[t] then return references[t] end --bypasses the proxy protecting read-only tables local mt = getmetatable(t) - local original_t - if mt and mt.__newindex == newindex then original_t = mt.__index end + if mt and mt.__newindex == readonly_newindex then t = mt.mutable end + if references[t] then return references[t] end local copy = setmetatable({}, { __original = t }) - references[original_t or t] = copy + references[t] = copy for key, value in pairs(t) do key = copy_table_recursive(key, references, depth - 1) From a051677f5cf08d172451331d08fcb21a02a1d2f7 Mon Sep 17 00:00:00 2001 From: CogentRedTester Date: Sat, 5 Nov 2022 17:28:55 +1030 Subject: [PATCH 10/14] Allow `pairs`, `ipairs`, and `next` to work with read-only tables --- file-browser.lua | 46 +++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 43 insertions(+), 3 deletions(-) diff --git a/file-browser.lua b/file-browser.lua index 11ef9ce..7722df2 100644 --- a/file-browser.lua +++ b/file-browser.lua @@ -253,6 +253,33 @@ local compatible_file_extensions = { -------------------------------------------------------------------------------------------------------- -------------------------------------------------------------------------------------------------------- +local original_ipairs = _G.ipairs +local original_next = _G.next +local original_pairs = _G.pairs + +--adds ipairs support for readonly tables +function ipairs(t) + if not API.is_read_only(t) then return original_ipairs(t) end + + local function iter (a, i) + i = i + 1 + if a[i] then return API.read_only_values(i, a[i]) end + end + return iter, t, 0 +end + +--adds next support for readonly tables +function next(t, ...) + if API.is_read_only(t) then return API.read_only_values(original_next(getmetatable(t).mutable, ...)) + else return original_next(t, ...) end +end + +--this only needs to use the new next function in order to support readonly tables +function pairs(t) + if API.is_read_only(t) then return next, t, nil + else return original_pairs(t) end +end + --this value can be used to remove values when updating the current state local NIL_STATE = {} @@ -378,7 +405,7 @@ API.coroutine = {} local ABORT_ERROR = { msg = "browser is no longer waiting for list - aborting parse" } -local newindex = function(t, k, v) error(("attempted to assign `%s` to key `%s` in read-only %s"):format(v, k, t), 2) end +local readonly_newindex = function(t, k, v) error(("attempted to assign `%s` to key `%s` in read-only %s"):format(v, k, t), 2) end --implements table.pack if on lua 5.1 if not table.pack then @@ -730,7 +757,7 @@ do if references[t] then return references[t] end local ro = setmetatable({}, { - __newindex = newindex, + __newindex = readonly_newindex, mutable = t, __index = function(_, k) return API.read_only( t[k] ) @@ -743,6 +770,19 @@ do end end +--returns read-only references to all given values +function API.read_only_values(...) + local vals = table.pack(...) + for i, v in ipairs(vals) do vals[i] = API.read_only(v) end + return table.unpack(vals) +end + +--returns true if the given tale is read-only +function API.is_read_only(t) + local mt = getmetatable(t) + return mt and mt.__newindex == readonly_newindex +end + -------------------------------------------------------------------------------------------------------- @@ -1042,7 +1082,7 @@ end --select all items in the list local function select_all() local selection = {} - for i,_ in ipairs(state.list) do + for i in ipairs(state.list) do selection[i] = true end From 2d75f91af33875f434ad669eabcfa7a5e448b342 Mon Sep 17 00:00:00 2001 From: CogentRedTester Date: Sat, 5 Nov 2022 18:01:31 +1030 Subject: [PATCH 11/14] fix memory leak from readonly cache Since the readonly tables contain a reference to the original table we can't make the keys weak, at least not in Lua5.1. In Lua 5.2 weakly keyed tables act as ephemeron tables, so they would probably work there. I don't have a Lua5.2 build of mpv, so I can't test, but it might be worth using ephemeral tables when Lua5.2 is detected. --- file-browser.lua | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/file-browser.lua b/file-browser.lua index 7722df2..3d154f6 100644 --- a/file-browser.lua +++ b/file-browser.lua @@ -748,7 +748,7 @@ end --handles the read-only table logic do - local references = setmetatable({}, { __mode = 'k' }) + local references = setmetatable({}, { __mode = 'kv' }) --returns a read-only reference to the table t --based on https://stackoverflow.com/a/28315547 From d0eb295dcadaf54d19428ae8179f39b2ce4cf6fa Mon Sep 17 00:00:00 2001 From: CogentRedTester Date: Sat, 5 Nov 2022 18:18:51 +1030 Subject: [PATCH 12/14] ass: use a string buffer when drawing the browser String manipulation is quite costly in Lua, this significantly reduces the number of garbage string produced and should help to speed up scrolling and reduce garbage collection. Reducing garbage collection is quite important since it directly impacts the performance of read-only tables lookups by clearing the cache. --- file-browser.lua | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/file-browser.lua b/file-browser.lua index 3d154f6..2c73f63 100644 --- a/file-browser.lua +++ b/file-browser.lua @@ -859,15 +859,22 @@ local file_parser = { -------------------------------------------------------------------------------------------------------- -------------------------------------------------------------------------------------------------------- +local string_buffer = {} + --appends the entered text to the overlay local function append(text) if text == nil then return end - ass.data = ass.data .. text + table.insert(string_buffer, text) end --appends a newline character to the osd local function newline() -ass.data = ass.data .. '\\N' + table.insert(string_buffer, '\\N') +end + +local function flush_buffer() + ass.data = table.concat(string_buffer, '') + string_buffer = {} end --detects whether or not to highlight the given entry as being played @@ -903,7 +910,7 @@ end local function update_ass() if overlay.hidden then overlay.flag_update = true ; return end - ass.data = style.global + append(style.global) local dir_name = state.directory_label or state.directory if dir_name == "" then dir_name = "ROOT" end @@ -914,6 +921,7 @@ local function update_ass() if #state.list < 1 then append(state.empty_text) + flush_buffer() ass:update() return end @@ -987,6 +995,8 @@ local function update_ass() end if overflow then append('\\N'..style.footer_header..#state.list-finish..' item(s) remaining') end + + flush_buffer() ass:update() end From c92b3c1b8e47ca21f831f19b9242c40ffd14d167 Mon Sep 17 00:00:00 2001 From: CogentRedTester Date: Sat, 5 Nov 2022 18:19:50 +1030 Subject: [PATCH 13/14] API.copy_table: add option to not store original --- file-browser.lua | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/file-browser.lua b/file-browser.lua index 2c73f63..1e23909 100644 --- a/file-browser.lua +++ b/file-browser.lua @@ -347,7 +347,7 @@ local function update_state(level, values) return end - local new_state = API.copy_table(s, 1) + local new_state = API.copy_table(s, 1, false) for k, v in pairs(values) do if v == NIL_STATE then new_state[k] = nil else new_state[k] = v end @@ -722,7 +722,7 @@ end --copies a table without leaving any references to the original --uses a structured clone algorithm to maintain cyclic references -local function copy_table_recursive(t, references, depth) +local function copy_table_recursive(t, references, depth, store_original) if type(t) ~= "table" or depth == 0 then return t end --bypasses the proxy protecting read-only tables @@ -730,20 +730,20 @@ local function copy_table_recursive(t, references, depth) if mt and mt.__newindex == readonly_newindex then t = mt.mutable end if references[t] then return references[t] end - local copy = setmetatable({}, { __original = t }) + local copy = setmetatable({}, { __original = store_original and t }) references[t] = copy for key, value in pairs(t) do - key = copy_table_recursive(key, references, depth - 1) - copy[key] = copy_table_recursive(value, references, depth - 1) + key = copy_table_recursive(key, references, depth - 1, store_original) + copy[key] = copy_table_recursive(value, references, depth - 1, store_original) end return copy end --a wrapper around copy_table to provide the reference table -function API.copy_table(t, depth) +function API.copy_table(t, depth, store_original) --this is to handle cyclic table references - return copy_table_recursive(t, {}, depth or math.huge) + return copy_table_recursive(t, {}, depth or math.huge, store_original or true) end --handles the read-only table logic @@ -1022,7 +1022,7 @@ end local function enable_select_mode() update_state(2, { multiselect_start = state.selected, - initial_selection = API.copy_table(state.selection) + initial_selection = API.copy_table(state.selection, nil, false) }) print_state() end @@ -1031,7 +1031,7 @@ end local function drag_select(original_pos, new_pos) if original_pos == new_pos then return end - local new_selection = API.copy_table(state.selection) + local new_selection = API.copy_table(state.selection, nil, false) local setting = state.selection[state.multiselect_start] for i = original_pos, new_pos, (new_pos > original_pos and 1 or -1) do --if we're moving the cursor away from the starting point then set the selection @@ -1083,7 +1083,7 @@ end local function toggle_selection() if not state.list[state.selected] then return end -- state.selection[state.selected] = not state.selection[state.selected] or nil - local selection = API.copy_table(state.selection) + local selection = API.copy_table(state.selection, nil, false) selection[state.selected] = not selection[state.selected] or nil update_state(2, { selection = selection }) update_ass() @@ -1282,7 +1282,7 @@ local function update_list(moving_adjacent) else select_playing_item() end finished_state.prev_directory = finished_state.directory - print(utils.to_string(finished_state)) + -- print(utils.to_string(finished_state)) return finished_state end From 781ce3f38b49487aae56dbf1aa3d9405f6b6f397 Mon Sep 17 00:00:00 2001 From: CogentRedTester Date: Sat, 5 Nov 2022 18:44:13 +1030 Subject: [PATCH 14/14] read-only tables: store all indexed sub-tables in upvalue This prevents the sub-tables from being garbage collected from the cache until the base read-only table has been garbage collected. This should significantly reduce the number of new read-only tables that are created while scrolling. --- file-browser.lua | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/file-browser.lua b/file-browser.lua index 1e23909..2addf83 100644 --- a/file-browser.lua +++ b/file-browser.lua @@ -748,24 +748,28 @@ end --handles the read-only table logic do - local references = setmetatable({}, { __mode = 'kv' }) + local readonly_cache = setmetatable({}, { __mode = 'kv' }) --returns a read-only reference to the table t --based on https://stackoverflow.com/a/28315547 function API.read_only(t) if type(t) ~= 'table' then return t end - if references[t] then return references[t] end + if readonly_cache[t] then return readonly_cache[t] end + --this prevents the garbage collector from removing sub-tables from the cache + local internal_record = {} local ro = setmetatable({}, { __newindex = readonly_newindex, mutable = t, __index = function(_, k) - return API.read_only( t[k] ) + local val = API.read_only( t[k] ) + if type(val) == 'table' then internal_record[val] = true end + return val end, __len = function () return #t end }) - references[t] = ro + readonly_cache[t] = ro return ro end end