From c3b6d1bab93c4771bf2715e197ac80fddfc2a77f Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Mon, 4 Apr 2022 13:13:34 +0200 Subject: [PATCH] Warn about Id clashes for Grid, Plot, ScrollArea, Table (#1452) Id clashes can cause subtle bugs. egui already warns when the same Id is used to interact with different parts of the screen. This adds warnings about id clashes for some widgets that store state: Grid, Plot, ScrollArea, Table. The PR also adds `Context::check_for_id_clash` so users who create their own widgets can add the same type of check. --- CHANGELOG.md | 3 ++- egui/src/containers/scroll_area.rs | 5 +++++ egui/src/context.rs | 33 ++++++++++++++++++++++-------- egui/src/grid.rs | 2 ++ egui/src/ui.rs | 5 +++-- egui/src/widgets/plot/mod.rs | 1 + egui_extras/src/table.rs | 28 +++++++++++++------------ 7 files changed, 53 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e527649b85..f2306c7c8fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,13 +11,14 @@ NOTE: [`epaint`](epaint/CHANGELOG.md), [`eframe`](eframe/CHANGELOG.md), [`egui_w * Added `Frame::canvas` ([#1362](https://github.com/emilk/egui/pull/1362)). * `Context::request_repaint` will wake up UI thread, if integrations has called `Context::set_request_repaint_callback` ([#1366](https://github.com/emilk/egui/pull/1366)). * Added `Plot::allow_scroll`, `Plot::allow_zoom` no longer affects scrolling ([#1382](https://github.com/emilk/egui/pull/1382)). -* Added `Ui::push_id` ([#1374](https://github.com/emilk/egui/pull/1374)). +* Added `Ui::push_id` to resolve id clashes ([#1374](https://github.com/emilk/egui/pull/1374)). * Added `Frame::outer_margin`. ### Changed 🔧 * `ClippedMesh` has been replaced with `ClippedPrimitive` ([#1351](https://github.com/emilk/egui/pull/1351)). * Renamed `Frame::margin` to `Frame::inner_margin`. * Renamed `AlphaImage` to `FontImage` to discourage any other use for it ([#1412](https://github.com/emilk/egui/pull/1412)). +* Warnings will pe painted on screen when there is an `Id` clash for `Grid`, `Plot` or `ScrollArea` ([#1452](https://github.com/emilk/egui/pull/1452)). ### Fixed 🐛 * Fixed ComboBoxes always being rendered left-aligned ([#1304](https://github.com/emilk/egui/pull/1304)). diff --git a/egui/src/containers/scroll_area.rs b/egui/src/containers/scroll_area.rs index 8b6ab960134..7bd5576e7a6 100644 --- a/egui/src/containers/scroll_area.rs +++ b/egui/src/containers/scroll_area.rs @@ -325,6 +325,11 @@ impl ScrollArea { let id_source = id_source.unwrap_or_else(|| Id::new("scroll_area")); let id = ui.make_persistent_id(id_source); + ui.ctx().check_for_id_clash( + id, + Rect::from_min_size(ui.available_rect_before_wrap().min, Vec2::ZERO), + "ScrollArea", + ); let mut state = State::load(&ctx, id).unwrap_or_default(); state.offset.x = offset_x.unwrap_or(state.offset.x); diff --git a/egui/src/context.rs b/egui/src/context.rs index 8f3abf3fd52..e0212496b65 100644 --- a/egui/src/context.rs +++ b/egui/src/context.rs @@ -223,9 +223,16 @@ impl Context { // --------------------------------------------------------------------- - /// If the given [`Id`] is not unique, an error will be printed at the given position. - /// Call this for [`Id`]:s that need interaction or persistence. - pub(crate) fn register_interaction_id(&self, id: Id, new_rect: Rect) { + /// If the given [`Id`] has been used previously the same frame at at different position, + /// then an error will be printed on screen. + /// + /// This function is already called for all widgets that do any interaction, + /// but you can call this from widgets that store state but that does not interact. + /// + /// The given [`Rect`] should be approximately where the widget will be. + /// The most important thing is that [`Rect::min`] is approximately correct, + /// because that's where the warning will be painted. If you don't know what size to pick, just pick [`Vec2::ZERO`]. + pub fn check_for_id_clash(&self, id: Id, new_rect: Rect, what: &str) { let prev_rect = self.frame_state().used_ids.insert(id, new_rect); if let Some(prev_rect) = prev_rect { // it is ok to reuse the same ID for e.g. a frame around a widget, @@ -244,7 +251,8 @@ impl Context { painter.error( rect.left_bottom() + vec2(2.0, 4.0), "ID clashes happens when things like Windows or CollapsingHeaders share names,\n\ - or when things like ScrollAreas and Resize areas aren't given unique id_source:s.", + or when things like Plot and Grid:s aren't given unique id_source:s.\n\n\ + Sometimes the solution is to use ui.push_id.", ); } } @@ -253,10 +261,19 @@ impl Context { let id_str = id.short_debug_format(); if prev_rect.min.distance(new_rect.min) < 4.0 { - show_error(new_rect.min, format!("Double use of ID {}", id_str)); + show_error( + new_rect.min, + format!("Double use of {} ID {}", what, id_str), + ); } else { - show_error(prev_rect.min, format!("First use of ID {}", id_str)); - show_error(new_rect.min, format!("Second use of ID {}", id_str)); + show_error( + prev_rect.min, + format!("First use of {} ID {}", what, id_str), + ); + show_error( + new_rect.min, + format!("Second use of {} ID {}", what, id_str), + ); } } } @@ -322,7 +339,7 @@ impl Context { return response; } - self.register_interaction_id(id, rect); + self.check_for_id_clash(id, rect, "widget"); let clicked_elsewhere = response.clicked_elsewhere(); let ctx_impl = &mut *self.write(); diff --git a/egui/src/grid.rs b/egui/src/grid.rs index 7b6267346ee..780164803f1 100644 --- a/egui/src/grid.rs +++ b/egui/src/grid.rs @@ -80,6 +80,8 @@ impl GridLayout { "Grid not yet available for right-to-left layouts" ); + ui.ctx().check_for_id_clash(id, initial_available, "Grid"); + Self { ctx: ui.ctx().clone(), style: ui.style().clone(), diff --git a/egui/src/ui.rs b/egui/src/ui.rs index 5bd94d3a069..874cc4160c2 100644 --- a/egui/src/ui.rs +++ b/egui/src/ui.rs @@ -1566,9 +1566,10 @@ impl Ui { /// ``` /// # egui::__run_test_ui(|ui| { /// for i in 0..10 { - /// // `ui.make_persistent_id("foo")` here will produce the same id each loop. + /// // ui.collapsing("Same header", |ui| { }); // this will cause an ID clash because of the same title! + /// /// ui.push_id(i, |ui| { - /// // `ui.make_persistent_id("foo")` here will produce different id:s + /// ui.collapsing("Same header", |ui| { }); // this is fine! /// }); /// } /// # }); diff --git a/egui/src/widgets/plot/mod.rs b/egui/src/widgets/plot/mod.rs index e6183bac75d..15a8ce35cb2 100644 --- a/egui/src/widgets/plot/mod.rs +++ b/egui/src/widgets/plot/mod.rs @@ -494,6 +494,7 @@ impl Plot { // Load or initialize the memory. let plot_id = ui.make_persistent_id(id_source); + ui.ctx().check_for_id_clash(plot_id, rect, "Plot"); let mut memory = PlotMemory::load(ui.ctx(), plot_id).unwrap_or_else(|| PlotMemory { auto_bounds: !min_auto_bounds.is_valid(), hovered_entry: None, diff --git a/egui_extras/src/table.rs b/egui_extras/src/table.rs index 6d7b8ec6f53..caa5e13d451 100644 --- a/egui_extras/src/table.rs +++ b/egui_extras/src/table.rs @@ -9,7 +9,7 @@ use crate::{ Size, StripLayout, }; -use egui::{Response, Ui}; +use egui::{Rect, Response, Ui, Vec2}; /// Builder for a [`Table`] with (optional) fixed header and scrolling body. /// @@ -139,12 +139,8 @@ impl<'a> TableBuilder<'a> { } = self; let resize_id = resizable.then(|| ui.id().with("__table_resize")); - let widths = if let Some(resize_id) = resize_id { - ui.data().get_persisted(resize_id) - } else { - None - }; - let widths = widths + + let widths = read_table_widths(ui, resize_id) .unwrap_or_else(|| sizing.to_lengths(available_width, ui.spacing().item_spacing.x)); let table_top = ui.cursor().top(); @@ -190,12 +186,8 @@ impl<'a> TableBuilder<'a> { } = self; let resize_id = resizable.then(|| ui.id().with("__table_resize")); - let widths = if let Some(resize_id) = resize_id { - ui.data().get_persisted(resize_id) - } else { - None - }; - let widths = widths + + let widths = read_table_widths(ui, resize_id) .unwrap_or_else(|| sizing.to_lengths(available_width, ui.spacing().item_spacing.x)); let table_top = ui.cursor().top(); @@ -215,6 +207,16 @@ impl<'a> TableBuilder<'a> { } } +fn read_table_widths(ui: &egui::Ui, resize_id: Option) -> Option> { + if let Some(resize_id) = resize_id { + let rect = Rect::from_min_size(ui.available_rect_before_wrap().min, Vec2::ZERO); + ui.ctx().check_for_id_clash(resize_id, rect, "Table"); + ui.data().get_persisted(resize_id) + } else { + None + } +} + /// Table struct which can construct a [`TableBody`]. /// /// Is created by [`TableBuilder`] by either calling [`TableBuilder::body`] or after creating a header row with [`TableBuilder::header`].