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

OSC logo conflict #55

Closed
stax76 opened this issue May 18, 2022 · 24 comments
Closed

OSC logo conflict #55

stax76 opened this issue May 18, 2022 · 24 comments
Labels
documentation Improvements or additions to documentation enhancement New feature or request good first issue Good for newcomers

Comments

@stax76
Copy link

stax76 commented May 18, 2022

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)

@stax76 stax76 changed the title OSC conflict OSC logo conflict May 18, 2022
@CogentRedTester
Copy link
Owner

CogentRedTester commented May 18, 2022

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.

@CogentRedTester
Copy link
Owner

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.

@stax76
Copy link
Author

stax76 commented May 18, 2022

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

@stax76
Copy link
Author

stax76 commented May 18, 2022

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.

@stax76
Copy link
Author

stax76 commented May 18, 2022

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.

@CogentRedTester
Copy link
Owner

CogentRedTester commented May 18, 2022

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?

@CogentRedTester
Copy link
Owner

CogentRedTester commented May 18, 2022

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

@stax76
Copy link
Author

stax76 commented May 18, 2022

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

CogentRedTester added a commit that referenced this issue May 18, 2022
This allows users to set conditional auto-profiles depending on the open
state of the browser

Also added a section of the README that explains this and shows an
example.
@CogentRedTester
Copy link
Owner

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

@stax76
Copy link
Author

stax76 commented May 18, 2022

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

@CogentRedTester
Copy link
Owner

Out of curiosity, what adjustments were needed?

@stax76
Copy link
Author

stax76 commented May 18, 2022

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.

@stax76
Copy link
Author

stax76 commented May 18, 2022

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

@hooke007
Copy link

profile-restore=copy-equal

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

@CogentRedTester
Copy link
Owner

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.

@hooke007
Copy link

hooke007 commented May 19, 2022

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

@dyphire
Copy link
Contributor

dyphire commented May 19, 2022

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.

@CogentRedTester
Copy link
Owner

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.

@CogentRedTester
Copy link
Owner

CogentRedTester commented May 19, 2022

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

Dudemanguy added a commit to Dudemanguy/mpv that referenced this issue May 19, 2022
This is mainly for other user scripts that may conflict with the osc
logo in some way. While you can use script-message and
shared-script-properties (highly discouraged in the documentation for
some reason) to work around this, there's still some brittle edge cases
and complicated logic that can pop up. Adding an option for these two
things (the logo and the "drop files..." text) is really trivial and
would make things easier for scripts. Discussed in a couple of issues
below:
mpv-player#10201
CogentRedTester/mpv-file-browser#55
Dudemanguy added a commit to Dudemanguy/mpv that referenced this issue May 19, 2022
This is mainly for other user scripts that may conflict with the osc
logo in some way. While you can use script-message and
shared-script-properties (highly discouraged in the documentation for
some reason) to work around this, there's still some brittle edge cases
and complicated logic that can pop up. Adding an option for these two
things (the logo and the "drop files..." text) is really trivial and
would make things easier for scripts. Discussed in a couple of issues
below:
mpv-player#10201
CogentRedTester/mpv-file-browser#55
@CogentRedTester
Copy link
Owner

CogentRedTester commented May 19, 2022

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.

@dyphire
Copy link
Contributor

dyphire commented May 19, 2022

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

@CogentRedTester CogentRedTester added documentation Improvements or additions to documentation good first issue Good for newcomers labels May 21, 2022
Dudemanguy added a commit to Dudemanguy/mpv that referenced this issue May 24, 2022
This is mainly for other user scripts that may conflict with the osc
logo in some way. Although it is possible to listen for
shared-script-properties, this has many edge cases that could easily pop
up. A user could want other OSC things to happen at the same time (say
osc-message). They just don't want the logo. The idlelogo option
disables all logo related things (including the santa hat) if it is set
to "no". A new script message (osc-idlelogo) is also added so users and
scripts can easily toggle the value (passing "cycle" or just explictly
setting "yes" or "no"). Some more discussion on this is found on the
below github issues.
mpv-player#10201
CogentRedTester/mpv-file-browser#55
Dudemanguy added a commit to Dudemanguy/mpv that referenced this issue May 25, 2022
This is mainly for other user scripts that may conflict with the osc
logo in some way. Although it is possible to listen for
shared-script-properties, this has many edge cases that could easily pop
up. A user could want other OSC things to happen at the same time (say
osc-message). They just don't want the logo. The idlelogo option
disables all logo related things (including the santa hat) if it is set
to "no". A new script message (osc-idlelogo) is also added so users and
scripts can easily toggle the value (passing "cycle" or just explictly
setting "yes" or "no"). Some more discussion on this is found on the
below github issues.
mpv-player#10201
CogentRedTester/mpv-file-browser#55
Dudemanguy added a commit to Dudemanguy/mpv that referenced this issue May 29, 2022
This is mainly for other user scripts that may conflict with the osc
logo in some way. Although it is possible to listen for
shared-script-properties, this has many edge cases that could easily pop
up. A user could want other OSC things to happen at the same time (say
osc-message). They just don't want the logo. The idlelogo option
disables all logo related things (including the santa hat) if it is set
to "no". A new script message (osc-idlelogo) is also added so users and
scripts can easily toggle the value (passing "cycle" or just explictly
setting "yes" or "no"). Some more discussion on this is found on the
below github issues.
mpv-player#10201
CogentRedTester/mpv-file-browser#55
Dudemanguy added a commit to Dudemanguy/mpv that referenced this issue May 30, 2022
This is mainly for other user scripts that may conflict with the osc
logo in some way. Although it is possible to listen for
shared-script-properties, this has many edge cases that could easily pop
up. A user could want other OSC things to happen at the same time (say
osc-message). They just don't want the logo. The idlelogo option
disables all logo related things (including the santa hat) if it is set
to "no". A new script message (osc-idlelogo) is also added so users and
scripts can easily toggle the value (passing "cycle" or just explictly
setting "yes" or "no"). Some more discussion on this is found on the
below github issues.
mpv-player#10201
CogentRedTester/mpv-file-browser#55
Dudemanguy added a commit to Dudemanguy/mpv that referenced this issue Jun 1, 2022
This is mainly for other user scripts that may conflict with the osc
logo in some way. Although it is possible to listen for
shared-script-properties, this has many edge cases that could easily pop
up. A user could want other OSC things to happen at the same time (say
osc-message). They just don't want the logo. The idlelogo option
disables all logo related things (including the santa hat) if it is set
to "no". A new script message (osc-idlelogo) is also added so users and
scripts can easily toggle the value (passing "cycle" or just explictly
setting "yes" or "no"). Some more discussion on this is found on the
below github issues.
mpv-player#10201
CogentRedTester/mpv-file-browser#55
Dudemanguy added a commit to Dudemanguy/mpv that referenced this issue Jun 3, 2022
This is mainly for other user scripts that may conflict with the osc
logo in some way. Although it is possible to listen for
shared-script-properties, this has many edge cases that could easily pop
up. A user could want other OSC things to happen at the same time (say
osc-message). They just don't want the logo. The idlescreen option
disables all idlescreen related things (including the santa hat) if it
is set to "no". A new script message (osc-idlescreen) is also added so
users can easily toggle the value (passing "cycle" or just explictly
setting "yes" or "no"). Some more discussion on this is found in the
below github issues.
mpv-player#10201
CogentRedTester/mpv-file-browser#55
Dudemanguy added a commit to Dudemanguy/mpv that referenced this issue Jun 4, 2022
This is mainly for other user scripts that may conflict with the osc
logo in some way. Although it is possible to listen for
shared-script-properties, this has many edge cases that could easily pop
up. A user could want other OSC things to happen at the same time (say
osc-message). They just don't want the logo. The idlescreen option
disables all idlescreen related things (including the santa hat) if it
is set to "no". A new script message (osc-idlescreen) is also added so
users can easily toggle the value (passing "cycle" or just explictly
setting "yes" or "no"). Some more discussion on this is found in the
below github issues.
mpv-player#10201
CogentRedTester/mpv-file-browser#55
Dudemanguy added a commit to mpv-player/mpv that referenced this issue Jun 4, 2022
This is mainly for other user scripts that may conflict with the osc
logo in some way. Although it is possible to listen for
shared-script-properties, this has many edge cases that could easily pop
up. A user could want other OSC things to happen at the same time (say
osc-message). They just don't want the logo. The idlescreen option
disables all idlescreen related things (including the santa hat) if it
is set to "no". A new script message (osc-idlescreen) is also added so
users can easily toggle the value (passing "cycle" or just explictly
setting "yes" or "no"). Some more discussion on this is found in the
below github issues.
#10201
CogentRedTester/mpv-file-browser#55
@stax76
Copy link
Author

stax76 commented Jun 5, 2022

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")

@dyphire
Copy link
Contributor

dyphire commented Jun 5, 2022

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

@stax76
Copy link
Author

stax76 commented Jun 5, 2022

That makes it much easier, thanks for pointing out!

po5 pushed a commit to po5/thumbfast that referenced this issue Sep 18, 2022
This is mainly for other user scripts that may conflict with the osc
logo in some way. Although it is possible to listen for
shared-script-properties, this has many edge cases that could easily pop
up. A user could want other OSC things to happen at the same time (say
osc-message). They just don't want the logo. The idlescreen option
disables all idlescreen related things (including the santa hat) if it
is set to "no". A new script message (osc-idlescreen) is also added so
users can easily toggle the value (passing "cycle" or just explictly
setting "yes" or "no"). Some more discussion on this is found in the
below github issues.
mpv-player/mpv#10201
CogentRedTester/mpv-file-browser#55
dyphire pushed a commit to dyphire/mpv that referenced this issue Feb 22, 2023
This is mainly for other user scripts that may conflict with the osc
logo in some way. Although it is possible to listen for
shared-script-properties, this has many edge cases that could easily pop
up. A user could want other OSC things to happen at the same time (say
osc-message). They just don't want the logo. The idlescreen option
disables all idlescreen related things (including the santa hat) if it
is set to "no". A new script message (osc-idlescreen) is also added so
users can easily toggle the value (passing "cycle" or just explictly
setting "yes" or "no"). Some more discussion on this is found in the
below github issues.
mpv-player#10201
CogentRedTester/mpv-file-browser#55
dyphire pushed a commit to dyphire/mpv that referenced this issue Feb 22, 2023
This is mainly for other user scripts that may conflict with the osc
logo in some way. Although it is possible to listen for
shared-script-properties, this has many edge cases that could easily pop
up. A user could want other OSC things to happen at the same time (say
osc-message). They just don't want the logo. The idlescreen option
disables all idlescreen related things (including the santa hat) if it
is set to "no". A new script message (osc-idlescreen) is also added so
users can easily toggle the value (passing "cycle" or just explictly
setting "yes" or "no"). Some more discussion on this is found in the
below github issues.
mpv-player#10201
CogentRedTester/mpv-file-browser#55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants