Shortcuts run in the order pressed; Shift+Delete fills on Windows; colour pickers stay put while dragging (#440, #530, #534) (#552)

* Shortcuts pressed in one frame run in the order they were pressed (#440)

Undo then Save in the same frame dropped the Undo: only the first binding in table order ran. Each key press now dispatches in event order, and a shortcut that opens a dialog, prompt, menu or inline type leaves the frame's later presses to it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Shift+Delete opens Edit › Fill on Windows instead of cutting (#530)

egui-winit reports Shift+Delete as a Cut on Windows. Outside text fields, a Cut with Shift and without Ctrl is now the Shift+Delete press again, so it fills like Shift+Backspace; Ctrl+X still cuts and text fields keep their Cut.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Shape and text colour pickers stay put while dragging (#534)

The picker's width follows its RGB readouts, and egui placed the popup anew every frame: near the right edge of the window it flipped between opening right and left of the swatch, under the pointer. A swatch's picker now keeps the left edge it opened with, shifted left of the swatch when the window edge needs it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
BFlat
2026-10-07 13:54:31 +07:00
committed by GitHub
co-authored by Claude Opus 5.5
parent e30b121969
commit 13b0d856b6
6 changed files with 264 additions and 32 deletions
+37 -15
View File
@@ -11,7 +11,7 @@ use egui::{Key, KeyboardShortcut};
use serde_json::json;
use crate::PhotocraftApp;
use crate::shortcuts::{consume, parse};
use crate::shortcuts::{key_matches, parse};
/// What a dispatched shortcut did (kept for tests and `ui.inspect`-style debugging).
#[derive(Clone, Debug, PartialEq)]
@@ -156,25 +156,47 @@ impl Focus {
}
}
/// The command whose shortcut was pressed this frame (its key press consumed), if any.
/// While typing (inline type), clipboard and select-all shortcuts belong to the text.
pub fn pressed_command(app: &PhotocraftApp, ctx: &egui::Context, focus: Focus, editing: bool) -> Option<String> {
/// What decides which keys [`crate::shortcuts::handle`] gives the shortcuts: an open menu, a
/// dialog, the unsaved-changes prompt, a warp or the Filter Gallery, inline type.
fn key_owner(app: &PhotocraftApp, ctx: &egui::Context) -> (bool, usize, bool, bool, bool) {
let distort = app.distort.active() || app.distort.gallery.is_some();
(crate::menu_nav::is_open(ctx), app.ui.dialogs.len(), app.discard.is_some(), distort, app.ui.text_edit.is_some())
}
/// Dispatch this frame's shortcut presses in the order they arrived (⌘Z then ⌘S undoes, then
/// saves the undone state; #440), consuming each. A press matching several bindings goes to the
/// first in table order (⇧⌘Z before ⌘Z). Stops after a shortcut that hands the keyboard to
/// someone else (opens a dialog, a prompt, a menu or inline type), leaving the later presses to
/// it. While typing (inline type), clipboard and select-all shortcuts belong to the text.
/// Returns true when a shortcut was dispatched.
pub fn dispatch_pressed(app: &mut PhotocraftApp, ctx: &egui::Context, focus: Focus, editing: bool) -> bool {
// Building the table walks the registry: only do it when a key went down.
if !ctx.input(|i| i.events.iter().any(|e| matches!(e, egui::Event::Key { pressed: true, .. }))) {
return None;
return false;
}
for (id, sc) in bindings(app) {
if !focus.allows(&sc) {
continue;
}
if editing && matches!(id.as_str(), "edit.copy" | "edit.cut" | "edit.paste" | "edit.copyMerged" | "select.all" | "edit.pasteSpecial.pasteInPlace") {
continue;
}
if consume(ctx, &sc) {
return Some(id);
const TEXT_OWNED: [&str; 6] = ["edit.copy", "edit.cut", "edit.paste", "edit.copyMerged", "select.all", "edit.pasteSpecial.pasteInPlace"];
let table: Vec<(String, KeyboardShortcut)> =
bindings(app).into_iter().filter(|(id, sc)| focus.allows(sc) && !(editing && TEXT_OWNED.contains(&id.as_str()))).collect();
let command = |e: &egui::Event| match e {
egui::Event::Key { key, pressed: true, modifiers, .. } => table.iter().find(|(_, sc)| key_matches(sc, *key, *modifiers)).map(|(id, _)| id.clone()),
_ => None,
};
let mut ran = false;
loop {
let next = ctx.input_mut(|i| {
let (at, id) = i.events.iter().enumerate().find_map(|(at, e)| Some((at, command(e)?)))?;
i.events.remove(at);
Some(id)
});
let Some(id) = next else { break };
let owner = key_owner(app, ctx);
dispatch(app, ctx, &id);
ran = true;
if key_owner(app, ctx) != owner {
break;
}
}
None
ran
}
/// Why `id` can't run now (the engine's reason when it has one).
@@ -312,3 +312,86 @@ fn alt_brackets_walk_the_layers_panel() {
assert_eq!(active_name(&h), "Hello");
assert_eq!(h.state().session.active().unwrap().selected_layers().len(), 2, "⇧⌥[ adds to the selection");
}
/// Bytes the app saved, newest last.
type Saved = std::rc::Rc<std::cell::RefCell<Vec<Vec<u8>>>>;
/// [`harness`] with a document that saves in place, an exporter writing its top-level layer count
/// and a writer recording it; then one new layer on top (history to undo).
fn saving_harness() -> (Harness<'static, PhotocraftApp>, Saved) {
let saved = Saved::default();
let out = saved.clone();
let mut h = Harness::builder().with_size(vec2(1440.0, 900.0)).with_max_steps(64).build_eframe(move |cc| {
PhotocraftApp::setup_context(&cc.egui_ctx, Default::default());
let services = crate::Services {
export: Some(Box::new(|doc, _, _| Ok((doc.layers.len().to_string().into_bytes(), Vec::new())))),
write: Some(Box::new(move |_, bytes| {
out.borrow_mut().push(bytes.to_vec());
Ok(())
})),
..Default::default()
};
let mut session = realistic();
session.active_mut().unwrap().path = Some("/tmp/layout.psd".into());
session.execute("layer.new.layer", json!({"name": "added"})).unwrap();
PhotocraftApp::new(session, services)
});
h.run_steps(8);
put_focus(&mut h, Place::Canvas);
(h, saved)
}
fn layer_count(h: &Harness<'_, PhotocraftApp>) -> usize {
h.state().session.active().unwrap().doc.layers.len()
}
/// Press `shortcuts` one after another within a single frame (`Harness::event` runs a frame per
/// event).
fn press_in_one_frame(h: &mut Harness<'_, PhotocraftApp>, shortcuts: &[&str]) {
let events = &mut h.input_mut().events;
for sc in shortcuts {
let sc = parse(sc).unwrap();
let m = platform(sc.modifiers);
events.push(egui::Event::ModifiersChanged(m));
events.push(egui::Event::Key { key: sc.logical_key, physical_key: None, pressed: true, repeat: false, modifiers: m });
events.push(egui::Event::Key { key: sc.logical_key, physical_key: None, pressed: false, repeat: false, modifiers: m });
}
events.push(egui::Event::ModifiersChanged(Modifiers::NONE));
h.run_steps(2);
}
fn logged(h: &Harness<'_, PhotocraftApp>) -> Vec<String> {
take_log(&h.ctx).into_iter().map(|(id, _)| id).collect()
}
/// #440: shortcuts arriving in one frame run in the order they were pressed.
#[test]
fn shortcuts_in_one_frame_run_in_order() {
let (mut h, saved) = saving_harness();
assert_eq!(layer_count(&h), 5);
// ⌘Z then ⌘S: the undone document is saved.
press_in_one_frame(&mut h, &["Cmd+Z", "Cmd+S"]);
assert_eq!(layer_count(&h), 4, "⌘Z undid the new layer");
assert_eq!(saved.borrow().as_slice(), [b"4".to_vec()], "⌘S saved after the undo");
assert_eq!(logged(&h), ["edit.undo", "file.save"]);
// ⇧⌘Z redoes; then ⌘S before ⌘Z saves first and undoes after.
press_in_one_frame(&mut h, &["Cmd+Shift+Z"]);
assert_eq!(layer_count(&h), 5);
logged(&h);
saved.borrow_mut().clear();
press_in_one_frame(&mut h, &["Cmd+S", "Cmd+Z"]);
assert_eq!(saved.borrow().as_slice(), [b"5".to_vec()], "saved before the undo");
assert_eq!(layer_count(&h), 4, "then undone");
assert_eq!(logged(&h), ["file.save", "edit.undo"]);
}
/// A shortcut that opens a dialog leaves the frame's later keys to it: ⌘L then ⌘Z doesn't undo
/// behind the Levels dialog.
#[test]
fn a_shortcut_opening_a_dialog_takes_the_frames_later_keys() {
let (mut h, _) = saving_harness();
press_in_one_frame(&mut h, &["Cmd+L", "Cmd+Z"]);
assert_eq!(h.state().ui.dialogs.len(), 1, "⌘L opened Levels");
assert_eq!(layer_count(&h), 5, "⌘Z didn't undo behind the dialog");
assert_eq!(logged(&h), ["image.adjustments.levels"]);
}
+50 -14
View File
@@ -155,6 +155,20 @@ fn nav_keys(app: &mut PhotocraftApp, ctx: &egui::Context) -> bool {
false
}
/// The key press a clipboard event stands for outside text fields, with `mods` held: Copy, Cut
/// and Paste are ⌘C, ⌘X and ⌘V (with ⇧/⌥ as held). On Windows egui-winit also sends Cut for
/// Shift+Delete, which is Edit › Fill here (#530): a Cut with ⇧ and without Ctrl is that key.
fn clipboard_press(e: &egui::Event, mods: Modifiers, is_windows: bool) -> Option<(Key, Modifiers)> {
let held = if mods.command { mods } else { Modifiers::COMMAND };
match e {
egui::Event::Copy => Some((Key::C, held)),
egui::Event::Cut if is_windows && mods.shift && !mods.command => Some((Key::Delete, mods)),
egui::Event::Cut => Some((Key::X, held)),
egui::Event::Paste(_) => Some((Key::V, held)),
_ => None,
}
}
/// egui-winit turns ⌘C / ⌘X / ⌘V into `Event::Copy` / `Cut` / `Paste` and drops the key press, and
/// drops ⌘V entirely when the clipboard holds no text (an image). Outside text fields, give the
/// pixel commands their key presses back: Copy/Cut/Paste become ⌘C/⌘X/⌘V presses (with ⇧/⌥ as
@@ -191,14 +205,7 @@ pub fn clipboard_keys(ctx: &egui::Context, typing: bool, raw: &mut egui::RawInpu
if let egui::Event::ModifiersChanged(m) | egui::Event::Key { modifiers: m, .. } = &e {
mods = *m;
}
let held = if mods.command { mods } else { Modifiers::COMMAND };
let key = match &e {
egui::Event::Copy => Some(Key::C),
egui::Event::Cut => Some(Key::X),
egui::Event::Paste(_) => Some(Key::V),
_ => None,
};
if let Some(key) = key {
if let Some((key, held)) = clipboard_press(&e, mods, cfg!(target_os = "windows")) {
if key == Key::V {
ctx.data_mut(|d| d.insert_temp(seen, true));
}
@@ -231,7 +238,7 @@ pub fn handle(app: &mut PhotocraftApp, ctx: &egui::Context) {
if app.distort.liquify.is_some() && !ctx.text_edit_focused() {
crate::liquify_ui::keys(app, ctx);
}
use crate::shortcut_dispatch::{Focus, dispatch, pressed_command};
use crate::shortcut_dispatch::{Focus, dispatch_pressed};
let focus = Focus::of(ctx);
// Dialogs (and Liquify's panel while one of its controls has focus) keep canvas zoom; a
// focused text field keeps its keys.
@@ -245,9 +252,7 @@ pub fn handle(app: &mut PhotocraftApp, ctx: &egui::Context) {
if focus == Focus::Text {
// A focused field keeps its typing and editing keys; menu shortcuts (⌘J, ⌘S, F7…) still
// fire, as in Photoshop.
if let Some(id) = pressed_command(app, ctx, focus, false) {
dispatch(app, ctx, &id);
}
dispatch_pressed(app, ctx, focus, false);
return;
}
// Liquify / Puppet Warp / Perspective Warp: ↩ commits, Esc cancels.
@@ -283,8 +288,7 @@ pub fn handle(app: &mut PhotocraftApp, ctx: &egui::Context) {
// Inline type editing eats text and navigation keys; ⌘-shortcuts still reach the menus.
let editing = crate::type_tool::handle_keys(app, ctx);
// Registry, UI and menu-catalogue shortcuts (see [`crate::shortcut_dispatch::bindings`]).
if let Some(id) = pressed_command(app, ctx, focus, editing) {
dispatch(app, ctx, &id);
if dispatch_pressed(app, ctx, focus, editing) {
return;
}
if editing {
@@ -465,6 +469,38 @@ mod tests {
assert!(matches!(r.events.as_slice(), [_, egui::Event::Paste(_)]));
}
/// #530: egui-winit sends Cut for Shift+Delete on Windows; on the canvas it opens Fill.
#[test]
fn windows_shift_delete_fills_instead_of_cutting() {
let shift = Modifiers::SHIFT;
let ctrl = Modifiers::CTRL | Modifiers::COMMAND;
let app = PhotocraftApp::new(photocraft_engine::Session::new(), Default::default());
let bound = |held| {
let (key, mods) = clipboard_press(&egui::Event::Cut, held, true)?;
assert_eq!((key, mods), (Key::Delete, held));
crate::shortcut_dispatch::bindings(&app).into_iter().find(|(_, sc)| key_matches(sc, key, mods)).map(|(id, _)| id)
};
assert_eq!(bound(shift).as_deref(), Some("edit.fill"));
// ⇧⌥Delete keeps ⌥: fill with the foreground, keeping transparency.
assert_eq!(bound(shift | Modifiers::ALT).as_deref(), Some("edit.fillForegroundPreserve"));
// Ctrl+X (and Ctrl+Shift+X) still cut; elsewhere a Cut is always ⌘X.
assert_eq!(clipboard_press(&egui::Event::Cut, ctrl, true), Some((Key::X, ctrl)));
assert_eq!(clipboard_press(&egui::Event::Cut, ctrl | shift, true), Some((Key::X, ctrl | shift)));
assert_eq!(clipboard_press(&egui::Event::Cut, shift, false), Some((Key::X, Modifiers::COMMAND)));
// Shift+Insert pastes on Windows.
assert_eq!(clipboard_press(&egui::Event::Paste("x".into()), shift, true), Some((Key::V, Modifiers::COMMAND)));
// Through the raw-input hook on this platform; a text field keeps its Cut.
let ctx = egui::Context::default();
let cut = || egui::RawInput { events: vec![egui::Event::ModifiersChanged(shift), egui::Event::Cut], ..Default::default() };
let mut r = cut();
clipboard_keys(&ctx, false, &mut r);
let want = if cfg!(target_os = "windows") { Key::Delete } else { Key::X };
assert!(matches!(r.events.as_slice(), [_, egui::Event::Key { key, pressed: true, .. }] if *key == want), "{:?}", r.events);
let mut r = cut();
clipboard_keys(&ctx, true, &mut r);
assert!(matches!(r.events.as_slice(), [_, egui::Event::Cut]));
}
#[cfg(target_os = "linux")]
#[test]
fn ctrl_and_shift_insert_copy_and_paste_on_linux() {
+2 -2
View File
@@ -847,7 +847,7 @@ pub fn options_bar(app: &mut PhotocraftApp, ui: &mut egui::Ui) {
ui.painter().rect_filled(rect, 2.0, Color32::from_rgb(q(c[0]), q(c[1]), q(c[2])));
ui.painter().rect_stroke(rect, 2.0, Stroke::new(1.0, t.field_border), egui::StrokeKind::Outside);
let resp = resp.on_hover_text(tl!("Set the text color"));
egui::Popup::from_toggle_button_response(&resp).close_behavior(egui::PopupCloseBehavior::CloseOnClickOutside).show(|ui| {
crate::widgets::swatch_popup(&resp).show(|ui| {
let mut col = Color32::from_rgb(q(c[0]), q(c[1]), q(c[2]));
if egui::color_picker::color_picker_color32(ui, &mut col, egui::color_picker::Alpha::Opaque) {
apply(app, ui.ctx(), json!({"color": format!("#{:02x}{:02x}{:02x}", col.r(), col.g(), col.b())}));
@@ -1152,7 +1152,7 @@ fn type_sections(app: &mut PhotocraftApp, ui: &mut egui::Ui, character: bool, pa
ui.painter().rect_filled(rect, t.radius_sm, Color32::from_rgb(q(rgb[0]), q(rgb[1]), q(rgb[2])));
ui.painter().rect_stroke(rect, t.radius_sm, Stroke::new(1.0, t.field_border), egui::StrokeKind::Inside);
let resp = resp.on_hover_text(tl!("Text color"));
egui::Popup::from_toggle_button_response(&resp).close_behavior(egui::PopupCloseBehavior::CloseOnClickOutside).show(|ui| {
crate::widgets::swatch_popup(&resp).show(|ui| {
let mut col = Color32::from_rgb(q(rgb[0]), q(rgb[1]), q(rgb[2]));
if egui::color_picker::color_picker_color32(ui, &mut col, egui::color_picker::Alpha::Opaque) {
apply(app, ui.ctx(), json!({"color": format!("#{:02x}{:02x}{:02x}", col.r(), col.g(), col.b())}));
+62 -1
View File
@@ -495,7 +495,7 @@ fn swatch(ui: &mut egui::Ui, fill: Option<&photocraft_doc::Fill>, tip: &str) ->
ui.painter().rect_stroke(r, 2.0, Stroke::new(1.0, t.field_border), egui::StrokeKind::Outside);
let resp = resp.on_hover_text(tip);
let mut out = None;
egui::Popup::from_toggle_button_response(&resp).close_behavior(egui::PopupCloseBehavior::CloseOnClickOutside).show(|ui| {
crate::widgets::swatch_popup(&resp).show(|ui| {
let mut c = current.unwrap_or(Color32::BLACK);
if egui::color_picker::color_picker_color32(ui, &mut c, egui::color_picker::Alpha::Opaque) {
out = Some(format!("#{:02x}{:02x}{:02x}", c.r(), c.g(), c.b()));
@@ -846,6 +846,67 @@ mod tests {
let wp = app.session.active().unwrap().doc.work_path.clone().unwrap();
assert_eq!((wp.subpaths[0].knots[0].anchor.x, wp.subpaths[0].knots[0].anchor.y), (30.0, 25.0));
}
/// #534: dragging in a shape's fill picker, opened from the Properties panel at the right edge
/// of the window, keeps the picker in place (it flipped from side to side as its width
/// followed the colour readouts, which moved it under the pointer).
#[test]
fn shape_colour_picker_stays_put_while_dragging() {
use egui::{PointerButton, vec2};
use egui_kittest::kittest::Queryable;
let mut app = app();
let id = app.run("shape.create", json!({"kind": "rect", "rect": [10, 10, 100, 60], "fill": "#3070c0"})).unwrap()["layer"].as_u64().unwrap();
let mut h = egui_kittest::Harness::builder().with_size(vec2(1440.0, 900.0)).build_ui_state(
move |ui, app: &mut PhotocraftApp| {
// Fonts set up after the first frame only apply from the next one.
if !ui.ctx().fonts(|f| f.families().contains(&egui::FontFamily::Name("medium".into()))) {
return;
}
egui::Panel::right("properties").resizable(false).exact_size(285.0).show(ui, |ui| shape_properties(app, ui, photocraft_doc::LayerId(id)));
},
app,
);
PhotocraftApp::setup_context(&h.ctx, crate::theme::ThemeKind::Pro);
h.run_steps(4);
let label = h.query_all(egui_kittest::kittest::by().label("Fill").include_labels()).next().expect("the Fill label").rect();
// The swatch is right of its label.
let swatch = egui::pos2(label.right() + 17.0, label.center().y);
let press = |h: &mut egui_kittest::Harness<'_, PhotocraftApp>, pos, pressed| {
h.event(egui::Event::PointerButton { pos, button: PointerButton::Primary, pressed, modifiers: egui::Modifiers::NONE });
h.step();
};
h.hover_at(swatch);
h.step();
press(&mut h, swatch, true);
press(&mut h, swatch, false);
h.run_steps(3);
let popup = |h: &egui_kittest::Harness<'_, PhotocraftApp>| {
h.ctx.memory(|m| m.areas().visible_layer_ids().into_iter().filter(|l| l.order == egui::Order::Foreground).find_map(|l| m.area_rect(l.id)))
};
let opened = popup(&h).expect("the picker opened");
let fill = |h: &egui_kittest::Harness<'_, PhotocraftApp>| {
let st = h.state().session.active().unwrap();
match &st.doc.layer(photocraft_doc::LayerId(id)).unwrap().content {
LayerContent::Shape(sh) => sh.fill.clone(),
_ => None,
}
};
let before = fill(&h);
// Drag across the picker's colour area.
let start = opened.min + vec2(60.0, opened.height() * 0.5);
h.hover_at(start);
h.step();
press(&mut h, start, true);
for i in 1..=10 {
h.event(egui::Event::PointerMoved(start + vec2(i as f32 * 5.0, i as f32)));
h.step();
let r = popup(&h).expect("the picker stays open");
assert_eq!(r.left_top(), opened.left_top(), "step {i}: the picker moved from {opened:?} to {r:?}");
assert!(h.ctx.content_rect().contains_rect(r), "step {i}: {r:?} runs off the window");
}
press(&mut h, start, false);
assert_ne!(fill(&h), before, "the drag picked a colour");
}
}
#[cfg(test)]
+30
View File
@@ -382,6 +382,36 @@ pub fn dropdown<T: PartialEq + Clone>(ui: &mut Ui, id: &str, current: &mut T, op
changed
}
/// The colour picker popup of a colour swatch: a click on `swatch` toggles it, a click outside
/// closes it. While open, its left edge stays where it first showed (at the swatch, or further
/// left when the window edge needs it), below the swatch or above it as room allows. The picker's
/// width follows its value readouts, and placing it anew every frame moved it under the pointer,
/// flipping it from side to side near the right edge of the window (#534).
pub fn swatch_popup(swatch: &Response) -> egui::Popup<'static> {
// Room for the readouts to widen after the picker opened.
const SLACK: f32 = 32.0;
let popup = egui::Popup::from_toggle_button_response(swatch).close_behavior(egui::PopupCloseBehavior::CloseOnClickOutside);
let ctx = &swatch.ctx;
let key = popup.get_id().with("left");
if !popup.is_open() {
ctx.data_mut(|d| d.remove::<f32>(key));
return popup;
}
// Its width is known from the frame after it opened (egui sizes it unseen first).
let left = ctx.data(|d| d.get_temp::<f32>(key)).or_else(|| {
let width = popup.get_expected_size()?.x;
let screen = ctx.content_rect();
let left = swatch.rect.left().min(screen.right() - width - SLACK).max(screen.left());
ctx.data_mut(|d| d.insert_temp(key, left));
Some(left)
});
let Some(left) = left else { return popup };
popup
.anchor(Rect::from_x_y_ranges(left..=left, swatch.rect.y_range()))
.align(egui::RectAlign::BOTTOM_START)
.align_alternatives(&[egui::RectAlign::TOP_START])
}
/// Paint a small checkerboard (transparency) in `rect`.
pub fn checker(painter: &egui::Painter, rect: Rect, cell: f32) {
painter.rect_filled(rect, 0.0, Color32::from_gray(250));