Update imbl dependency to fix their bug in Focus::narrow()

[kevinaboos]

Update the imbl dependency to include this recent fix specifically: jneem/imbl#89

This is causing bugs in the matrix-rust-sdk and in downstream clients, e.g., project-robius/robrix#332

Once this is merged in, I will similarly patch the matrix-rust-sdk such that clients can leverage the fix too.

[jplatte]

I'm happy merging an upgrade to imbl 4.0, but the patch doesn't make sense as it only affects cargo commands run inside this repo, not any uses of eyeball-im as a dependency. Or was the bug that you linked a fix of only introduced in 4.0 such that tests would fail with the upgrade alone?

[kevinaboos]

I'm happy merging an upgrade to imbl 4.0, but the patch doesn't make sense as it only affects cargo commands run inside this repo, not any uses of eyeball-im as a dependency. Or was the bug that you linked a fix of only introduced in 4.0 such that tests would fail with the upgrade alone?

That's correct, the patch is listed here for the sake of (1) consistency and clarity of intent, and (2) allowing direct cargo builds of this repo alone to succeed.

The real key change is upgrading the eyeball workspace to depend on v4.0.0 of imbl, such that patches both here and in other downstream users of this crate will actually take effect. If we leave it at v3.0.0, cargo will not select the latest commit specified in the patch override.

For example, I also intend to update matrix-rust-sdk to depend on v4.0.0 of imbl which can then be patched with the same patch statement included here. Users of matrix-rust-sdk (such as our own client implementation) can then also utilize that same patch.

Alternatively, if you don't like the patch override approach, that's fine -- we can just directly write the dependency as a git dependency instead of a crates.io version dependency. That's just a little bit less canonical, but it may actually be preferable here since this crate is typically a deeply-nested transitive dependency.

[Hywan]

@kevinaboos Out of curiosity, can you point at a bug in the SDK caused by the bug in imbl?

[jplatte]

Honestly, let's just ask the maintainer of imbl to do a patch release before merging this? That seems a lot easier 😅

[kevinaboos]

@kevinaboos Out of curiosity, can you point at a bug in the SDK caused by the bug in imbl?

not directly within the SDK, but easily reproducible for immediate downstream users of the SDK: project-robius/robrix#330

Basically the problem is that the bounds check is wrong on Focus::narrow(). So, for example, if you are searching through the Vector of timeline items, narrowing the Focus with a completely valid Range that includes the last item in the timeline Vector will fail with a full panic.

[kevinaboos]

Honestly, let's just ask the maintainer of imbl to do a patch release before merging this? That seems a lot easier 😅

Haha, totally fair!

EDIT: just asked them as you recommended. I agree that's a better approach than multiple rounds of patch overrides.

[Hywan]

jneem/imbl#91 (comment) 0.4.1 is available. Are you willing to update eyeball @kevinaboos since you've initiated the idea. If you don't have time, I'll take care of it.

[kevinaboos]

jneem/imbl#91 (comment) 0.4.1 is available. Are you willing to update eyeball @kevinaboos since you've initiated the idea. If you don't have time, I'll take care of it.

Sure, happy to do it. Will submit PRs tomorrow!

[jplatte]

There, now you just need to accept my suggestions and I can squash-merge the result ^^

[kevinaboos]

There, now you just need to accept my suggestions and I can squash-merge the result ^^

Thanks! Apologies for the delay, swamped lately.

[kevinaboos]

Nice. If y'all plan to publish a new release to crates.io soon, I can wait on that before PR-ing the matrix-rust-sdk. If you intend to wait a while, I can submit a patch-based PR to point to this latest git commit.

[jplatte]

Going to release soon.

[jplatte]

eyeball-im 0.6.0 and eyeball-im-util 0.8.0 are out!

[kevinaboos]

PR submitted to matrix-rust-sdk. Thanks again for your quick replies!

matrix-org/matrix-rust-sdk#4583