emilk / egui

egui: an easy-to-use immediate mode GUI in Rust that runs on both web and native
https://www.egui.rs/
Apache License 2.0
20.81k stars 1.5k forks source link

Add `Response::context_menu_with_id` API #4057

Open abey79 opened 4 months ago

abey79 commented 4 months ago

Currently, the context menu code uses the Response's id as key to store its state. Because this id uses egui's auto-increment mechanism, this can lead to unexpected behaviour when the clicked widget has an unstable id (for example because the previous content changes on hover).

See this comment for a real-life example (this particular case is more complicated, but the root cause was widget id instability triggering the wrong widget call the context menu closure): https://github.com/rerun-io/rerun/pull/5205#issuecomment-1947935840

To work around this class of issue, it would be helpful to have a Response::context_menu_with_id to let the user code provide a (potentially more stable) id for the context menu (similar to how collapsing header requires an id). The current Response::context_menu() API could then be implemented based on the new one, passing the Response id as fallback.

YgorSouza commented 4 months ago

Maybe add a with_new_id method to match with_new_rect? Then it could be used for other things if needed.

Edit: never mind, it doesn't work if we change the response's id.

abey79 commented 4 months ago

Maybe add a with_new_id method to match with_new_rect? Then it could be used for other things if needed.

Edit: never mind, it doesn't work if we change the response's id.

It sounds to me like this would actually work. Why don't you think so?

YgorSouza commented 4 months ago

I don't know why it doesn't work, since I'm not familiar with the context menu code or how egui handles IDs in general. I just had a suspicion after writing my comment that maybe it wasn't as simple as that, so I decided to test it, and it didn't work:

--- a/examples/hello_world/src/main.rs
+++ b/examples/hello_world/src/main.rs
@@ -47,7 +47,11 @@ impl eframe::App for MyApp {
             if ui.button("Increment").clicked() {
                 self.age += 1;
             }
-            ui.label(format!("Hello '{}', age {}", self.name, self.age));
+            let mut res = ui.label(format!("Hello '{}', age {}", self.name, self.age));
+            res.id = "some unique ID".into();
+            res.context_menu(|ui| {
+                ui.label("Menu");
+            });

             ui.image(egui::include_image!(
                 "../../../crates/egui/assets/ferris.png"
YgorSouza commented 4 months ago

Ok, it does seem to work, actually. The problem is that I was testing with a label, and the label loses its selectable state when you change its response's id, for some reason, so it no longer senses click and can't open the menu. But if I try with something else that explicitly senses clicks, it works.