-
Notifications
You must be signed in to change notification settings - Fork 3.2k
cocoa-cb: re-enable wid (NSView) embedding on macOS for libmpv use
#17221
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
base: master
Are you sure you want to change the base?
Conversation
e332032 to
0b00ae9
Compare
wid (NSView) embedding on macOS for libmpv use
In version 0.37, support for the old cocoa backend has been dropped, which unfortunately also removed the ability of embedding mpv in native applications by specifying a wid - which is still working for all other platforms. This PR aims to remedy that situation by leveraging the existing cocoa-cb implementation which already had just about everything that it takes and it has been merely about gluing loose ends together and omitting those parts that are specific to standalone application use, like NSApplication and NSWindow.
0b00ae9 to
b4aaef0
Compare
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
|
When there's once another AI bot coming to respond, then I'll close this PR. |
Why? Didn't you request all of these to spam this PR? |
|
diffray-bot is spam advertising campaign (look at all these utm_source links) "review"ing code without permission aidenybai/react-grab#102 (comment) Just block it. |
Done. |
I didn't request anything and I never heard of diffray before. |
|
i am very hesitant to add this feature to the old cocoa-cb backend:
what benefit does this give us over a similar macvk feature for example? i am inclined to just closing this PR sorry. |
In which ways do you think this PR would be invasive? When you take a closer look you'll see that every change involves a clear separation of cases. The existing code paths for the working case are entirely untouched.
And still, there were 98 commits to the
Thanks a lot for looking at this PR |
Not all osdep/mac changes are related to the cocoa backend. |
Right, thanks. Then it's 18. Doesn't change the argument, though. ;-) |
they aren't really 'untouched' they are just wrapped in ifs now. this adds a whole new level of complexity to the whole backend. anyway i think this is more appropriate for a proper code review, in the case i decide to merge it.
i am not sure what kind of argument you are trying to make? first of all the amount of commits is irrelevant (might not even catch all commits directly/solely related to cocoa-cb though), but the content is. of those 18 commits (since 2021) or 16 (since 2024, not even sure why 2024 when the past year is 2025) the majority are bug fixes, feature replacements, things that were a necessity because other parts of the code changed or are plainly unrelated to cocoa-cb because only the commit message mentioned it. only one commit is an actual new feature (8a61929 #14017). something i did in 2020/2021 and only was added in 2024. even mentioned on the PR that this is the last new feature.
especially because it uses libmpv (rendering API) internally it shouldn't be used from an external libmpv instance/handler imo.
there is no way in cocoa-cb either atm, hence my question. if we speak PRs, we have/had a similar one for macvk support too.
not sure what point you try to make. the feature is not available atm, so there isn't even anything to test regarding cross platform support of wid embedding.
not sure where you got that policy from. we support what we feel like supporting and maintaining. nothing is dropped here it's quasi deprecated, like mentioned. those are different things, eg i am still fixing bugs. Apple dropped the support in the first place, i don't feel like supporting something Apple doesn't support, like already mentioned. |
|
Thanks a lot for your reply.
What I meant is that the "code paths" (not the code itself) are untouched in terms of the existing functionality - in other words: I made the changes very carefully to avoid any kinds of regression.
I simply meant to say that here has still been quite a bit of work done recently. I looked at the Git log of the
I see it somewhat differently: the code is perfectly suitable for the purpose - it already does almost everything that is needed for use via libmpv. All that was needed is to connect the loose ends and add some conditional treatment here and there. I know very well how uncomfortable it can sometimes feel when an alien person comes around and starts drilling holes into your code. I have some hope that it might get better when you review it (in case).
There has been one until not too long ago (via cocoa) - the sample is still sitting in the samples repo, confusing people for why it doesn't work - WID embedding works for all other platforms - that's what I meant by "closing the gap".
I haven't seen that one. Is it good? IMO, both ways should work with WID. I had planned to look into that as well.
I meant in case it would have been merged.
Yea, sorry, "policy" was a stupid term in this context, I should have found a better wording, but I'm glad you got what I meant to express :-) Thanks again for your time |
|
Thinking about it - how would you feel about having a descendant class of CocoaCB instead, which implements the different impls in overrides? The |
In version 0.37, support for the old cocoa backend had been dropped, which unfortunately also removed the ability of embedding mpv in native applications by specifying a wid - which is still working for all other platforms.
This PR aims to remedy that situation by leveraging the existing cocoa-cb implementation which already has just about everything that it takes and it has been merely about gluing loose ends together and omitting those parts that are specific to standalone application use, like NSApplication and NSWindow.