select.lua: update the playlist submenu after playlist-move #17336
Conversation
guidocella
force-pushed
the
playlist-move-fix
branch
2 times, most recently
from
e3f2299 to
7b36d8e
Compare
|
The only downside of this is that it updates |
Observing playlist-pos and playlist-count is not enough to update it
after playlist-move, but we can observe any playlist property with type
none to get notified of every MP_EVENT_CHANGE_PLAYLIST, even if the
property has not changed.
This is similar to the
mp.observe_property("vsync-jitter", "none", recorder) hack used by
stats.lua to replicate the deprecated tick event.
Fixes mpv-player#16816 (comment)
guidocella
force-pushed
the
playlist-move-fix
branch
from
7b36d8e to
01edd7c
Compare
| if not observed_properties[name] then | ||
| local result, err = mp.get_property_native(name) | ||
| function _G.get(name, default, none) | ||
| local key = name .. (none and " none" or "") |
There was a problem hiding this comment.
Why this? You'd rather not observe it twice, no?
There was a problem hiding this comment.
There was a problem hiding this comment.
Put into observed_properties[key] type of observation and assert that it doesn't change, instead of allowing both observe with all this changes
There was a problem hiding this comment.
So this?
diff --git a/player/lua/select.lua b/player/lua/select.lua
index 5784fcdfb9..73bff72eab 100644
--- a/player/lua/select.lua
+++ b/player/lua/select.lua
@@ -820,18 +820,25 @@ local function on_property_change(name, value)
end
end
-function _G.get(name, default)
- if not observed_properties[name] then
- local result, err = mp.get_property_native(name)
+function _G.get(name, default, observe_type)
+ observe_type = observe_type or "native"
+
+ if observed_properties[name] then
+ assert(observed_properties[name] == observe_type)
+ else
+ local result, err
+ if observe_type == "native" then
+ result, err = mp.get_property_native(name)
+ end
if err == "property not found" and not property_set(name:match("^([^/]+)")) then
mp.msg.error("Property '" .. name .. "' was not found.")
return default
end
- observed_properties[name] = true
+ observed_properties[name] = observe_type
property_cache[name] = result
- mp.observe_property(name, "native", on_property_change)
+ mp.observe_property(name, observe_type, on_property_change)
end
if current_item then
@@ -995,10 +1002,10 @@ local function clamp_submenu(submenu, max, cmd)
end
local function playlist()
- -- playlist is NOT observed. Observe playlist-count and playlist-pos and
- -- only retrieve playlist when playlist-count is not huge to not kill
- -- performance.
- get("playlist-pos")
+ -- Don't observe playlist and only retrieve it when playlist-count is not
+ -- huge to not kill performance. playlist is observed with type none to
+ -- receive every MP_EVENT_CHANGE_PLAYLIST.
+ get("playlist", nil, "none")
if get("playlist-count") > 99 then
return
endAnd it will just error if users try to use playlist in a condition?
There was a problem hiding this comment.
Don't know, how would it work otherwise with playlist? If this can be triggered external assert is bad.
There was a problem hiding this comment.
Don't know, how would it work otherwise with playlist? If this can be triggered external assert is bad.
There was a problem hiding this comment.
Don't know, how would it work otherwise with playlist? If this can be triggered external assert is bad.
There was a problem hiding this comment.
I don't follow. The other way it would work is the current PR commits which allow both observations. Also both ways add exactly 7 lines.
There was a problem hiding this comment.
So you want the user to be able to "kill performance", because they put the playlist in condition, instead of reusing none observer?
There was a problem hiding this comment.
It only kills performance with huge playlists. How could reusing the none observer work? Something like checked=playlist[1] would never be true because playlist is always nil. I don't think there is a reason to use playlist directly in conditions either way.
2 participants
Observing playlist-pos and playlist-count is not enough to update it after playlist-move, but we can observe any playlist property with type none to get notified of every MP_EVENT_CHANGE_PLAYLIST, even if the property has not changed.
This is similar to the mp.observe_property("vsync-jitter", "none", recorder) hack used by stats.lua to replicate the deprecated tick event.
Fixes #16816 (comment)