djkato/egui
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Warn about Id clashes for Grid, Plot, ScrollArea, Table (#1452)

Browse source 
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.
This commit is contained in:
Emil Ernerfeldt 2022-04-04 13:13:34 +02:00 committed by GitHub
parent 2d30bd751c
commit c3b6d1bab9
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
7 changed files with 53 additions and 24 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

3
CHANGELOG.md
View file

@ -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)).

5
egui/src/containers/scroll_area.rs
View file

@ -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);

33
egui/src/context.rs
View file

@ -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();

2
egui/src/grid.rs
View file

@ -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(),

5
egui/src/ui.rs
View file

@ -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!
/// });
/// }
/// # });

1
egui/src/widgets/plot/mod.rs
View file

@ -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,

28
egui_extras/src/table.rs
View file

@ -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<egui::Id>) -> Option<Vec<f32>> {
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`].