Improve Id generation, with more aggressive name clash warnings

egui/src/containers/area.rs

@@ -113,7 +113,7 @@ impl Area {
             fixed_pos,
         } = self;
 
-        let layer_id = LayerId { order, id };
+        let layer_id = LayerId::new(order, id);
 
         let state = ctx.memory().areas.get(id).cloned();
         let mut state = state.unwrap_or_else(|| State {

egui/src/containers/collapsing_header.rs

@@ -129,17 +129,19 @@ pub fn paint_icon(ui: &mut Ui, openness: f32, response: &Response) {
 pub struct CollapsingHeader {
     label: Label,
     default_open: bool,
-    id_source: Option<Id>,
+    id_source: Id,
 }
 
 impl CollapsingHeader {
     pub fn new(label: impl Into<String>) -> Self {
+        let label = Label::new(label)
+            .text_style(TextStyle::Button)
+            .multiline(false);
+        let id_source = Id::new(label.text());
         Self {
-            label: Label::new(label)
+            label,
-                .text_style(TextStyle::Button)
-                .multiline(false),
             default_open: false,
-            id_source: None,
+            id_source,
         }
     }
 
@@ -149,9 +151,9 @@ impl CollapsingHeader {
     }
 
     /// Explicitly set the source of the `Id` of this widget, instead of using title label.
-    /// This is useful if the title label is dynamics.
+    /// This is useful if the title label is dynamic or not unique.
     pub fn id_source(mut self, id_source: impl Hash) -> Self {
-        self.id_source = Some(Id::new(id_source));
+        self.id_source = Id::new(id_source);
         self
     }
 }
@@ -176,8 +178,7 @@ impl CollapsingHeader {
 
         // TODO: horizontal layout, with icon and text as labels. Insert background behind using Frame.
 
-        let title = label.text();
+        let id = ui.make_persistent_id(id_source);
-        let id = ui.make_unique_child_id_full(id_source, Some(title));
 
         let available = ui.available_finite();
         let text_pos = available.min + vec2(ui.style().spacing.indent, 0.0);

egui/src/containers/combo_box.rs

@@ -7,7 +7,7 @@ pub fn combo_box_with_label(
     menu_contents: impl FnOnce(&mut Ui),
 ) -> Response {
     let label = label.into();
-    let button_id = ui.make_unique_child_id(label.text());
+    let button_id = ui.make_persistent_id(label.text());
 
     ui.horizontal(|ui| {
         let mut response = combo_box(ui, button_id, selected, menu_contents);

egui/src/containers/resize.rs

@@ -145,7 +145,8 @@ struct Prepared {
 
 impl Resize {
     fn begin(&mut self, ui: &mut Ui) -> Prepared {
-        let id = self.id.unwrap_or_else(|| ui.make_child_id("resize"));
+        let position = ui.available().min;
+        let id = self.id.unwrap_or_else(|| ui.make_persistent_id("resize"));
 
         let mut state = ui.memory().resize.get(&id).cloned().unwrap_or_else(|| {
             ui.ctx().request_repaint(); // counter frame delay
@@ -167,8 +168,6 @@ impl Resize {
             .at_least(self.min_size)
             .at_most(self.max_size);
 
-        let position = ui.available().min;
-
         let corner_response = if self.resizable {
             // Resize-corner:
             let corner_size = Vec2::splat(ui.style().visuals.resize_corner_size);

egui/src/containers/scroll_area.rs

@@ -72,7 +72,7 @@ impl ScrollArea {
 
         let ctx = ui.ctx().clone();
 
-        let id = ui.make_child_id("scroll_area");
+        let id = ui.make_persistent_id("scroll_area");
         let state = ctx
             .memory()
             .scroll_areas
@@ -221,28 +221,21 @@ impl Prepared {
                 pos2(right, from_content(state.offset.y + inner_rect.height())),
             );
 
-            // intentionally use same id for inside and outside of handle
             let interact_id = id.with("vertical");
-            let mut response = ui.interact(handle_rect, interact_id, Sense::click_and_drag());
+            let response = ui.interact(outer_scroll_rect, interact_id, Sense::click_and_drag());
 
-            if let Some(mouse_pos) = ui.input().mouse.pos {
+            if response.active {
-                if response.active {
+                if let Some(mouse_pos) = ui.input().mouse.pos {
-                    if inner_rect.top() <= mouse_pos.y && mouse_pos.y <= inner_rect.bottom() {
+                    if handle_rect.contains(mouse_pos) {
-                        state.offset.y +=
+                        if inner_rect.top() <= mouse_pos.y && mouse_pos.y <= inner_rect.bottom() {
-                            ui.input().mouse.delta.y * content_size.y / inner_rect.height();
+                            state.offset.y +=
-                    }
+                                ui.input().mouse.delta.y * content_size.y / inner_rect.height();
-                } else {
+                        }
-                    // Check for mouse down outside handle:
+                    } else {
-                    let scroll_bg_response =
-                        ui.interact(outer_scroll_rect, interact_id, Sense::click_and_drag());
-
-                    if scroll_bg_response.active {
                         // Center scroll at mouse pos:
                         let mpos_top = mouse_pos.y - handle_rect.height() / 2.0;
                         state.offset.y = remap(mpos_top, top..=bottom, 0.0..=content_size.y);
                     }
-
-                    response = response.union(scroll_bg_response);
                 }
             }
 

egui/src/containers/window.rs

@@ -691,7 +691,7 @@ impl TitleBar {
             );
         }
 
-        let title_bar_id = ui.make_child_id("title_bar");
+        let title_bar_id = ui.make_position_id().with("title_bar");
         if ui
             .interact(self.rect, title_bar_id, Sense::click())
             .double_clicked
@@ -716,7 +716,7 @@ impl TitleBar {
 }
 
 fn close_button(ui: &mut Ui, rect: Rect) -> Response {
-    let close_id = ui.make_child_id("window_close_button");
+    let close_id = ui.make_position_id().with("window_close_button");
     let response = ui.interact(rect, close_id, Sense::click());
     ui.expand_to_include_rect(response.rect);
 

egui/src/context.rs

@@ -51,7 +51,7 @@ pub struct Context {
     // The output of a frame:
     graphics: Mutex<GraphicLayers>,
     output: Mutex<Output>,
-    /// Used to debug name clashes of e.g. windows
+    /// Used to debug `Id` clashes of widgets.
     used_ids: Mutex<AHashMap<Id, Pos2>>,
 
     paint_stats: Mutex<PaintStats>,
@@ -336,49 +336,26 @@ impl Context {
 
     // ---------------------------------------------------------------------
 
-    /// Generate a id from the given source.
+    /// If the given `Id` is not unique, an error will be printed at the given position.
-    /// If it is not unique, an error will be printed at the given position.
+    /// Call this for `Id`:s that need interaction or persistence.
-    pub fn make_unique_id<IdSource>(self: &Arc<Self>, source: IdSource, pos: Pos2) -> Id
+    pub(crate) fn register_unique_id(self: &Arc<Self>, id: Id, new_pos: Pos2) {
-    where
+        if let Some(prev_pos) = self.used_ids.lock().insert(id, new_pos) {
-        IdSource: std::hash::Hash + std::fmt::Debug + Copy,
+            if prev_pos == new_pos {
-    {
+                // Likely same Widget being interacted with twice, which is fine.
-        self.register_unique_id(Id::new(source), source, pos)
+                return;
-    }
-
-    pub fn is_unique_id(&self, id: Id) -> bool {
-        !self.used_ids.lock().contains_key(&id)
-    }
-
-    /// If the given Id is not unique, an error will be printed at the given position.
-    pub fn register_unique_id(
-        self: &Arc<Self>,
-        id: Id,
-        source_name: impl std::fmt::Debug,
-        pos: Pos2,
-    ) -> Id {
-        if let Some(clash_pos) = self.used_ids.lock().insert(id, pos) {
-            let painter = self.debug_painter();
-            if clash_pos.distance(pos) < 4.0 {
-                painter.error(
-                    pos,
-                    &format!("use of non-unique ID {:?} (name clash?)", source_name),
-                );
-            } else {
-                painter.error(
-                    clash_pos,
-                    &format!("first use of non-unique ID {:?} (name clash?)", source_name),
-                );
-                painter.error(
-                    pos,
-                    &format!(
-                        "second use of non-unique ID {:?} (name clash?)",
-                        source_name
-                    ),
-                );
             }
-            id
+
-        } else {
+            let id_str = id.short_debug_format();
-            id
+
+            let painter = self.debug_painter();
+            if prev_pos.distance(new_pos) < 4.0 {
+                painter.error(new_pos, format!("Double use of ID {}", id_str));
+            } else {
+                painter.error(prev_pos, format!("First use of ID {}", id_str));
+                painter.error(new_pos, format!("Second use of ID {}", id_str));
+            }
+
+            // TODO: a tooltip explaining this.
         }
     }
 
@@ -449,16 +426,14 @@ impl Context {
         layer_id: LayerId,
         clip_rect: Rect,
         rect: Rect,
-        interaction_id: Option<Id>,
+        id: Option<Id>,
         sense: Sense,
     ) -> Response {
         let interact_rect = rect.expand2(0.5 * self.style().spacing.item_spacing); // make it easier to click. TODO: nice way to do this
         let hovered = self.contains_mouse(layer_id, clip_rect, interact_rect);
-        let has_kb_focus = interaction_id
+        let has_kb_focus = id.map(|id| self.memory().has_kb_focus(id)).unwrap_or(false);
-            .map(|id| self.memory().has_kb_focus(id))
-            .unwrap_or(false);
 
-        if interaction_id.is_none() || sense == Sense::nothing() || !layer_id.allow_interaction() {
+        if id.is_none() || sense == Sense::nothing() || !layer_id.allow_interaction() {
             // Not interested or allowed input:
             return Response {
                 ctx: self.clone(),
@@ -471,15 +446,17 @@ impl Context {
                 has_kb_focus,
             };
         }
-        let interaction_id = interaction_id.unwrap();
+        let id = id.unwrap();
+
+        self.register_unique_id(id, rect.min);
 
         let mut memory = self.memory();
 
         memory.interaction.click_interest |= hovered && sense.click;
         memory.interaction.drag_interest |= hovered && sense.drag;
 
-        let active = memory.interaction.click_id == Some(interaction_id)
+        let active =
-            || memory.interaction.drag_id == Some(interaction_id);
+            memory.interaction.click_id == Some(id) || memory.interaction.drag_id == Some(id);
 
         if self.input.mouse.pressed {
             if hovered {
@@ -496,7 +473,7 @@ impl Context {
 
                 if sense.click && memory.interaction.click_id.is_none() {
                     // start of a click
-                    memory.interaction.click_id = Some(interaction_id);
+                    memory.interaction.click_id = Some(id);
                     response.active = true;
                 }
 
@@ -504,7 +481,7 @@ impl Context {
                     && (memory.interaction.drag_id.is_none() || memory.interaction.drag_is_window)
                 {
                     // start of a drag
-                    memory.interaction.drag_id = Some(interaction_id);
+                    memory.interaction.drag_id = Some(id);
                     memory.interaction.drag_is_window = false;
                     memory.window_interaction = None; // HACK: stop moving windows (if any)
                     response.active = true;

egui/src/demos/demo_window.rs

@@ -105,33 +105,6 @@ impl DemoWindow {
                     painter.line_segment([c, c + r * Vec2::angled(TAU * 3.0 / 8.0)], stroke);
                 });
             });
-
-        if false {
-            // TODO: either show actual name clash, or remove this example
-            ui.collapsing("Name clash demo", |ui| {
-                ui.label("\
-                    Widgets that store state require unique identifiers so we can track their state between frames. \
-                    Identifiers are normally derived from the titles of the widget.");
-
-                ui.label("\
-                    For instance, collapsable headers needs to store wether or not they are open. \
-                    If you fail to give them unique names then clicking one will open both. \
-                    To help you debug this, an error message is printed on screen:");
-
-                ui.collapsing("Collapsing header", |ui| {
-                    ui.label("Contents of first foldable ui");
-                });
-                ui.collapsing("Collapsing header", |ui| {
-                    ui.label("Contents of second foldable ui");
-                });
-
-                ui.label("\
-                    Most widgets don't need unique names, but are tracked \
-                    based on their position on screen. For instance, buttons:");
-                let _ = ui.button("Button");
-                let _ = ui.button("Button");
-            });
-        }
     }
 }
 

egui/src/demos/demo_windows.rs

@@ -39,6 +39,7 @@ impl Default for Demos {
             demos: vec![
                 (false, Box::new(crate::demos::DancingStrings::default())),
                 (false, Box::new(crate::demos::DragAndDropDemo::default())),
+                (false, Box::new(crate::demos::Tests::default())),
             ],
         }
     }
@@ -323,7 +324,7 @@ fn show_menu_bar(ui: &mut Ui, windows: &mut OpenWindows, seconds_since_midnight:
             }
             if ui
                 .button("Clear entire Egui memory")
-                .on_hover_text("Forget scroll, collapsibles etc")
+                .on_hover_text("Forget scroll, collapsing headers etc")
                 .clicked
             {
                 *ui.ctx().memory() = Default::default();

egui/src/demos/mod.rs

@@ -9,12 +9,13 @@ mod demo_windows;
 mod drag_and_drop;
 mod fractal_clock;
 mod sliders;
+mod tests;
 pub mod toggle_switch;
 mod widgets;
 
 pub use {
     app::*, color_test::ColorTest, dancing_strings::DancingStrings, demo_window::DemoWindow,
-    demo_windows::*, drag_and_drop::*, fractal_clock::FractalClock, sliders::Sliders,
+    demo_windows::*, drag_and_drop::*, fractal_clock::FractalClock, sliders::Sliders, tests::Tests,
     widgets::Widgets,
 };
 

egui/src/demos/tests.rs

@@ -0,0 +1,51 @@
+use crate::*;
+
+#[derive(Default)]
+pub struct Tests {}
+
+impl demos::Demo for Tests {
+    fn name(&self) -> &str {
+        "Tests"
+    }
+
+    fn show(&mut self, ctx: &std::sync::Arc<crate::Context>, open: &mut bool) {
+        Window::new(self.name()).open(open).show(ctx, |ui| {
+            use demos::View;
+            self.ui(ui);
+        });
+    }
+}
+
+impl demos::View for Tests {
+    fn ui(&mut self, ui: &mut Ui) {
+        ui.heading("Name collision example");
+
+        ui.label("\
+            Widgets that store state require unique and persisting identifiers so we can track their state between frames.\n\
+            For instance, collapsable headers needs to store wether or not they are open. \
+            Their Id:s are derived from their names. \
+            If you fail to give them unique names then clicking one will open both. \
+            To help you debug this, an error message is printed on screen:");
+
+        ui.collapsing("Collapsing header", |ui| {
+            ui.label("Contents of first foldable ui");
+        });
+        ui.collapsing("Collapsing header", |ui| {
+            ui.label("Contents of second foldable ui");
+        });
+
+        ui.label("\
+            Any widget that can be interacted with also need a unique Id. \
+            For most widgets the Id is generated by a running counter. \
+            As long as elements are not added or removed, the Id stays the same. \
+            This is fine, because during interaction (i.e. while dragging a slider), \
+            the number of widgets previously in the same window is most likely not changing \
+            (and if it is, the window will have a new layout, and the slider will endup somewhere else, and so aborthing the interaction probably makes sense).");
+
+        ui.label("So these buttons have automatic Id:s, and therefore there is no name clash:");
+        let _ = ui.button("Button");
+        let _ = ui.button("Button");
+
+        ui.add(__egui_github_link_file!());
+    }
+}

egui/src/id.rs

@@ -12,7 +12,7 @@ use std::hash::Hash;
 /// For some widgets `Id`s are also used to persist some state about the
 /// widgets, such as Window position or wether not a collapsing header region is open.
 ///
-/// This implies that the `Id`s must be unqiue.
+/// This implies that the `Id`s must be unique.
 ///
 /// For simple things like sliders and buttons that don't have any memory and
 /// doesn't move we can use the location of the widget as a source of identity.
@@ -54,4 +54,8 @@ impl Id {
         child.hash(&mut hasher);
         Id(hasher.finish())
     }
+
+    pub(crate) fn short_debug_format(&self) -> String {
+        format!("{:04X}", self.0 as u16)
+    }
 }

egui/src/ui.rs

@@ -85,8 +85,8 @@ impl Ui {
     }
 
     pub fn child_ui(&mut self, max_rect: Rect, layout: Layout) -> Self {
-        let id = self.make_position_id(); // TODO: is this a good idea?
         self.child_count += 1;
+        let id = self.make_position_id(); // TODO: ui.id should be persistent and not position based.
 
         let cursor = layout.initial_cursor(max_rect);
         let min_size = Vec2::zero(); // TODO: From Style
@@ -365,52 +365,22 @@ impl Ui {
 
 /// # `Id` creation
 impl Ui {
-    /// Will warn if the returned id is not guaranteed unique.
+    /// Use this to generate widget ids for widgets that have persistent state in `Memory`.
-    /// Use this to generate widget ids for widgets that have persistent state in Memory.
+    pub fn make_persistent_id<IdSource>(&self, id_source: IdSource) -> Id
-    /// If the `id_source` is not unique within this ui
-    /// then an error will be printed at the current cursor position.
-    pub fn make_unique_child_id<IdSource>(&self, id_source: IdSource) -> Id
     where
         IdSource: Hash + std::fmt::Debug,
     {
-        let id = self.id.with(&id_source);
+        self.id.with(&id_source)
-        // TODO: clip name clash error messages to clip rect
-        self.ctx().register_unique_id(id, id_source, self.cursor)
-    }
-
-    /// Ideally, all widgets should use this. TODO
-    /// Widgets can set an explicit id source (user picked, e.g. some loop index),
-    /// and a default id source (e.g. label).
-    /// If they fail to be unique, a positional id will be used instead.
-    pub fn make_unique_child_id_full(
-        &mut self,
-        explicit_id_source: Option<Id>,
-        default_id_source: Option<&str>,
-    ) -> Id {
-        let id = if let Some(explicit_id_source) = explicit_id_source {
-            self.id.with(&explicit_id_source)
-        } else {
-            let id = self.id.with(default_id_source);
-            if self.ctx().is_unique_id(id) {
-                id
-            } else {
-                self.make_position_id()
-            }
-        };
-        self.ctx()
-            .register_unique_id(id, default_id_source.unwrap_or_default(), self.cursor)
     }
 
     /// Make an Id that is unique to this position.
     /// Can be used for widgets that do NOT persist state in Memory
     /// but you still need to interact with (e.g. buttons, sliders).
+    /// Call AFTER allocating new space for your widget.
+    // TODO: return from `allocate_space` ?
     pub fn make_position_id(&self) -> Id {
         self.id.with(self.child_count)
     }
-
-    pub fn make_child_id(&self, id_seed: impl Hash) -> Id {
-        self.id.with(id_seed)
-    }
 }
 
 /// # Interaction
@@ -716,7 +686,7 @@ impl Ui {
         );
         let indent = vec2(self.style().spacing.indent, 0.0);
         let child_rect = Rect::from_min_max(self.cursor + indent, self.max_rect.right_bottom()); // TODO: wrong for reversed layouts
-        let mut child_ui = Ui {
+        let mut child_ui = Self {
             id: self.id.with(id_source),
             ..self.child_ui(child_rect, self.layout)
         };
@@ -736,20 +706,20 @@ impl Ui {
         (ret, self.interact_hover(rect))
     }
 
-    pub fn left_column(&mut self, width: f32) -> Ui {
+    pub fn left_column(&mut self, width: f32) -> Self {
         self.column(Align::Min, width)
     }
 
-    pub fn centered_column(&mut self, width: f32) -> Ui {
+    pub fn centered_column(&mut self, width: f32) -> Self {
         self.column(Align::Center, width)
     }
 
-    pub fn right_column(&mut self, width: f32) -> Ui {
+    pub fn right_column(&mut self, width: f32) -> Self {
         self.column(Align::Max, width)
     }
 
     /// A column ui with a given width.
-    pub fn column(&mut self, column_position: Align, width: f32) -> Ui {
+    pub fn column(&mut self, column_position: Align, width: f32) -> Self {
         let x = match column_position {
             Align::Min => 0.0,
             Align::Center => self.available().width() / 2.0 - width / 2.0,

egui/src/widgets/mod.rs

@@ -205,10 +205,11 @@ impl Widget for Hyperlink {
         } = self;
         let color = color::LIGHT_BLUE;
         let text_style = text_style.unwrap_or_else(|| ui.style().body_text_style);
-        let id = ui.make_child_id(&url);
         let font = &ui.fonts()[text_style];
         let galley = font.layout_multiline(text, ui.available().width());
         let rect = ui.allocate_space(galley.size);
+
+        let id = ui.make_position_id();
         let response = ui.interact(rect, id, Sense::click());
         if response.hovered {
             ui.ctx().output().cursor_icon = CursorIcon::PointingHand;
@@ -313,13 +314,13 @@ impl Widget for Button {
 
         let button_padding = ui.style().spacing.button_padding;
 
-        let id = ui.make_position_id();
         let font = &ui.fonts()[text_style];
         let galley = font.layout_multiline(text, ui.available().width());
         let mut desired_size = galley.size + 2.0 * button_padding;
         desired_size = desired_size.at_least(ui.style().spacing.interact_size);
         let rect = ui.allocate_space(desired_size);
 
+        let id = ui.make_position_id();
         let response = ui.interact(rect, id, sense);
         let visuals = ui.style().interact(&response);
         // let text_cursor = response.rect.center() - 0.5 * galley.size; // centered-centered (looks bad for justified drop-down menus
@@ -377,7 +378,6 @@ impl<'a> Widget for Checkbox<'a> {
             text_color,
         } = self;
 
-        let id = ui.make_position_id();
         let text_style = TextStyle::Button;
         let font = &ui.fonts()[text_style];
         let galley = font.layout_single_line(text);
@@ -392,6 +392,7 @@ impl<'a> Widget for Checkbox<'a> {
         desired_size.y = desired_size.y.max(icon_width);
         let rect = ui.allocate_space(desired_size);
 
+        let id = ui.make_position_id();
         let response = ui.interact(rect, id, Sense::click());
         if response.clicked {
             *checked = !*checked;
@@ -463,7 +464,6 @@ impl Widget for RadioButton {
             text,
             text_color,
         } = self;
-        let id = ui.make_position_id();
         let text_style = TextStyle::Button;
         let font = &ui.fonts()[text_style];
         let galley = font.layout_multiline(text, ui.available().width());
@@ -477,6 +477,7 @@ impl Widget for RadioButton {
         desired_size.y = desired_size.y.max(icon_width);
         let rect = ui.allocate_space(desired_size);
 
+        let id = ui.make_position_id();
         let response = ui.interact(rect, id, Sense::click());
 
         let text_cursor = pos2(

egui/src/widgets/slider.rs

@@ -227,9 +227,9 @@ fn x_range(rect: &Rect) -> RangeInclusive<f32> {
 impl<'a> Slider<'a> {
     /// Just the slider, no text
     fn allocate_slide_space(&self, ui: &mut Ui, height: f32) -> Response {
-        let id = ui.make_position_id();
         let desired_size = vec2(ui.style().spacing.slider_width, height);
         let rect = ui.allocate_space(desired_size);
+        let id = ui.make_position_id();
         ui.interact(rect, id, Sense::click_and_drag())
     }
 

egui/src/widgets/text_edit.rs

@@ -105,7 +105,7 @@ impl<'t> Widget for TextEdit<'t> {
 
         let desired_width = desired_width.unwrap_or_else(|| ui.style().spacing.text_edit_width);
 
-        let id = id.unwrap_or_else(|| ui.make_child_id(id_source));
+        let id = id.unwrap_or_else(|| ui.make_persistent_id(id_source));
 
         let mut state = ui.memory().text_edit.get(&id).cloned().unwrap_or_default();