Skip to content

Commit

Permalink
Do not deselect on ESC when it was used to close some other UI element (
Browse files Browse the repository at this point in the history
#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:
- emilk/egui#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`.
  • Loading branch information
abey79 committed Sep 30, 2024
1 parent edf9530 commit 6bfb5e8
Show file tree
Hide file tree
Showing 4 changed files with 59 additions and 6 deletions.
2 changes: 1 addition & 1 deletion crates/viewer/re_context_menu/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
3 changes: 2 additions & 1 deletion crates/viewer/re_ui/src/modal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,8 @@ impl Modal {
) -> ModalResponse<R> {
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);
Expand Down
12 changes: 8 additions & 4 deletions crates/viewer/re_viewer/src/app_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -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;
}
Expand Down
48 changes: 48 additions & 0 deletions tests/python/release_checklist/check_deselect_on_escape.py
Original file line number Diff line number Diff line change
@@ -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)

0 comments on commit 6bfb5e8

Please sign in to comment.