impl WordSplitter for Box<T> where T: WordSplitter

[sdroege]

No description provided.

[sdroege]

See https://gitlab.freedesktop.org/gstreamer/gst-plugins-rs/-/merge_requests/448 for a place where this is required and a workaround for the missing impl.

[mgeisler]

Hi @sdroege,

Thanks for plugging more holes in this. I tried testing with gst-plugin-textwrap and I cannot make it work.

I looked at https://gitlab.freedesktop.org/gstreamer/gst-plugins-rs/-/merge_requests/448 and tried to adapt it to what I think you had in mind:

modified   text/wrap/src/gsttextwrap/imp.rs
@@ -95,18 +95,8 @@ impl Default for Settings {
     }
 }
 
-// FIXME: Not needed anymore after https://github.com/mgeisler/textwrap/pull/254
-#[derive(Debug)]
-struct Splitter(Box<dyn textwrap::WordSplitter + Send>);
-
-impl textwrap::WordSplitter for Splitter {
-    fn split_points(&self, word: &str) -> Vec<usize> {
-        self.0.split_points(word)
-    }
-}
-
 struct State {
-    options: Option<textwrap::Options<'static, Splitter>>,
+    options: Option<textwrap::Options<'static, Box<dyn textwrap::WordSplitter>>>,
 }
 
 impl Default for State {
@@ -156,12 +146,12 @@ impl TextWrap {
 
             Some(textwrap::Options::with_splitter(
                 settings.columns as usize,
-                Splitter(Box::new(standard)),
+                Box::new(standard),
             ))
         } else {
             Some(textwrap::Options::with_splitter(
                 settings.columns as usize,
-                Splitter(Box::new(textwrap::NoHyphenation)),
+                Box::new(textwrap::NoHyphenation),
             ))
         };
     }

I then ran cargo check with the textwrap dependency set to my local copy. I tried merging this PR into the copy by coping the two implementations of WordSplitter. I also tried to uncomment the alternative implementation you commented on.

In both cases, I kept this error:

[mg@x1c]% cargo check
    Checking textwrap v0.13.1 (/home/mg/src/textwrap)
    Checking gst-plugin-textwrap v0.6.0 (/home/mg/src/gst-plugins-rs/text/wrap)
error[E0277]: `(dyn WordSplitter + 'static)` cannot be sent between threads safely
   --> text/wrap/src/gsttextwrap/imp.rs:372:6
    |
372 | impl ElementImpl for TextWrap {
    |      ^^^^^^^^^^^ `(dyn WordSplitter + 'static)` cannot be sent between threads safely
    | 
   ::: /home/mg/.cargo/git/checkouts/gstreamer-rs-79e52a2d27eb91a3/fb56af8/gstreamer/src/subclass/element.rs:23:61
    |
23  | pub trait ElementImpl: ElementImplExt + ObjectImpl + Send + Sync {
    |                                                             ---- required by this bound in `gstreamer::subclass::prelude::ElementImpl`
    |
    = help: the trait `Send` is not implemented for `(dyn WordSplitter + 'static)`
    = note: required because of the requirements on the impl of `Send` for `Unique<(dyn WordSplitter + 'static)>`
    = note: required because it appears within the type `Box<(dyn WordSplitter + 'static)>`
    = note: required because it appears within the type `Options<'static>`
    = note: required because it appears within the type `Option<Options<'static>>`
    = note: required because it appears within the type `imp::State`
    = note: required because of the requirements on the impl of `std::marker::Sync` for `Mutex<imp::State>`
    = note: required because it appears within the type `imp::TextWrap`

error: aborting due to previous error

I probably just misunderstood how to remove the Splitter work-around?

Though I'm a bit perplexed by the sheer number of implementations needed, I'm happy to merge this if you've tested it and gotten it to work on your side?

[sdroege]

-    options: Option<textwrap::Options<'static, Box<dyn textwrap::WordSplitter>>>,
+    options: Option<textwrap::Options<'static, Box<dyn textwrap::WordSplitter + Send>>>,

This is what is needed in both cases

[mgeisler]

```diff
-    options: Option<textwrap::Options<'static, Box<dyn textwrap::WordSplitter>>>,
+    options: Option<textwrap::Options<'static, Box<dyn textwrap::WordSplitter + Send>>>,

This is what is needed in both cases

Ah, right you are!

As you wrote, the uncommented and more general implementation by @Cryptjar also seems to work. So can i get you to change this PR to use that instead? I didn't fully understand the difference of the implementations back when #215 was merged and it seems we now have a great use case for the code. So it would be nice to use it :-)

[mgeisler]

Excellent, thanks!

[sdroege]

Thanks, I'll update the GStreamer code once you've uploaded a new release to crates.io

[mgeisler]

The new release has been made! 🎉