Embedding the Wrapper object in a struct

[MathieuDuponchelle]

Hey, thanks for the crate :)

I'm writing a simple GStreamer wrapper around it, my end use case is to format "any" text so that it can be used as input to a CEA 608 closed captioning element, CEA 608 exposes a simple grid system to position characters, that grid is at most 32 columns wide.

The element receives buffers of data, with a timestamp and a duration, and each buffer must be formatted as it arrives and sent out on the spot. I don't want to recreate a wrapper for each buffer, and instead store it in a State structure. In addition, the interface I exposed on the element accepts an optional path to a dictionary, which can be changed at runtime, including setting it to None to not use hyphenation at all.

The implementation I ended up with is https://gitlab.freedesktop.org/meh/gst-plugins-rs/-/blob/textwrap/text/wrap/src/gsttextwrap.rs#L101 , and I'll continue that way for now, but that's not ideal: I'd much rather store a Wrapper<WordSplitter>, with its default set to Wrapper::with_splitter(columns, NoHyphenation). I can't do this however of course, because the size of WordSplitter is not known at compile-time, but I also can't use Box because there is no impl for it either. I would also rather not have to use 'static as the lifetime here.

I'm not very well-versed in terms of designing rust APIs so I don't have a good suggestion, but I hope we can come up with something a bit more elegant for that use case :)

Again, thanks for the crate, the functionality is exactly what I was looking for!

[mgeisler]

Hi @MathieuDuponchelle, thanks for the feedback and the question. It seems the link is dead, I get a 404 Not Found when I visit it just now.

You raise a great point that I hadn't considered before... I'm also not an expert in this :-)

Since the Wrapper is just a small struct around the configuration, perhaps it's not so bad to store two of them? As a side note, I no longer don't like the API with the Wrapper struct. I think it would be nicer to have some free functions instead. They could then take a struct similar to Wrapper with a bunch of configuration options.

If you don't like creating two or more Wrapper values, then you can perhaps implement your own WordWrapper. Perhaps that is what you've already done? :-)

After some experimentation, I got this to compile:

use hyphenation::{Language, Load, Standard};
use std::cell::RefCell;
use textwrap::{HyphenSplitter, NoHyphenation, WordSplitter, Wrapper};

struct MyWordSplitter {
    splitter: RefCell<Box<dyn WordSplitter>>,
}

impl MyWordSplitter {
    fn new() -> MyWordSplitter {
        MyWordSplitter {
            splitter: RefCell::new(Box::new(NoHyphenation)),
        }
    }
}

impl WordSplitter for &MyWordSplitter {
    fn split<'w>(&self, word: &'w str) -> Vec<(&'w str, &'w str, &'w str)> {
        self.splitter.borrow().split(word)
    }
}

fn main() {
    let text = "Here is some well-behaved automatic wrapping.";
    let my_word_splitter = MyWordSplitter::new();
    let wrapper = Wrapper::with_splitter(20, &my_word_splitter);
    println!("NoHyphenation: {:#?}", wrapper.wrap(text));

    *my_word_splitter.splitter.borrow_mut() = Box::new(HyphenSplitter);
    println!("HyphenSplitter: {:#?}", wrapper.wrap(text));

    let dictionary = Standard::from_embedded(Language::EnglishUS).unwrap();
    *my_word_splitter.splitter.borrow_mut() = Box::new(dictionary);
    println!("EnglishUS: {:#?}", wrapper.wrap(text));
}

The output of cargo run:

NoHyphenation: [
    "Here is some",
    "well-behaved",
    "automatic wrapping.",
]
HyphenSplitter: [
    "Here is some well-",
    "behaved",
    "automatic wrapping.",
]
EnglishUS: [
    "Here is some",
    "well-behaved auto-",
    "matic wrapping.",
]

[MathieuDuponchelle]

Oh my bad, the branch was merged since then and it was set to auto-delete on merge, here's how I ended up doing this: https://gitlab.freedesktop.org/gstreamer/gst-plugins-rs/-/blob/master/text/wrap/src/gsttextwrap.rs#L100 , I do like it more than my initial version already :)

Your solution is I think the most optimal in terms of memory usage, only disadvantage I would see is that it sort of exposes implementation details (I ideally would like to know nothing about that split method :)), I'll think about which one I want to pick and let you know.

Thanks a lot for spending time on that exercise, I'll also be curious about your ideas for a different API, if you care to put down some pseudo code I can comment on it. CCing @sdroege, I had discussed the problem with him and I think he may have suggestions :)

[mgeisler]

Hi @MathieuDuponchelle,

With the changes in #206, you should be able to drop the

enum Wrapper {
    H(textwrap::Wrapper<'static, Standard>),
    N(textwrap::Wrapper<'static, textwrap::NoHyphenation>),
}

code since the type of the WordSplitter has been changed to Box<dyn WordSplitter>. That is, I changed the API to use dynamic dispatch — I couldn't measure any difference in performance and it seemed to make things smoother.

Since #206, I've renamed Wrapper to Options, please see #213. In short, you change

wrapper.fill("some text")

to

textwrap::fill("some text", options)

where options is a textwrap::Options which is constructed just like a good old textwrap::Wrapper was. I made this change since I felt it was nicer to separate state (options) from behavior (filling and wrapping).

In your code, I believe you can then simply put a textwrap::Options into your state. Your update_wrapper function will then assign a Box::new(your_dictionary) to the options.splitter field when changing language.

I made a little demo program here to test this kind of code myself: https://github.com/mgeisler/textwrap/blob/master/examples/interactive.rs#L191

Can I get you to try out the new API and let me know if you find any weirdness in the new API?

[sdroege]

This works fine but in @MathieuDuponchelle's code it only works after also doing #254

[MathieuDuponchelle]

hrm, sorry this slipped off my mind, thanks for testing @sdroege :)

[mgeisler]

Thanks both for helping test the new API, much appreciated! I think this can be closed now since #254 is on its way to being merged.