kornelski / rust-rgb

struct RGB for sharing pixels between crates
https://lib.rs/rgb
MIT License
98 stars 19 forks source link

Reverse Dependency Backwards Compatability Testing #129

Closed ripytide closed 2 months ago

ripytide commented 3 months ago

I've tested some of the most popular reverse dependencies of rgb to test the main branch's backwards compatibility:

Diff < left / right > : < Flame Graph

Flame Graph
error[E0599]: no method named into_u8 found for reference &Gray<C> in the current scope --> src/impls.rs:78:36 78 fn as_u32(&self) -> u32 { self.into_u8() as u32 * 0x010101 } ^^^^^^^
= help: items from traits can only be used if the trait is implemented and in scope note: Component defines an item into_u8, perhaps you need to implement it --> src/impls.rs:28:1 28 trait Component: Copy { ^^^^^^^^^^^^^^^^^^^^^ help: one of the expressions' fields has a method of the same name
78 fn as_u32(&self) -> u32 { self.0.into_u8() as u32 * 0x010101 }
++
help: one of the expressions' fields has a method of the same name 78 fn as_u32(&self) -> u32 { self.v.into_u8() as u32 * 0x010101 } ++ help: there is a method into with a similar name
78 fn as_u32(&self) -> u32 { self.into() as u32 * 0x010101 }

The root cause seems to be the lack of a `Deref<Target=T>` for the `Gray_v08` types since we made it implement `Deref<Target=Gray_v09>` instead, we might have to revert back to the old `Deref` implementation.

- `i-slint-core`: Compiles, fails the same tests with and without patching with the new `rgb` branch as I don't have one of the test fonts installed.
kornelski commented 3 months ago

The mss_saliency crate didn't work without the legacy From impls, because it depends on RGB<u32>: From<RGB<u8>>.

https://github.com/kornelski/mss_saliency/blob/2e685c5f5c247d7e73c261b2fb46f02c08b2e7fe/src/lib.rs#L18;L23

kornelski commented 3 months ago

use rgb::*; use rgb::alt::*; GRAY8 breaks, because it's defined in two places differently.

ripytide commented 2 months ago

Those two issues seem like they would require major rework to fix, is it worth the effort to fix those backwards incompatibilities or can we simply expect users to fix those issues themselves after we release v0.8.90?

I think we should release v0.8.90 as is, given we've attempted backwards compatibility on a best-effort basis, how about you?

kornelski commented 2 months ago

I've removed spaces from Display.

kornelski commented 2 months ago

Unfortunately Rust can't display deprecations for uses of Deref, so moving users away from Gray's deref will be tricky.

ripytide commented 2 months ago

Hopefully the Deref issue won't be that bad as eventually Gray_v08 will be deprecated so they can migrate away from Deref at the same time they migrate to Gray_v09.

kornelski commented 2 months ago

Now it's impossible to future-proof access to Gray due to the .0 field and lack of Deref. Maybe both types should get .v() or .value() getter?

There's also no Rgb::from(gray)