Skip to content

Commit

Permalink
Use new type Estring to avoid cloning &'static str
Browse files Browse the repository at this point in the history
`ui.label("static string")` is a very common use case,
and currently egui clones the string in these cases.

This PR introduces a new type:

``` rust
pub enum Estring {
    Static(&'static str),
    Owned(Arc<str>),
}
```

which is used everywhere text is needed, with
`impl Into<Estring>` in the API for e.g. `ui.label`.

This reduces the number of copies drastically and speeds up
the benchmark demo_with_tessellate__realistic by 17%.

This hurts the ergonomics of egui a bit, and this is a breaking change.

For instance, this used to work:

``` rust
fn my_label(ui: &mut egui::Ui, text: &str) {
    ui.label(text);
}
```

This must now either be changed to

``` rust
fn my_label(ui: &mut egui::Ui, text: &str) {
    ui.label(text.to_string());
}
```

(or the argument must be changed to either
`text: &'static str` or `text: String`)
  • Loading branch information
emilk committed Sep 3, 2021
1 parent 3b75a84 commit b3e41e4
Show file tree
Hide file tree
Showing 36 changed files with 413 additions and 225 deletions.
2 changes: 1 addition & 1 deletion egui/src/containers/collapsing_header.rs
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ impl CollapsingHeader {
/// If the label is unique and static this is fine,
/// but if it changes or there are several `CollapsingHeader` with the same title
/// you need to provide a unique id source with [`Self::id_source`].
pub fn new(label: impl ToString) -> Self {
pub fn new(label: impl Into<Estring>) -> Self {
let label = Label::new(label).wrap(false);
let id_source = Id::new(label.text());
Self {
Expand Down
16 changes: 7 additions & 9 deletions egui/src/containers/combo_box.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use epaint::Shape;
pub struct ComboBox {
id_source: Id,
label: Option<Label>,
selected_text: String,
selected_text: Estring,
width: Option<f32>,
}

Expand Down Expand Up @@ -56,9 +56,8 @@ impl ComboBox {
}

/// What we show as the currently selected value
#[allow(clippy::needless_pass_by_value)]
pub fn selected_text(mut self, selected_text: impl ToString) -> Self {
self.selected_text = selected_text.to_string();
pub fn selected_text(mut self, selected_text: impl Into<Estring>) -> Self {
self.selected_text = selected_text.into();
self
}

Expand Down Expand Up @@ -143,11 +142,10 @@ impl ComboBox {
}
}

#[allow(clippy::needless_pass_by_value)]
fn combo_box<R>(
ui: &mut Ui,
button_id: Id,
selected: impl ToString,
selected: impl Into<Estring>,
menu_contents: impl FnOnce(&mut Ui) -> R,
) -> InnerResponse<Option<R>> {
let popup_id = button_id.with("popup");
Expand All @@ -158,9 +156,9 @@ fn combo_box<R>(
let full_minimum_width = ui.spacing().slider_width;
let icon_size = Vec2::splat(ui.spacing().icon_width);

let galley =
ui.fonts()
.layout_delayed_color(selected.to_string(), TextStyle::Button, f32::INFINITY);
let galley = ui
.fonts()
.layout_delayed_color(selected, TextStyle::Button, f32::INFINITY);

let width = galley.size.x + ui.spacing().item_spacing.x + icon_size.x;
let width = width.at_least(full_minimum_width);
Expand Down
2 changes: 1 addition & 1 deletion egui/src/containers/popup.rs
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ fn show_tooltip_at_avoid<R>(
/// egui::show_tooltip_text(ui.ctx(), egui::Id::new("my_tooltip"), "Helpful text");
/// }
/// ```
pub fn show_tooltip_text(ctx: &CtxRef, id: Id, text: impl ToString) -> Option<()> {
pub fn show_tooltip_text(ctx: &CtxRef, id: Id, text: impl Into<Estring>) -> Option<()> {
show_tooltip(ctx, id, |ui| {
ui.add(crate::widgets::Label::new(text));
})
Expand Down
5 changes: 2 additions & 3 deletions egui/src/containers/window.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,8 @@ impl<'open> Window<'open> {
/// The window title is used as a unique [`Id`] and must be unique, and should not change.
/// This is true even if you disable the title bar with `.title_bar(false)`.
/// If you need a changing title, you must call `window.id(…)` with a fixed id.
#[allow(clippy::needless_pass_by_value)]
pub fn new(title: impl ToString) -> Self {
let title = title.to_string();
pub fn new(title: impl Into<Estring>) -> Self {
let title = title.into();
let area = Area::new(&title);
let title_label = Label::new(title).text_style(TextStyle::Heading).wrap(false);
Self {
Expand Down
4 changes: 2 additions & 2 deletions egui/src/layout.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::{egui_assert, emath::*, Align};
use crate::{egui_assert, emath::*, Align, Estring};
use std::f32::INFINITY;

// ----------------------------------------------------------------------------
Expand Down Expand Up @@ -737,7 +737,7 @@ impl Layout {
painter: &crate::Painter,
region: &Region,
stroke: epaint::Stroke,
text: impl ToString,
text: impl Into<Estring>,
) {
let cursor = region.cursor;
let next_pos = self.next_widget_position(region);
Expand Down
6 changes: 4 additions & 2 deletions egui/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,7 @@ pub use emath as math; // historical reasons
pub use emath::{lerp, pos2, remap, remap_clamp, vec2, Align, Align2, NumExt, Pos2, Rect, Vec2};
pub use epaint::{
color, mutex,
text::{FontDefinitions, FontFamily, TextStyle},
text::{Estring, FontDefinitions, FontFamily, TextStyle},
ClippedMesh, Color32, Rgba, Shape, Stroke, Texture, TextureId,
};

Expand All @@ -378,7 +378,9 @@ pub use {
};

pub mod text {
pub use epaint::text::{Galley, LayoutJob, LayoutSection, TextFormat, TAB_SIZE};
pub use epaint::text::{
Galley, LayoutJob, LayoutJobBuilder, LayoutSection, TextFormat, TAB_SIZE,
};
}

// ----------------------------------------------------------------------------
Expand Down
7 changes: 3 additions & 4 deletions egui/src/menu.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ pub fn bar<R>(ui: &mut Ui, add_contents: impl FnOnce(&mut Ui) -> R) -> InnerResp
/// Returns `None` if the menu is not open.
pub fn menu<R>(
ui: &mut Ui,
title: impl ToString,
title: impl Into<Estring>,
add_contents: impl FnOnce(&mut Ui) -> R,
) -> Option<R> {
menu_impl(ui, title, Box::new(add_contents))
Expand Down Expand Up @@ -104,13 +104,12 @@ pub(crate) fn menu_ui<'c, R>(
})
}

#[allow(clippy::needless_pass_by_value)]
fn menu_impl<'c, R>(
ui: &mut Ui,
title: impl ToString,
title: impl Into<Estring>,
add_contents: Box<dyn FnOnce(&mut Ui) -> R + 'c>,
) -> Option<R> {
let title = title.to_string();
let title = title.into();
let bar_id = ui.id();
let menu_id = bar_id.with(&title);

Expand Down
27 changes: 9 additions & 18 deletions egui/src/painter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use crate::{
};
use epaint::{
mutex::Mutex,
text::{Fonts, Galley, TextStyle},
text::{Estring, Fonts, Galley, TextStyle},
Shape, Stroke,
};

Expand Down Expand Up @@ -193,33 +193,25 @@ impl Painter {

/// ## Debug painting
impl Painter {
#[allow(clippy::needless_pass_by_value)]
pub fn debug_rect(&mut self, rect: Rect, color: Color32, text: impl ToString) {
pub fn debug_rect(&mut self, rect: Rect, color: Color32, text: impl Into<Estring>) {
self.rect_stroke(rect, 0.0, (1.0, color));
let text_style = TextStyle::Monospace;
self.text(
rect.min,
Align2::LEFT_TOP,
text.to_string(),
text_style,
color,
);
self.text(rect.min, Align2::LEFT_TOP, text.into(), text_style, color);
}

pub fn error(&self, pos: Pos2, text: impl std::fmt::Display) -> Rect {
self.debug_text(pos, Align2::LEFT_TOP, Color32::RED, format!("🔥 {}", text))
}

/// text with a background
#[allow(clippy::needless_pass_by_value)]
pub fn debug_text(
&self,
pos: Pos2,
anchor: Align2,
color: Color32,
text: impl ToString,
text: impl Into<Estring>,
) -> Rect {
let galley = self.layout_no_wrap(text.to_string(), TextStyle::Monospace, color);
let galley = self.layout_no_wrap(text, TextStyle::Monospace, color);
let rect = anchor.anchor_rect(Rect::from_min_size(pos, galley.size));
let frame_rect = rect.expand(2.0);
self.add(Shape::Rect {
Expand Down Expand Up @@ -332,16 +324,15 @@ impl Painter {
/// [`Self::layout`] or [`Self::layout_no_wrap`].
///
/// Returns where the text ended up.
#[allow(clippy::needless_pass_by_value)]
pub fn text(
&self,
pos: Pos2,
anchor: Align2,
text: impl ToString,
text: impl Into<Estring>,
text_style: TextStyle,
text_color: Color32,
) -> Rect {
let galley = self.layout_no_wrap(text.to_string(), text_style, text_color);
let galley = self.layout_no_wrap(text, text_style, text_color);
let rect = anchor.anchor_rect(Rect::from_min_size(pos, galley.size));
self.galley(rect.min, galley);
rect
Expand All @@ -353,7 +344,7 @@ impl Painter {
#[inline(always)]
pub fn layout(
&self,
text: String,
text: impl Into<Estring>,
text_style: TextStyle,
color: crate::Color32,
wrap_width: f32,
Expand All @@ -367,7 +358,7 @@ impl Painter {
#[inline(always)]
pub fn layout_no_wrap(
&self,
text: String,
text: impl Into<Estring>,
text_style: TextStyle,
color: crate::Color32,
) -> std::sync::Arc<Galley> {
Expand Down
2 changes: 1 addition & 1 deletion egui/src/placer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ impl Placer {
}

impl Placer {
pub(crate) fn debug_paint_cursor(&self, painter: &crate::Painter, text: impl ToString) {
pub(crate) fn debug_paint_cursor(&self, painter: &crate::Painter, text: impl Into<Estring>) {
let stroke = Stroke::new(1.0, Color32::DEBUG_COLOR);

if let Some(grid) = &self.grid {
Expand Down
7 changes: 3 additions & 4 deletions egui/src/response.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
use crate::{
emath::{lerp, Align, Pos2, Rect, Vec2},
CursorIcon, PointerButton, NUM_POINTER_BUTTONS,
CtxRef, CursorIcon, Estring, Id, LayerId, PointerButton, Sense, Ui, NUM_POINTER_BUTTONS,
};
use crate::{CtxRef, Id, LayerId, Sense, Ui};

// ----------------------------------------------------------------------------

Expand Down Expand Up @@ -373,14 +372,14 @@ impl Response {
/// For that, use [`Self::on_disabled_hover_text`] instead.
///
/// If you call this multiple times the tooltips will stack underneath the previous ones.
pub fn on_hover_text(self, text: impl ToString) -> Self {
pub fn on_hover_text(self, text: impl Into<Estring>) -> Self {
self.on_hover_ui(|ui| {
ui.add(crate::widgets::Label::new(text));
})
}

/// Show this text when hovering if the widget is disabled.
pub fn on_disabled_hover_text(self, text: impl ToString) -> Self {
pub fn on_disabled_hover_text(self, text: impl Into<Estring>) -> Self {
self.on_disabled_hover_ui(|ui| {
ui.add(crate::widgets::Label::new(text));
})
Expand Down
2 changes: 1 addition & 1 deletion egui/src/style.rs
Original file line number Diff line number Diff line change
Expand Up @@ -919,7 +919,7 @@ impl DebugOptions {
fn slider_vec2<'a>(
value: &'a mut Vec2,
range: std::ops::RangeInclusive<f32>,
text: &'a str,
text: &'static str,
) -> impl Widget + 'a {
move |ui: &mut crate::Ui| {
ui.horizontal(|ui| {
Expand Down
22 changes: 11 additions & 11 deletions egui/src/ui.rs
Original file line number Diff line number Diff line change
Expand Up @@ -979,7 +979,7 @@ impl Ui {
/// Shortcut for `add(Hyperlink::new(url))`
///
/// See also [`Hyperlink`].
pub fn hyperlink(&mut self, url: impl ToString) -> Response {
pub fn hyperlink(&mut self, url: impl Into<Estring>) -> Response {
Hyperlink::new(url).ui(self)
}

Expand All @@ -991,7 +991,7 @@ impl Ui {
/// ```
///
/// See also [`Hyperlink`].
pub fn hyperlink_to(&mut self, label: impl ToString, url: impl ToString) -> Response {
pub fn hyperlink_to(&mut self, label: impl Into<Estring>, url: impl Into<Estring>) -> Response {
Hyperlink::new(url).text(label).ui(self)
}

Expand Down Expand Up @@ -1031,7 +1031,7 @@ impl Ui {
/// See also [`Button`].
#[must_use = "You should check if the user clicked this with `if ui.button(…).clicked() { … } "]
#[inline(always)]
pub fn button(&mut self, text: impl ToString) -> Response {
pub fn button(&mut self, text: impl Into<Estring>) -> Response {
Button::new(text).ui(self)
}

Expand All @@ -1041,19 +1041,19 @@ impl Ui {
///
/// Shortcut for `add(Button::new(text).small())`
#[must_use = "You should check if the user clicked this with `if ui.small_button(…).clicked() { … } "]
pub fn small_button(&mut self, text: impl ToString) -> Response {
pub fn small_button(&mut self, text: impl Into<Estring>) -> Response {
Button::new(text).small().ui(self)
}

/// Show a checkbox.
pub fn checkbox(&mut self, checked: &mut bool, text: impl ToString) -> Response {
pub fn checkbox(&mut self, checked: &mut bool, text: impl Into<Estring>) -> Response {
Checkbox::new(checked, text).ui(self)
}

/// Show a [`RadioButton`].
/// Often you want to use [`Self::radio_value`] instead.
#[must_use = "You should check if the user clicked this with `if ui.radio(…).clicked() { … } "]
pub fn radio(&mut self, selected: bool, text: impl ToString) -> Response {
pub fn radio(&mut self, selected: bool, text: impl Into<Estring>) -> Response {
RadioButton::new(selected, text).ui(self)
}

Expand All @@ -1078,7 +1078,7 @@ impl Ui {
&mut self,
current_value: &mut Value,
selected_value: Value,
text: impl ToString,
text: impl Into<Estring>,
) -> Response {
let mut response = self.radio(*current_value == selected_value, text);
if response.clicked() {
Expand All @@ -1092,7 +1092,7 @@ impl Ui {
///
/// See also [`SelectableLabel`].
#[must_use = "You should check if the user clicked this with `if ui.selectable_label(…).clicked() { … } "]
pub fn selectable_label(&mut self, checked: bool, text: impl ToString) -> Response {
pub fn selectable_label(&mut self, checked: bool, text: impl Into<Estring>) -> Response {
SelectableLabel::new(checked, text).ui(self)
}

Expand All @@ -1106,7 +1106,7 @@ impl Ui {
&mut self,
current_value: &mut Value,
selected_value: Value,
text: impl ToString,
text: impl Into<Estring>,
) -> Response {
let mut response = self.selectable_label(*current_value == selected_value, text);
if response.clicked() {
Expand Down Expand Up @@ -1303,7 +1303,7 @@ impl Ui {
/// A [`CollapsingHeader`] that starts out collapsed.
pub fn collapsing<R>(
&mut self,
heading: impl ToString,
heading: impl Into<Estring>,
add_contents: impl FnOnce(&mut Ui) -> R,
) -> CollapsingResponse<R> {
CollapsingHeader::new(heading).show(self, add_contents)
Expand Down Expand Up @@ -1644,7 +1644,7 @@ impl Ui {

/// Shows the given text where the next widget is to be placed
/// if when [`Context::set_debug_on_hover`] has been turned on and the mouse is hovering the Ui.
pub fn trace_location(&self, text: impl ToString) {
pub fn trace_location(&self, text: impl Into<Estring>) {
let rect = self.max_rect();
if self.style().debug.debug_on_hover && self.rect_contains_pointer(rect) {
self.placer
Expand Down
Loading

0 comments on commit b3e41e4

Please sign in to comment.