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

feat: added disable_elements option and disable-elements script message #695

Merged
merged 7 commits into from
Oct 14, 2023

Conversation

tomasklaen
Copy link
Owner

Allows disabling elements and various indicators by adding their IDs to the list:

disable_elements=timeline,audio_indicator

Also includes a new script message listener disable-elements, that allows scripts to do the same thing:

local id = mp.get_script_name()
mp.commandv('script-message-to', 'uosc', 'disable-elements', id, 'timeline,audio_indicator')

It'll register what elements each script wants disabled. The element will be enabled only when it was not disabled by neither user nor any script.

To cancel or re-enable the elements, pass an empty list:

mp.commandv('script-message-to', 'uosc', 'disable-elements', id, '')

ref #686, closes #592


This was way more work then I expected :(

I hope I didn't introduce many bugs.

Also, is it really not possible to get the name of a script that sent the message? We wouldn't need the script_id parameter on the message then.

…essage

Allows disabling elements and various indicators by adding their IDs to the list:

```conf
disable_elements=timeline,audio_indicator
```

Also includes a new script message listener `disable-elements`, that does the same thing:

```lua
local id = mp.get_script_name()
mp.commandv('script-message-to', 'uosc', 'disable-elements', id, 'timeline,audio_indicator')
```

It'll register what elements each script wants disabled. The element will be enabled only when it is not disabled by neither user nor any script.

To cancel or re-enable the elements, just pass an empty list:

```lua
mp.commandv('script-message-to', 'uosc', 'disable-elements', id, '')
```

ref #686, closes #592
@christoph-heinrich
Copy link
Contributor

Is it really necessary do destroy all disabled elements? I would have expected it to have some additional management around element.enabled and that's it.

Sets in lua can be made with set[value] = true, I don't think checking the final table if a value is already in there is faster or simpler (maybe for small lists? I didn't benchmark it).

@tomasklaen
Copy link
Owner Author

tomasklaen commented Oct 12, 2023

Is it really necessary do destroy all disabled elements? I would have expected it to have some additional management around element.enabled and that's it.

element.enabled is intended for temporary toggling during use (like timeline hiding when obstructed, or no duration/time available). disable_elements is for "I never want to see this element again". In practice element.enabled won't cleanup everything, like registered mp event listeners, or property observers, but destroying an element will now.

Sets in lua can be made with set[value] = true, I don't think checking the final table if a value is already in there is faster or simpler (maybe for small lists? I didn't benchmark it).

That Manager.disabled table is to be read by outside stuff that is not a full element, like audio indicator (and idle indicator I'll add when this is merged), which is just a condition in render().

But I'll simplify it a bit with a set.

@christoph-heinrich
Copy link
Contributor

There is no need for itable_join_unique()...

@christoph-heinrich
Copy link
Contributor

christoph-heinrich commented Oct 12, 2023

image
image

diff --git a/scripts/uosc/lib/ass.lua b/scripts/uosc/lib/ass.lua
index 16ffb09..a9e742f 100644
--- a/scripts/uosc/lib/ass.lua
+++ b/scripts/uosc/lib/ass.lua
@@ -91,7 +91,7 @@ function ass_mt:tooltip(element, value, opts)
 	local x = element.ax + (element.bx - element.ax) / 2
 	local y = align_top and element.ay - offset or element.by + offset
 	local width_half = (opts.width_overwrite or text_width(value, opts)) / 2 + padding_x
-	local min_edge_distance = width_half + opts.margin + Elements.window_border.size
+	local min_edge_distance = width_half + opts.margin + Elements:ev('window_border', 'size', 0)
 	x = clamp(min_edge_distance, x, display.width - min_edge_distance)
 	local ax, bx = round(x - width_half), round(x + width_half)
 	local ay = (align_top and y - opts.size * opts.lines - 2 * padding_y or y)
diff --git a/scripts/uosc/main.lua b/scripts/uosc/main.lua
index 2732717..a70b714 100644
--- a/scripts/uosc/main.lua
+++ b/scripts/uosc/main.lua
@@ -1297,7 +1297,7 @@ Manager = {
 ---@param element_ids string|string[]|nil `foo,bar` or `{'foo', 'bar'}`.
 function Manager:disable(manager_id, element_ids)
 	self._disabled_by[manager_id] = comma_split(element_ids)
-	self.disabled = make_set(itable_join(table.unpack(table_values(self._disabled_by))))
+	self.disabled = make_set(itable_join(unpack(table_values(self._disabled_by))))
 	self:_commit()
 end

@christoph-heinrich
Copy link
Contributor

christoph-heinrich commented Oct 12, 2023

I'll have a closer look at the code tomorrow.

Copy link
Contributor

@christoph-heinrich christoph-heinrich left a comment

Choose a reason for hiding this comment

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

Instead of having a hard coded number for render_order in each element, we could have a static list with element ids and then use the index as the render_order. That would make keeping track of what gets rendered in which order easier.

scripts/uosc/elements/Volume.lua Outdated Show resolved Hide resolved
scripts/uosc/lib/std.lua Outdated Show resolved Hide resolved
scripts/uosc/elements/PauseIndicator.lua Show resolved Hide resolved
scripts/uosc/elements/Element.lua Show resolved Hide resolved
@tomasklaen
Copy link
Owner Author

Instead of having a hard coded number for render_order in each element, we could have a static list with element ids and then use the index as the render_order. That would make keeping track of what gets rendered in which order easier.

I don't like the idea of it being disjointed from the element's implementation on a new state config. It would make sense if elements were actually completely self contained entities oblivious about the outside world, but since they know about each other and use each other's states, I think it's fine for them to configure themselves like that in their :init()s, just as they are configuring their own IDs in the Elements collection. At least in the current code base. And it's simple :)

Damn I wish I had time to rewrite it all. I don't like how the current element system and rendering works at all.

@christoph-heinrich
Copy link
Contributor

christoph-heinrich commented Oct 13, 2023

It really wouldn't be that complicated. e.g.

render_order_ids = {
	'window_border',
	'buffer_indicator',
	...
}

function Element:init(id, props)
	self.id = id
	self.render_order = itable_index_of(render_order_ids, id)
	...

But if you'd rather have {render_order = x} in each element, then that's fine too I guess.

@po5
Copy link
Contributor

po5 commented Oct 14, 2023

Also, is it really not possible to get the name of a script that sent the message? We wouldn't need the script_id parameter on the message then.

I looked into how events are propagated internally a while ago and I don't believe there is a way to get that info.

@tomasklaen
Copy link
Owner Author

But if you'd rather have {render_order = x} in each element, then that's fine too I guess.

We need to have the init passing of render_order anyway, since bunch of elements need to inherit their parent order (control buttons/speed, volume slider).

@tomasklaen tomasklaen merged commit 3af5ccf into main Oct 14, 2023
@christoph-heinrich
Copy link
Contributor

We need to have the init passing of render_order anyway, since bunch of elements need to inherit their parent order (control buttons/speed, volume slider).

That's only necessary in those few instances and those two things are not mutually exclusive.

@tomasklaen tomasklaen deleted the disable_elements branch October 27, 2023 07:47
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.

Using uosc with other UIs
3 participants