Replace impl Into<String> with impl ToString

[davidpdrsn]

This is something I ran into today. Types that implement
std::fmt::Display cannot be passed to functions that take
impl Into<String>. You have to call display_thing.to_string().
Its a small thing but would be avoided by instead taking impl ToString.

Afaik impl ToString is a superset of impl Into<String>, unless users
manually implement Into<String> for T (or From<T> for String) for
their own types. However I think its more common to implement Display
as that works with println and friends. The main difference is that
Display::fmt can return errors but thats also quite rare in my
experience.

I did some testing in a playground and seems to work.

[emilk]

Thanks for the PR!

A common use case is ui.label(format!("answer: {}", 42)); which will pass in a String. With this PR, the string will need an additional clone (since to_string takes a &self).

The ergonomic win may outweigh this, but I think we should create a micro-benchmark for the label + format case and check what the added cost is before we merge this.

[davidpdrsn]

With this PR, the string will need an additional clone (since to_string takes a &self).

Ah yeah thats true. I didn't consider that.

Would you add a benchmark here https://github.com/emilk/egui/blob/master/egui_demo_lib/benches/benchmark.rs?

[emilk]

Yeah! I think it would be good to bench both ui.label("hello world"); and ui.label("hello world".to_string());.

There is also a possibility that, in a separate PR, I can remove the need for a String completely, and allow e.g. impl AsRef<str> or whatever would be the suitable alternative. When that day comes it would be good to have an idea of what the performance implications would be. Maybe impl ToString is always gonna be the most ergonomic, and the perf hit is worth it.

[emilk]

In my benchmark the ui.label(format!("…")); becomes around 6% slower with this PR, which I think is acceptable. If you get the CI green, I'll merge it!

[davidpdrsn]

Awesome!

I looked into the clippy warnings and most of them are along the lines of

error: this argument is passed by value, but not consumed in the function body
   --> egui/src/widgets/text_edit.rs:207:43
    |
207 |     pub fn hint_text(mut self, hint_text: impl ToString) -> Self {
    |                                           ^^^^^^^^^^^^^ help: consider taking a reference instead: `&impl ToString`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_pass_by_value

Supporting that lint would require changing all the callsites to use &thing instead of `thing which would be a breaking change.

@emilk What do you think about this? Should I add #[allow(...)] to the places where the lint fails or is it a breaking change you'd be okay with? I would personally go for adding #[allow(...)] to the methods that takes impl ToString but its of course up to you.

[emilk]

Yeah, mute it. The point of this PR is an ergonomic win, so forcing people to write &format!(…) would be against the purpose.

[davidpdrsn]

Done!

[emilk]

Thanks!