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

Remove window handles from Apple handles #129

Merged
merged 1 commit into from
Jul 28, 2023

Conversation

madsmtm
Copy link
Member

@madsmtm madsmtm commented Jun 24, 2023

The window can easily be fetched from the view, and having it leads to confusion over which place you should be e.g. taking the drawing dimensions (always the view).

Part of #123

@madsmtm madsmtm added the enhancement New feature or request label Jun 24, 2023
Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's precedent for keeping derivable information in the window handle itself; the X11 window handles contain the visual ID despite the fact that you can figure out the visual from the window. This could be fixed by just adjusting the order/documentation to make it clearer that the view takes precedence.

Although it would be fine, the reason why I'm opposed is because I was planning on doing the semver trick by re-exporting the RawWindowHandle from v0.6 in v0.5 to avoid breaking that part of the API. If there's a breaking change in the window handle here that would become impossible or at least significantly less effective.

That being said it's macOS, you definitely know best in this area. Your call.

@kchibisov
Copy link
Member

I think the reason we have them is to be able to pass opaque pointers around and don't pull dependencies to take ns_window or x11_visual, it's true that you may not set them, but it could make things more complicated. We do use ns_window in glutin for example, though, we already have objc pulled.

@madsmtm
Copy link
Member Author

madsmtm commented Jun 24, 2023

The problem is that the documentation says that an implementation of the trait is allowed to only fill in the view, and that the user of the trait, if the window is not present, must then figure it out from the view. So a fully conforming user must pull in objc2 to get the window anyhow.

Another point against providing redundant information is that you have to ensure that they are in sync.

@madsmtm
Copy link
Member Author

madsmtm commented Jun 24, 2023

I don't think this prevents the semver trick? It should go something like:

// In version 0.5.4
impl<T: v6::HasRawWindowHandle> HasRawWindowHandle for T {
    fn raw_window_handle(&self) -> RawWindowHandle {
        match ... {
            v6::AppKitHandle(handle) => {
                let new_handle = AppKitHandle::empty();
                new_handle.ns_view = handle.ns_view;
                new_handle
            }
            ...
        }
    }
}

@Lokathor
Copy link
Contributor

The current fields are that way because SDL2 provides the window and does not give the view.

@madsmtm
Copy link
Member Author

madsmtm commented Jun 24, 2023

Perhaps any SDL2 Rust crate that uses raw-window-handle should be changed to output the view then? Since that is what anything that wants to render will have to do anyway. Should just be a call to [window contentView].

But thanks for the reference!

@notgull
Copy link
Member

notgull commented Jul 16, 2023

Perhaps any SDL2 Rust crate that uses raw-window-handle should be changed to output the view then? Since that is what anything that wants to render will have to do anyway. Should just be a call to [window contentView].

This sounds like a sane solution. @Lokathor would this work for SDL2 crates?

LGTM otherwise

@Lokathor
Copy link
Contributor

Yeah I mean I guess that sounds fine? I'm not an apple user really.

There's some changelog conflicts that have cropped up that need to be fixed it seems

The window can easily be fetched from the view, and having it leads to confusion over which place you should be e.g. taking the drawing dimensions (always the view).
@madsmtm madsmtm merged commit 880b305 into rust-windowing:master Jul 28, 2023
4 checks passed
@madsmtm madsmtm deleted the trim-apple-handles branch July 28, 2023 12:36
@notgull notgull mentioned this pull request Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

None yet

4 participants