Eisa01 / mpv-scripts

This repository contain scripts I have made for mpv media player...
BSD 2-Clause "Simplified" License
507 stars 35 forks source link

feat: add `shared-script-properties` and script-message to control the osc logo display on idle #49

Closed dyphire closed 1 year ago

dyphire commented 2 years ago

Code refer to https://github.com/CogentRedTester/mpv-file-browser/commit/b90c5a94edf3dd5497cab64e6178b0502e0cfb38 This allows users to set conditional auto-profiles depending on the open state of menu. Here is an example of an auto-profile that hides the OSC logo when menu is open in an idle window:

[hide-logo]
profile-cond=idle_active and shared_script_properties["menu-open"] == "yes"
profile-restore=copy
script-opts-append=osc-visibility=never

Close https://github.com/Eisa01/mpv-scripts/issues/47

dyphire commented 2 years ago

@Eisa01 Do you have any opinion on this PR?

Eisa01 commented 2 years ago

I'll be doing a small review (give me few days) and will provide my comments for merging. I think only slight changes are needed but I need to double check.

dyphire commented 1 year ago

Now, I try to add a new function to use script message to control the osc logo display in idle state. This will allow automatic control the logo display of osc scripts with have the osc-idlesereen feature. However, since this feature is a recently added function, let's keep utils.shared_script_property_set.

CogentRedTester commented 1 year ago

I'm not familiar with this codebase so take what I say with a grain of salt, but isn't it a little dangerous to have each script writing to the same exact property? If one were to open a menu before the previous one has closed then you may have a script setting the property to 'no' while another menu is still open. A similar issue could happen with the script message, but at least those are disabled by default.

dyphire commented 1 year ago

I'm not familiar with this codebase so take what I say with a grain of salt, but isn't it a little dangerous to have each script writing to the same exact property? If one were to open a menu before the previous one has closed then you may have a script setting the property to 'no' while another menu is still open. A similar issue could happen with the script message, but at least those are disabled by default.

Damn, this is an issue I didn't think of. Of course, this really needs to be handled.

At the same time, we also need to consider the osd overlay when another menu is opened. This will also need to share property through the script message.

CogentRedTester commented 1 year ago

At the same time, we also need to consider the osd overlay when another menu is opened. This will also need to share property through the script message.

I don't think that's a battle worth fighting, overlapping text from scripts are always going to happen and it's barely ever more than an inconvenience and is usually user triggered. The overlap is only visual, it's not like they're actually causing conflicting behaviour (beyond perhaps overwritten keybinds, but these should also go away when the overlay does). @Eisa01 perhaps you agree?

Trying to synchronise reads and writes to the property is likely not a great idea either. Scripts run in parallel, so you can't treat an 'if read then write' as an atomic operation. This is why I always use unique shared_script_properties keys for each script.

dyphire commented 1 year ago

This seems to be a reasonable view. I agree with you.

Eisa01 commented 1 year ago

@dyphire thanks for the additions, as @CogentRedTester pointed out, there should be a change in the name of menu-open. you can add the script name to it. e.g.: simplehistory-menu-open, simplebookmark-menu-open, etc..

Eisa01 commented 1 year ago

At the same time, we also need to consider the osd overlay when another menu is opened. This will also need to share property through the script message.

I don't think that's a battle worth fighting, overlapping text from scripts are always going to happen and it's barely ever more than an inconvenience and is usually user triggered. The overlap is only visual, it's not like they're actually causing conflicting behaviour (beyond perhaps overwritten keybinds, but these should also go away when the overlay does). @Eisa01 perhaps you agree?

Trying to synchronise reads and writes to the property is likely not a great idea either. Scripts run in parallel, so you can't treat an 'if read then write' as an atomic operation. This is why I always use unique shared_script_properties keys for each script.

@CogentRedTester I agree 100%, there is only so much you can handle. One way I am avoiding overlays of accidentally appearing while (atleast) these scripts are being used, is via an option to ignore certain keybinds. This makes it easy to not accidentally trigger any other (user defined) visuals while the menu is open. Sample:

#--Keybind thats are ignored when list is open
list_ignored_keybind=["h", "H", "r", "R", "c", "C"]
Eisa01 commented 1 year ago

@CogentRedTester On another note, I have been trying to make LogManager as a separate API. I really love the work you have done in file-browser.lua and I am using it as a reference. However, I have some questions here and there (especially since the programming logic is entirely different). I wish to communicate with you on while working on it, if you don't mind reaching out to me at eisa_01@outlook.com or any communication method of your preference.

dyphire commented 1 year ago

@dyphire thanks for the additions, as @CogentRedTester pointed out, there should be a change in the name of menu-open. you can add the script name to it. e.g.: simplehistory-menu-open, simplebookmark-menu-open, etc..

Done. Refer https://github.com/CogentRedTester/mpv-file-browser/commit/f1dd7f359da3d48d7b0234ec2a263efc2e2f8403# to add relevant warning information.

dyphire commented 1 year ago

Tested it and all is fine, I did not experience a problem. Could you change the message to this before merge (for both .conf and .lua).

hides OSC idle screen message when opening and closing menu (could cause unexpected behavior if multiple scripts are triggering osc-idlescreen off)

Done.