CogentRedTester/mpv-file-browser

OSC logo conflict

stax76 opened this issue · 24 comments

Proposed solution which is not perfect but good enough:

Provide 2 new conf options:

show-command
Command executed before show.
Users can set this to:
script-message osc-visibility never

hide-command
Command executed after hide.
Users can set this to:
script-message osc-visibility auto

More info:
mpv-player/mpv#10201

Screenshot (23)

How about I store the browser open status in the shared_script_properties property? Then you can activate things using conditional auto-profiles:

[file-browser]
profile-cond=shared_script_properties["file_browser-open"] == "yes"
profile-restore=copy-equal
script-opts-append=osc-visibility=never

This would just be easier than adding extra commands and options.

I just checked and shared_script_properties was added in mpv v0.31, which is the earliest version I support, so that would be a pretty neat solution.

I didn't really know about shared-script-properties and profiles is something I've never used. I'll take a look, thanks!

I think it would be ideal if things would work without every user having to search for a solution.

Maybe determine if a logo issue is possible by checking:

  • osc-visibility is always or auto
  • playlist-pos is -1
  • how many lines has the menu string and how many characters has the longest line
  • Font size

If a logo issue is possible, hide and restore the OSC.

Maybe it would be totally fine to always hide and restore the osc when the logo is visible (playlist-pos is -1), because the moment a user shows the menu, he is interested in the menu and not in the logo, simple and good solution.

It's true that I could automatically hide the OSC from within the script, and it might add a little extra functionality over using the auto-profiles. But, that would require me to implement and maintain support for the enabling/disabling logic (when to enable/disable, what to do if the user changes the settings, etc), which puts an extra burden on me for generally pretty insignificant improvements.

On the other hand, leaving it up to the user's auto-profiles gives them full control over what conditions are required (and I don't have to maintain anything). In addition, it allows for setting any arbitrary option using the profile rather than just being limited to the OSC visibility. What you suggested about hiding the OSC only when playlist-pos is -1 can be done as an additional condition as well.

So what I'm thinking is: save the file_browser-open field in shared_script_properties and add a section to the README saying it is there and giving the example profile I specified above.

What do you think?

This is the example I'd give, seems to work quite well:

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

Edit: changed playlist_pos == -1 to idle_active since this the property that the OSC uses to decide whether or not to display the logo

If it works, and you document it, then it might indeed be a great solution.

I have pushed support for the feature. Please test it out and let me know what you think.

It works, with adjustment in mpv.net too, thanks!

Out of curiosity, what adjustments were needed?

The mpv.net logo is added with overlay-add, I had to add:

            ObservePropertyString("script-opts", value => {
                if (value.ContainsEx("osc-visibility=never"))
                    HideLogo();
                else if (GetPropertyInt("playlist-pos") == -1)
                    ShowLogo();
            });

I found it astounding how your solution works, I didn't know about shared_script_properties and did not expect that changes to it can be observed, since it's a dictionary, and I also did not expect the osc reacts to changes of script-opts.

It could make sense, renaming the variable to menu-is-open, other menu scripts could use that too.

profile-restore=copy-equal

Are there any edge cases that not using profile-restore=copy ?

The issue is that script-opts is a single property that contains a list of options; the restore behaviour can only operate on the whole list, not on individual script-opts. If you use profile-restore=copy then any other script-opt changes you make (either intentionally or from other auto-profiles) will be undone. Using profile-restore=copy-equal will only restore the value if script-opts hasn't changed, which prevents this behaviour. The downside to this is that those script-opt changes might have nothing to do with osc-visibility, which means that you're not getting the OSC back despite wanting to.

Both options have positives and downsides, and it is up to the user to decide which they want to use. I chose to use copy-equal as the suggested default on the principle that a profile should try to interfere with other profiles as little as possible.

A 3rd solution to this issue is to use two profiles instead of one:

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

[unhide-logo]
profile-cond=shared_script_properties["osc-visibility"] == "never" and ( shared_script_properties["file_browser-open"] == "no" or not idle_active )
script-opts-append=osc-visibility=auto

This will successfully avoid any conflicts with other script-opt changes, but you need to hard-set whether or not it uses auto or always as the osc-visibility option.

This is what I meant when I said above that implementing the feature inside the script would be slightly more powerful, but in practice I doubt the vast majority of people are going to run into these problems.

Seems making a standalone script to control osc's visibility would be more safe. (If upstream show no interest in improving original osc script.)

Seems making a standalone script to control osc's visibility would be more safe. (If upstream show no interest in improving original osc script.)

This is actually more difficult to implement because it needs to adapt to other OSC scripts.

Seems making a standalone script to control osc's visibility would be more safe. (If upstream show no interest in improving original osc script.)

This is actually more difficult to implement because it needs to adapt to other OSC scripts.

If other menu scripts put their status in shared_script_properties you could detect when any are open. You'd be able to use an auto-profile for that as well. It would still require user tweaking for whatever scripts are installed though.

I have done it people, I have come up with a way to exploit lua syntax and the auto-profiles script to save a record of the original osc-visibility before changing it the first time. Behold its glory:

[hide-logo]
profile-cond=shared_script_properties["file_browser-open"] == "yes" and idle_active and (function() osc_visibility_original = osc_visibility_original or shared_script_properties["osc-visibility"] ; return true end )()
script-opts-append=osc-visibility=never

[unhide-logo-auto]
profile-cond=shared_script_properties["osc-visibility"] == "never" and osc_visibility_original == "auto" and ( shared_script_properties["file_browser-open"] == "no" or not idle_active ) and (function() osc_visibility_original = nil ; return true end )()
script-opts-append=osc-visibility=auto

[unhide-logo-always]
profile-cond=shared_script_properties["osc-visibility"] == "never" and osc_visibility_original == "always" and ( shared_script_properties["file_browser-open"] == "no" or not idle_active ) and (function() osc_visibility_original = nil ; return true end )()
script-opts-append=osc-visibility=always

Theoretically you could use this method to add any Lua logic inside the inline function and be able to write any script behaviour directly in mpv.conf.

Edit: it would just be all impossible to read since it is all in one line

Another option that I just became aware of is to use the osc option instead of script-opts-append

[hide-logo]
profile-cond=shared_script_properties["file_browser-open"] == "yes" and idle_active
profile-restore=copy
osc=no

This bypasses all of the gotchas that come from modifying script-opts and may be the cleanest solution, though there might be other side-effects that I'm not aware of.

Edit: I may replace the example in the README with this one unless someone knows about any problems with it.

Edit: I may replace the example in the README with this one unless someone knows about any problems with it.

It is suggested to add the following:
for other user scripts of osc ui.

[hide-logo]
profile-cond=shared_script_properties["file_browser-open"] == "yes" and idle_active
profile-restore=copy-equal
script-opts-append=scriptname*-visibility=never

What I do now is send:

script-message osc-idlescreen no

This is supported by osc.lua and also by the upcoming mpv.net release.

One problem I had is that osc.lua generates an unwanted OSD message, which I suppress with a script:

silent-invoke.lua:

function restore_osd_level()
    mp.set_property_number("osd-level", osd_level)
end

function silent_2_sec()
    osd_level = mp.get_property_number("osd-level")
    mp.set_property_number("osd-level", 0)
    mp.add_timeout(2, restore_osd_level)
end

function client_message(event)
    if event.args[1] == "silent-invoke" then
        silent_2_sec()
        table.remove(event.args, 1)
        mp.command_native(event.args)
    end
end

mp.register_event("client-message", client_message)

So my full input command is:

Ctrl+F script-message-to silent_invoke silent-invoke script-message osc-idlescreen no; script-binding file_browser/open-browser; show-text ' '

In my osm script, I also send the message with ensured silence:

function restore_osd_Level()
    mp.set_property_number("osd-level", osd_level)
end

function invoke_silent_command(cmd)
    osd_level = mp.get_property_number("osd-level")
    mp.set_property_number("osd-level", 0)
    mp.command(cmd)
    mp.add_timeout(2, restore_osd_Level)
end

invoke_silent_command("script-message osc-idlescreen no")

One problem I had is that osc.lua generates an unwanted OSD message, which I suppress with a script

You can use script-message osc-idlescreen no no_osd

That makes it much easier, thanks for pointing out!