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

Add a knob to specify a class for window creation. #90

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

vseguip
Copy link

@vseguip vseguip commented Jun 29, 2023

This is useful to allow overriding some behaviours that are not sent to WindowsDelegate (for example canBecomeKey, etc). Will default to NSWindow so it will be a noop for existing code.

@ryanmcgrath
Copy link
Owner

Hmmm, any reason to not just implement support for e.g canBecomeKey? Seems like it would be more apparent in docs/etc if we went that route.

(Is there a list of things cacao's missing here?)

@vseguip
Copy link
Author

vseguip commented Jun 29, 2023

Two main reasons:

  1. We need to allow the user to override canBecomeKey (et. al) but the method is an NSWindow method so I'm not sure how we could allow that in Rust cacao. Potentially the Cacao NSWindow could delegate on to the controller we pass in but this would be a significant change from the Cocoa API where the method is not delegated to the controller (and I'm not even sure what default implementation we could provide in the controller trait, perhaps calling back to NSWindow?).
  2. The current implementation provides more flexibility (ie users can override any method in NSWindow, in my case I'm also overriding canBecomeMainWindow so I can implement a focusable frameless window similar to Spotlight) without increasing the surface API for Cacao itself which makes the latter easier to maintain IMO.

@ryanmcgrath
Copy link
Owner

but this would be a significant change from the Cocoa API where the method is not delegated to the controller

This is something I'm fine with, personally. cacao is meant to be "a Cocoa-ish API, but in Rust" - I'm not sure most people care enough about the distinction between the window/delegate/etc.

and I'm not even sure what default implementation we could provide in the controller trait, perhaps calling back to NSWindow?)

This is a very good question and I've unfortunately been up for 24 hours now, so will need to mull it over after some sleep.

The current implementation provides more flexibility (ie users can override any method in NSWindow, in my case I'm also overriding canBecomeMainWindow so I can implement a focusable frameless window similar to Spotlight) without increasing the surface API for Cacao itself which makes the latter easier to maintain IMO.

I'm less interested in the flexibility and more interested in "can users who read the Rust/cacao docs immediately see what possibilities are there" without needing to go and learn all of Cocoa itself. If people have to drop to doing their own subclasses for specific bits of Cocoa functionality, then they may as well use objc directly or something.

That all said, I'm not necessarily against this PR - that is to say, merging it since it does give an interesting lower level hook strategy, but also simultaneously looking at improving the Cocoa hooks that cacao offers. I'll sleep on it and see what I land on - either way the effort's appreciated!

@madsmtm
Copy link
Contributor

madsmtm commented Jun 29, 2023

For reference, in winit we need to override canBecomeMainWindow and canBecomeKeyWindow, so take that as a data point that both of those are potentially useful.

I think the better solution for cacao is to delegate to the WindowDelegate for things like this, as has been said - that makes the API much easier to use for users.

@vseguip
Copy link
Author

vseguip commented Jul 3, 2023

This is something I'm fine with, personally. cacao is meant to be "a Cocoa-ish API, but in Rust" - I'm not sure most people care enough about the distinction between the window/delegate/etc.

Fair point, although I think there is value in consistency with Cocoa, at least in my experience (which is admmitedly not much when it comes to MacOS development) when I have to search for how to do things I will usually find the Cocoa version then adapt it to Rust.

and I'm not even sure what default implementation we could provide in the controller trait, perhaps calling back to NSWindow?)

This is a very good question and I've unfortunately been up for 24 hours now, so will need to mull it over after some sleep.

Any conclusion here on what would be the best default implementation?

I'm less interested in the flexibility and more interested in "can users who read the Rust/cacao docs immediately see what possibilities are there" without needing to go and learn all of Cocoa itself. If people have to drop to doing their own subclasses for specific bits of Cocoa functionality, then they may as well use objc directly or something.

That all said, I'm not necessarily against this PR - that is to say, merging it since it does give an interesting lower level hook strategy, but also simultaneously looking at improving the Cocoa hooks that cacao offers. I'll sleep on it and see what I land on - either way the effort's appreciated!

Having both implementations sounds good to me too :)

@ryanmcgrath
Copy link
Owner

Any conclusion here on what would be the best default implementation?

I've not had time to sit down and figure out what the exact API should look like, but I do want to try and find time this week for it - work's just been insane lately. :(

This is useful to allow overriding some behaviours that are not
sent to WindowsDelegate (for example canBecomeKey, etc). Will default to
NSWindow so it will be a noop for existing code.
@vseguip
Copy link
Author

vseguip commented Jul 13, 2023

I've updated the PR with a fix for the formatting issue.

@vseguip
Copy link
Author

vseguip commented Aug 1, 2023

Friendly ping :)

@ryanmcgrath
Copy link
Owner

Appreciate the ping, unfortunately I'm juggling a lot at the moment so this might sit for a bit longer - and I want to get ahead and merge @madsmtm objc2 work before doing anything else that touches a layer like this just so we're not getting too wildly divergent codebases going on.

I'll leave it open tho since I'm still interested on whether there's something to bring in here, definitely still open for discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants