From 6bfb5e8b323a4034c5b3bd252896ae6de98b7658 Mon Sep 17 00:00:00 2001 From: Antoine Beyeler <49431240+abey79@users.noreply.github.com> Date: Mon, 30 Sep 2024 16:43:52 +0200 Subject: [PATCH] Do not deselect on ESC when it was used to close some other UI element (#7548) ### What This PR attempt to make the "deselect-on-ESC" less aggressive and avoid triggering it when the ESC keystroke was used to close some other UI element. Work around for: - https://github.com/emilk/egui/issues/5189 #### Supported - any popup menu (including color picker) - context menu - modals #### TODO - rerun menu (haven't found an easy way yet) #### Not supported - tooltip (on purpose, these disappear by themselves) ### Checklist * [x] I have read and agree to [Contributor Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and the [Code of Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md) * [x] I've included a screenshot or gif (if applicable) * [x] I have tested the web demo (if applicable): * Using examples from latest `main` build: [rerun.io/viewer](https://rerun.io/viewer/pr/7548?manifest_url=https://app.rerun.io/version/main/examples_manifest.json) * Using full set of examples from `nightly` build: [rerun.io/viewer](https://rerun.io/viewer/pr/7548?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json) * [x] The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG * [x] If applicable, add a new check to the [release checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)! * [x] If have noted any breaking changes to the log API in `CHANGELOG.md` and the migration guide - [PR Build Summary](https://build.rerun.io/pr/7548) - [Recent benchmark results](https://build.rerun.io/graphs/crates.html) - [Wasm size tracking](https://build.rerun.io/graphs/sizes.html) To run all checks from `main`, comment on the PR with `@rerun-bot full-check`. --- crates/viewer/re_context_menu/src/lib.rs | 2 +- crates/viewer/re_ui/src/modal.rs | 3 +- crates/viewer/re_viewer/src/app_state.rs | 12 +++-- .../check_deselect_on_escape.py | 48 +++++++++++++++++++ 4 files changed, 59 insertions(+), 6 deletions(-) create mode 100644 tests/python/release_checklist/check_deselect_on_escape.py diff --git a/crates/viewer/re_context_menu/src/lib.rs b/crates/viewer/re_context_menu/src/lib.rs index 2ca4209c9f4b..603d503b93cc 100644 --- a/crates/viewer/re_context_menu/src/lib.rs +++ b/crates/viewer/re_context_menu/src/lib.rs @@ -43,7 +43,7 @@ pub fn context_menu_ui_for_item( selection_update_behavior: SelectionUpdateBehavior, ) { item_response.context_menu(|ui| { - if ui.input(|i| i.key_pressed(egui::Key::Escape)) { + if ui.input_mut(|i| i.consume_key(egui::Modifiers::NONE, egui::Key::Escape)) { ui.close_menu(); return; } diff --git a/crates/viewer/re_ui/src/modal.rs b/crates/viewer/re_ui/src/modal.rs index b2cc9d03820c..ba8048e15a01 100644 --- a/crates/viewer/re_ui/src/modal.rs +++ b/crates/viewer/re_ui/src/modal.rs @@ -164,7 +164,8 @@ impl Modal { ) -> ModalResponse { Self::dim_background(ctx); - let mut open = ctx.input(|i| !i.key_pressed(egui::Key::Escape)); + // We consume such as to avoid the top-level deselect-on-ESC behavior. + let mut open = ctx.input_mut(|i| !i.consume_key(egui::Modifiers::NONE, egui::Key::Escape)); let screen_height = ctx.screen_rect().height(); let modal_vertical_margins = (75.0).at_most(screen_height * 0.1); diff --git a/crates/viewer/re_viewer/src/app_state.rs b/crates/viewer/re_viewer/src/app_state.rs index 4c39a8ae9951..7e85957a0cbc 100644 --- a/crates/viewer/re_viewer/src/app_state.rs +++ b/crates/viewer/re_viewer/src/app_state.rs @@ -157,6 +157,9 @@ impl AppState { focused_item, } = self; + // check state early, before the UI has a chance to close these popups + let is_any_popup_open = ui.memory(|m| m.any_popup_open()); + // Some of the mutations APIs of `ViewportBlueprints` are recorded as `Viewport::TreeAction` // and must be applied by `Viewport` at the end of the frame. We use a temporary channel for // this, which gives us interior mutability (only a shared reference of `ViewportBlueprint` @@ -202,10 +205,6 @@ impl AppState { )), ); - if ui.input(|i| i.key_pressed(egui::Key::Escape)) { - selection_state.clear_selection(); - } - let applicable_entities_per_visualizer = space_view_class_registry .applicable_entities_for_visualizer_systems(recording.store_id()); let indicated_entities_per_visualizer = @@ -470,6 +469,11 @@ impl AppState { // This must run after any ui code, or other code that tells egui to open an url: check_for_clicked_hyperlinks(&egui_ctx, ctx.selection_state); + // Deselect on ESC. Must happen after all other UI code to let them capture ESC if needed. + if ui.input(|i| i.key_pressed(egui::Key::Escape)) && !is_any_popup_open { + selection_state.clear_selection(); + } + // Reset the focused item. *focused_item = None; } diff --git a/tests/python/release_checklist/check_deselect_on_escape.py b/tests/python/release_checklist/check_deselect_on_escape.py new file mode 100644 index 000000000000..fcbb5ca1603e --- /dev/null +++ b/tests/python/release_checklist/check_deselect_on_escape.py @@ -0,0 +1,48 @@ +from __future__ import annotations + +import os +from argparse import Namespace +from uuid import uuid4 + +import rerun as rr + +README = """\ +# Deselect on Escape + +This checks that the deselect-on-escape works as expected. + +### Actions + +* Base behavior: select the 3D view and hit ESC => the view is no longer selected. + +In all the following cases, the 3D view should *remain selected*. + +* Select the 3D view, open the "Background Kind" dropdown in the selection panel, and hit ESC. +* Select the 3D view, open the "Add space view or container" modal, and hit ESC. +* Select the 3D view, right-click on it in the blueprint tree, and hit ESC. +* Select the 3D view, open the Rerun menu, and hit ESC (KNOWN FAIL). +""" + + +def log_readme() -> None: + rr.log("readme", rr.TextDocument(README, media_type=rr.MediaType.MARKDOWN), timeless=True) + + +def log_some_data() -> None: + rr.log("data", rr.Points3D([0, 0, 0])) + + +def run(args: Namespace) -> None: + rr.script_setup(args, f"{os.path.basename(__file__)}", recording_id=uuid4()) + + log_readme() + log_some_data() + + +if __name__ == "__main__": + import argparse + + parser = argparse.ArgumentParser(description="Interactive release checklist") + rr.script_add_args(parser) + args = parser.parse_args() + run(args)