ios: Upgrade to objc2 0.6 and drop block2

[MarijnS95]

The impeding objc2 0.6 release series was already hinted at in #94, and is now finally available.

Even though there's a new objc2-ui-kit dependency available that represents the UIApplication type and its methods faithfully, there was a strong preference to cut down on the number of dependencies instead so its improved signature has been incorporated here. By extension the block2 crate was removed too by inlining a simple type definition just to be able to set the Option<&Block> to None.

CC @madsmtm

[amodm]

What's the motivation for these changes, particularly the expansion of dependencies? Is there a bug which this addresses? If yes, can you please share more? If not, I wouldn't be inclined to merge this PR, except for the version upgrade.

[MarijnS95]

What's the motivation for these changes

From the PR description: to use the latest version of objc2 crates, and get rid of duplicates in my tree where we are migrating to the latest objc2 bindings too (whose improvements are critical to us).

particularly the expansion of dependencies? Is there a bug which this addresses? If yes, can you please share more? If not, I wouldn't be inclined to merge this PR

So you're asking me to drop this from the PR - and not even bother to "share more" - because it's not addressing a bug?

I'll go against your request and elaborate the comment from the PR description anyway:

It further expands this crates' usage of auto-generated bindings from the objc2 ecosystem, as started in #87. Back then bindings for objc2-ui-kit were not yet available requiring this crate to open-code the implementations, but now that @madsmtm has them generated we can trivially use it and build on top of continued usability and correctness improvements provided by such auto-generated bindings.

The fact that it's a new crate is merely because @madsmtm split out the objc2 crate to lighten the main crate.

[madsmtm]

Ugh, I hate to disagree with you @MarijnS95, since you're so passionately defending my crates (I appreciate it ❤️). But I actually superficially agree with @amodm here; there is no free lunch. While we have feature flags for controlling some of this, objc2-ui-kit is still a fairly large dependency, if not only in terms of download size. Real iOS applications will need objc2-ui-kit anyhow, so this should not matter, but as you're experiencing yourself, users might have multiple objc2 versions in their dependency tree.

Besides, UIApplication::sharedApplication isn't actually usable by webbrowser, since it panics internally when used before UIApplicationMain (that's how the header defines it, so that's how we're mapping it, but it is actually nullable). But webbrowser does want to do the NULL check, because an unsuspecting user might use it outside UIApplicationMain, and then they'd blame webbrowser for panicking.

And since we don't use the block from openURL:options:completionHandler: (and I don't see how webbrowser could with it's current API), we could actually avoid the block2 dependency as well (which won't be possible with objc2-ui-kit):

use core::ffi::c_void;
use objc2::encode::{Encode, Encoding};

#[repr(transparent)]
struct FakeBlock(*const c_void);

// SAFETY: The type is `#[repr(transparent)]` over a pointer (same layout as `Option<&block::Block<...>>`).
unsafe impl Encode for FakeBlock {
    const ENCODING: Encoding = Encoding::Block;
}

fn open_url(
    app: &NSObject,
    url: &NSURL,
    options: &NSDictionary<NSString, AnyObject>,
) {
    let fake_handler = FakeBlock(core::ptr::null());
    // SAFETY: The signature is correct, we only pass `UIApplication` as the app,
    // and the completion handler can be NULL.
    unsafe { msg_send![app, openURL: url, options: options, completionHandler: fake_handler] }
}

[amodm]

Thank you @madsmtm, you've articulated my thoughts perfectly.

@MarijnS95, I'm sure you mean well, but while I'm personally encouraging of expanded use of objc2 (evidenced by my comments in #87), changes to webbrowser have to be based in either a bug or an enhancement (including maintainability, which is why I merged #87), and my question stems from that.

If it's tree duplicates you're running into, any reason why Cargo's [patch.] mechanism is not sufficient for your requirements?

[MarijnS95]

If it's tree duplicates you're running into, any reason why Cargo's [patch.] mechanism is not sufficient for your requirements?

That mechanism only allows patching crates on the same version, using it to force webbrowser to use a different and above all incompatible version of objc2 is fortunately impossible.

While I could patch my tree to use my own fork with this update, please explain to me how that precludes opening a PR to share this update with the rest of the world?

If anything it's totally undestandable if your stance on updates is "not yet", but now it seems to be "not at all"? I'd be happy to wait and leave this PR open for community visibility until the wider ecosystem requires upgrading - or webbrowser features demand newer objc2 bindings - rather than having these changes hidden on a fork, for someone else to redo when they need it too.

enhancement (including maintainability, which is why I merged #87)

Using pregenerated bindings over incomplete handrolled wrappers is definitely a maintainability improvement.

[Jasper-Bekkers]

Using pregenerated bindings over incomplete handrolled wrappers is definitely a maintainability improvement.

And usually a compile time regression, sometimes for small crates like this are better served with 1-2 simple hand rolled bindings, it's not like the underlaying objc api will change.

[amodm]

please explain to me how that precludes opening a PR to share this update with the rest of the world?

It does not. I don't know what part of my statement implies so. However, the discretion of acceptance/rejection is mine.

but now it seems to be "not at all"?

Again, I went through my comment history on this PR, and I have no idea what's giving you this impression. I was very clear that version upgrade is fine, but for expansion of dependencies I want to see a stronger motivation.

Using pregenerated bindings over incomplete handrolled wrappers is definitely a maintainability improvement.

Without disagreeing with you entirely, I hold a different opinion on the significance of this. #87 was about doing away with a poorly maintained crate, while this adds a whole new dependency for a tiny change. Hence my question about "what bug it addresses", because the bar for such a change needs to be higher (except for the version upgrade part).

I'd be happy to wait and leave this PR open for community visibility

This is fine by me, and was my plan until you closed the PR.

To summarise again: version upgrade is fine, expansion of dependencies requires more proof of need. You can choose to keep the PR limited to only the version upgrade in which case I'll merge it immediately (tests passing). Or you can choose to wait until I get more comfortable with expansion of dependencies.

[MarijnS95]

You can choose to keep the PR limited to only the version upgrade in which case I'll merge it immediately (tests passing)

Done, incorporated all your and @madsmtm's requests.

[madsmtm]

Sorry, I filed #97 and #98 because I mis-interpreted your closing of this PR as not wanting to work on the version upgrade any more, apologies for being hasty here!

[amodm]

@madsmtm, given that this PR also has the benefit of dropping block2 (which is effectively not being used anyway), and given the discussion that has gone into it, implementation wise I'll prefer to merge this over #97 and #98. I appreciate your constructive approach in working with users of objc2.

[amodm]

This is now out as v1.0.4