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

MacOSHandle shouldn't have ns_view property #32

Closed
thomcc opened this issue Oct 5, 2019 · 5 comments
Closed

MacOSHandle shouldn't have ns_view property #32

thomcc opened this issue Oct 5, 2019 · 5 comments

Comments

@thomcc
Copy link

thomcc commented Oct 5, 2019

It's redundant. Anybody who wants this can just get it via

let ns_window = handle.ns_window as *mut Object;
let ns_view: *mut Object = msg_send![ns_window, contentView];

or similar. Same goes for ns_window_controller (via the windowController message) and ns_view_controller (via the contentViewController message), which don't currently exist, but are mentioned in comments as potential future extensions.

@Osspial
Copy link
Contributor

Osspial commented Oct 5, 2019

So, there's been a bit of back-and-forth recently about whether or not to expose HINSTANCE in RawWindowHandle::Windows. An HINSTANCE is basically a handle to the application the window was created with. I was initially opposed to doing that, since similarly to ns_view it can be derived from the hwnd, but I've recently changed my mind about it. Exposing it isn't too much of a stretch, and it seems to give downstream users peace of mind when they don't have to reach into the platform-specific documentation to get a field they need to initialize a vulkan surface or whatever it is they need to do. After all, understanding the platform-specific APIs is largely the job of the people abstracting around the windowing APIs. I feel like ns_view is in a similar sort of territory.

If somebody submits a PR adding ns_view_controller and ns_window_controller, I'll probably accept them. If not, stuff will remain as-is. I don't think there's any harm in exposing a little bit more than necessary, and I don't really feel like engaging in an argument about it, since it's mostly a matter of "what design principles do you think matter most".

@thomcc
Copy link
Author

thomcc commented Oct 6, 2019

I don't know if I really agree, as this leaves more room for bugs caused by implementers of the RawWindowHandle trait, but it's not really my call to make.

If you're keeping it, I strongly feel that the documentation should note which view it is precisely. as it is, it's just some view associated with the window. The contentView is probably the most obvious choice, but isn't the only one. (Additionally, if a user happens to hear they need to pass the window's contentView into something, they probably will have to look up and verify that the view they have is the contentView -- I would do this, at least).

It's also possibly worth noting that NSWindow are safe to use from whichever thread you want, but the NSView is a main-thread-only type (e.g. NSWindow is send, NSView isn't and also has a special relationship with the main thread). I don't know if this matters here, though -- probably not as MacOSHandle isn't send (admittedly, it not being send is not enough for it to be sound to use the NSView, but only unsafe code can do anything meaningful with it anyway, so that's possibly fine).

@Osspial
Copy link
Contributor

Osspial commented Oct 6, 2019

If you're keeping it, I strongly feel that the documentation should note which view it is precisely. as it is, it's just some view associated with the window. The contentView is probably the most obvious choice, but isn't the only one.

What sorts of bugs could happen as a result of not doing that? If users can associate multiple NSViews with a particular window, it seems useful to allow implementors to specify the exact view they're referring to. I'm not inclined to take that flexibility away without good reason.

@wrl
Copy link

wrl commented Nov 12, 2020

Audio plugins on macOS receive (or are expected to return) an NSView * which is independent of any particular window and which will not necessarily (in fact, rarely) end up as the contentView.

Removing the NSView * would break our use case irreparably.

@thomcc
Copy link
Author

thomcc commented Jan 3, 2021

Removing the NSView * would break our use case irreparably.

Sold.

@thomcc thomcc closed this as completed Jan 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants