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 webkit example #360

Merged
merged 5 commits into from
Jan 29, 2023
Merged

Add webkit example #360

merged 5 commits into from
Jan 29, 2023

Conversation

silvanshade
Copy link
Contributor

I created a small example using the generated WebKit bindings. If you have any ideas on how to improve the structure, feel free to make whatever changes you'd like.

I also made some feature sets for the examples so that they can be run a little easier and I added a README.

@silvanshade
Copy link
Contributor Author

silvanshade commented Jan 19, 2023

EDIT: I think I figured out most of the issues I mentioned (but feedback still welcome)

Copy link
Owner

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

Yes please, examples are very much welcome!

crates/icrate/examples/README.md Outdated Show resolved Hide resolved
crates/icrate/Cargo.toml Show resolved Hide resolved
crates/icrate/examples/browser.rs Outdated Show resolved Hide resolved
crates/icrate/examples/browser.rs Show resolved Hide resolved
crates/icrate/examples/browser.rs Outdated Show resolved Hide resolved
@silvanshade
Copy link
Contributor Author

One observation I had about writing the init method is that it would be nice if we could somehow use ? but I wasn't sure if it was possible to return a Result here:

unsafe fn __init_withTextField_andWebView(self: &mut Self, text_field: *mut NSTextField, web_view: *mut WKWebView) -> Option<&mut Self> {
    let this: Option<&mut Self> = msg_send![super(self), init];
    this.and_then(|this| {
        Id::retain(text_field).and_then(|text_field| {
            Id::retain(web_view).and_then(|web_view| {
                Ivar::write(&mut this.text_field, text_field);
                Ivar::write(&mut this.web_view, web_view);
                Some(this)
            })
        })
    })
}

The .and_then chaining feels a little clunky though. Ideally Id::retain would return a Result there too then, or maybe we could have some variant of it that does (since writing .ok_or(<some long-ish reason>) repeatedly is still verbose IMO).

If try_blocks were stable, that would also work nicely here if returning Result is generally problematic, since we could just write a try-block and terminate with .ok().

@madsmtm
Copy link
Owner

madsmtm commented Jan 26, 2023

One observation I had about writing the init method is that it would be nice if we could somehow use ?

You can use ? on Option<T>.

After #173, you would even be able to write:

#[method_id(initWithTextField:andWebView:)]
unsafe fn __init_withTextField_andWebView(this: Allocated<Self>, text_field: *mut NSTextField, web_view: *mut WKWebView) -> Option<Id<Self, Owned>> {
    let this = unsafe { msg_send_id![super(self), init] }?;
    Ivar::write(&mut this.text_field, unsafe { Id::retain(text_field) }?);
    Ivar::write(&mut this.web_view, unsafe { Id::retain(web_view) }?);
    Some(this)
}

Also, I have ideas for making Id::retain safe for some cases, see #399.

Copy link
Owner

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

Thanks for doing the unsafe change; I know it looks ugly, but hopefully with #359 and the property stuff in #330, things should get a lot better!

crates/icrate/examples/browser.rs Outdated Show resolved Hide resolved
crates/icrate/examples/browser.rs Outdated Show resolved Hide resolved
crates/icrate/examples/browser.rs Show resolved Hide resolved
@silvanshade
Copy link
Contributor Author

You can use ? on Option<T>.

Very interesting. Did this change recently? I thought it used to not work (or it kind of did, but there was some problem involving NoneError)?

crates/icrate/Cargo.toml Outdated Show resolved Hide resolved
crates/icrate/Cargo.toml Show resolved Hide resolved
crates/icrate/examples/browser.rs Outdated Show resolved Hide resolved
crates/icrate/examples/browser.rs Outdated Show resolved Hide resolved
@madsmtm
Copy link
Owner

madsmtm commented Jan 27, 2023

You can use ? on Option<T>.

Very interesting. Did this change recently? I thought it used to not work (or it kind of did, but there was some problem involving NoneError)?

Well, recently is subjective, but godbolt shows it worked since Rust 1.22 ;)

@madsmtm madsmtm added documentation Improvements or additions to documentation A-framework Affects the framework crates and the translator for them labels Jan 27, 2023
.github/workflows/ci.yml Outdated Show resolved Hide resolved
Copy link
Owner

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

Cool, thanks!

text_field: *mut NSTextField,
web_view: *mut WKWebView,
) -> Option<&mut Self> {
let this: Option<&mut Self> = msg_send![super(self), init];
Copy link
Owner

Choose a reason for hiding this comment

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

That's weird, I wouldn't have expected you to be able to elide the unsafe around this message send when we have #![deny(unsafe_op_in_unsafe_fn)]. But that's a different issue

@madsmtm madsmtm merged commit 7e702d2 into madsmtm:master Jan 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-framework Affects the framework crates and the translator for them documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants