client: add MPV_EVENT_INITIALIZED #15089
Conversation
|
Download the artifacts for this pull request: |
|
Shouldn't this follow the suit of |
What's the value in making it a property? To me it "feels" more like an event than property, because it's one point in time in the lifetime of [lib]mpv (similar to shutdown, but sent only once), but on its own that shouldn't be considered a substantial argument. |
This may be useful for waiting until all scripts have finished loading. Use with caution, as scripts shouldn't perform too many tasks during initialization or afterward. Instead, they should wait to be triggered by property or event changes.
It depends how such event would be used. I wanted to keep it internal, because I don't see clear use-cases. Having this as property would allow script to check "initialized" state if it is loaded after actually initialization has taken place. On the other hand, unlike idle, we would never change initialized property during runtime. It is specifically one time trigger that happens on initial load. Though like I said, I have no idea, what to do if script is loaded later and missed the init event. |
If a script is loaded later then this is irrelevant for them. It's only relevant for clients/scripts which load/exist at startup - during the initialization phase, and which want to know when init is complete. |
|
what's the use case here? |
Technically it's required for #15058 . Making it public is to allow 3rd party scripts the same capabities which internal scripts have, because they can find it equally useful in various use cases related to sync between scripts which depends on knowing that all other scripts are loaded. |
Well, "required" - if we want to fix an existing bug where It currently waits using a timeout of 0, but this method doesn't actually guarantee that all scripts have finished init, while this new Arguably this reason on its own might not be strong enough to add an mpv event, however, the functionality can be very useful to scripts which need init-sync with other scripts - in a similar way to how |
It's only "required" because for
init-sync can be done by letting scripts writing their init status to |
|
Even if |
I don't think input-bindings notifications would allow for better bindlist behavior, because you don't know how long to wait, like in your example - where there's late initialization. bindlist needs to cut it off at some stage. The original design was to wait till all scripts are loaded, so that the common case where script add bindings during init is covered. Cases of bindings at a later stages can never be covered compoletely however you look at it. However, the original design can be changed if someone has a better idea at which point in time to grab the input-bindings property in order to list them and exit - while still covering the common case of bindings which are added at the init phase.
I don't think this covers the init-sync use case, for the same reason that observing input-bindings would not cover it - you don't know how long to wait and decide that "that's it, all scripts which should have changed user-data already changed it". However, while I still think that this event can be very useful to scripts, and this event or something equivalent would be very nice to have, I don't have very strong opinion about adding this event. My only take is that if we're adding this event, then there's a very good reason to also make it public, so that 3rd party clients/scripts can use it as well. |
Exactly.
Agreed. Having I will let you guys decide, I will adjust this PR accordingly to your guidance. I stated my position before. |
|
@na-na-hi do you have a viable alternative to adding this event? Do I understand correctly that you suggest to add observer notification to input-bindings, and use that somehow instead of this event? care to explain how? If we don't want to add this event (I have no opinion about that, but I agree that fixing the 0-timeout for bindlist might not be a strong enough reason to add an mpv event), then my suggestion is to simply increase the timeout, for instance to 250ms, or maybe use some other trigger (instead of timeout) which would reasonably cover the common case of binding added by scripts/clients on init. It only has to be good enough, we don't need perfect coverage here IMO (which we can't get anyway - for instance with late init in scripts). If we can't come up with an acceptable alternative to adding this event (bindings notifications and a good procedure to use them, bigger timeoue, whatever), then I think we should go ahead with adding this event. My single take is that if we do add this event because we consider it useful to stats.lua, then we should make it public, because it would be snobbish of us to think that only we need it but 3rd party clients/scripts should not have use cases for it. |
|
I don't have a strong opinion, just wanted to know what this is for. |
Make the first change notification of |
First of all, that's one step. When should the bindlist code stop listening for input-bindings notifications? after the first notification? To me, that's just a roundabout method to do the same function as the Why do you consider it better than adding the event?
I don't think so, but the notification arrives after the init of all scripts is complete - not during init. I.e. after the "main" code of each script was executed, and the event loop is entered, and the notification arrives as an event to the script's event loop. It signals that all scripts are loaded, had a chance to register any listeners during their init, etc, and guarantees that any script-message sent after that event would be recieved by any other script which mpv loads on startup and registered the appropriate handler during init.
I don't think so, but do correct me if I'm wrong. This event is designed exactly to make the behavior consistent regardless of scripts load order by mpv. It doesn't matter whether script A or B were loaded first, because once that event is recieved, A knows that if B exists then it's already loaded, and B knows that if A exists then it's already loaded.
Not sure what this means or implies. Care to elaborate? |
Yes, because the first notification happens after all scripts are initialized.
I don't think it's more limited. It achieves the same result, plus the ability to receive the change notification if the bindings change afterwards.
It would achieve the same result for the script order issue, but without a new event and explicit handling by the scripts. Overall my point is that this event has limited use cases which can be better served by changes elsewhere which don't introduce a new event which the scripts need to handle. |
|
I wrote a lot of things but deleted them. Let me put it simply: the event method is bullet-proof for the thing it guarantees (fire after all scripts are initialized), can have general init-sync value, covers our specific use case perfectly (bindlist) by ensuring we run after all scripts which add bindings at init have already added them. And it's literally one line of clean code. What arguments you have against merging it? Do you think it's not worth adding an mpv event only to fix the bindlist scenario? I won't disagree with that. If that's the reason, how about the 250ms method I suggested above? or maybe some other trigger you can think of which we can use today without additional infrastructure? |
|
Well, I'm out of the discussion about which solution is best for the bindlist 0-timeout issue. I didn't advocate to use this event, and I don't care which solution is used as long as it's reasonable (which I do consider both the event solution or a 250ms solution or something else which would cover our use case), or even not solve it at all if there's ongoing disagreement. @kasper93 if you still want to use this event, please decide what to do with @na-na-hi arguments. |
No description provided.