Skip to content

osc.lua: add observe_cached() helper#17563

Open
guidocella wants to merge 1 commit intompv-player:masterfrom
guidocella:osc-cache
Open

osc.lua: add observe_cached() helper#17563
guidocella wants to merge 1 commit intompv-player:masterfrom
guidocella:osc-cache

Conversation

@guidocella
Copy link
Copy Markdown
Contributor

This removes code repetition and will make caching more properties easier.

Some state fields are renamed to have the same name as the property.

Comment on lines +316 to +321
local function observe_cached(property, callback)
mp.observe_property(property, "native", function (_, value)
state[property:gsub("-", "_")] = value
callback()
end)
end
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
local function observe_cached(property, callback)
mp.observe_property(property, "native", function (_, value)
state[property:gsub("-", "_")] = value
callback()
end)
end
local function observe_cached(property, callback)
local key = property:gsub("-", "_")
mp.observe_property(property, "native", function (_, value)
state[key] = value
callback()
end)
end

This is probably trivial to suggest, but wouldn't it better to use gsub outside the observer? To avoid excess calls.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure we can micro-optimize it.

This removes code repetition and will make caching more properties
easier.

Some state fields are renamed to have the same name as the property.
@Samillion
Copy link
Copy Markdown
Contributor

I've been running this for a while now, works really well.

Are you planning to do the same for:

  • playlist-count and playlist-pos
  • maybe: volume and mute?

@guidocella
Copy link
Copy Markdown
Contributor Author

Yeah I'll cache everything that is useful after this is merged.

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