mirror of
https://github.com/storytold/pdfcraft.git
synced 2026-11-10 18:14:25 +00:00
M3: All tools view keeps Escape for dialogs, leaves with the mode option, and searches Arabic
Review follow-ups for the expanded All tools view:
- Escape closed the view even when a dialog or prompt over it owned the key.
It now checks modal_open() first.
- set_option("mode", …) changed workspace without clearing the view, so
leaving and coming back to All tools reopened it. Another workspace now puts
it away, as switch_mode does.
- Right-to-left translations are stored in display order, so a multi-word
Arabic query typed in logical order missed its tool. Such a query now matches
word by word, as the command palette's search does.
Each test was seen to fail with its fix reverted.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5.5
parent
522c1dcd22
commit
23ed4cca65
@@ -12,11 +12,23 @@ const TILE: egui::Vec2 = vec2(148.0, 118.0);
|
||||
/// Item names a tile lists when the search found the tool by them.
|
||||
const MATCHES_SHOWN: usize = 2;
|
||||
|
||||
/// Whether `query` (already trimmed and lower-cased) is in `label` or its translation.
|
||||
/// A right-to-left language's translations are in display order (their words run the other way)
|
||||
/// while the query is in typing order, so there several words match one by one, as in the
|
||||
/// command palette.
|
||||
fn label_hit(label: &str, translated: &str, query: &str, rtl: bool) -> bool {
|
||||
let translated = translated.to_lowercase();
|
||||
label.to_lowercase().contains(query)
|
||||
|| translated.contains(query)
|
||||
|| (rtl && query.contains(char::is_whitespace) && query.split_whitespace().all(|word| translated.contains(word)))
|
||||
}
|
||||
|
||||
/// Whether `group` matches `query` (already trimmed and lower-cased), in English or the UI
|
||||
/// language. `None` is no match; otherwise the items that matched, empty when the tool's own
|
||||
/// name did.
|
||||
fn matches(query: &str, group: &ToolGroup) -> Option<Vec<&'static str>> {
|
||||
let hit = |label: &str| label.to_lowercase().contains(query) || tl!(label).to_lowercase().contains(query);
|
||||
let rtl = crate::i18n::current().rtl();
|
||||
let hit = |label: &str| label_hit(label, tl!(label), query, rtl);
|
||||
if query.is_empty() || hit(group.label) {
|
||||
return Some(Vec::new());
|
||||
}
|
||||
@@ -26,7 +38,8 @@ fn matches(query: &str, group: &ToolGroup) -> Option<Vec<&'static str>> {
|
||||
|
||||
pub fn show(app: &mut PdfCraftApp, ui: &mut egui::Ui) {
|
||||
let t = Tokens::get(ui.ctx());
|
||||
if ui.input(|i| i.key_pressed(egui::Key::Escape)) {
|
||||
// A dialog or prompt over the view owns Escape.
|
||||
if !app.modal_open() && ui.input(|i| i.key_pressed(egui::Key::Escape)) {
|
||||
app.all_tools_full = false;
|
||||
return;
|
||||
}
|
||||
@@ -155,4 +168,14 @@ mod tests {
|
||||
assert!(!rotate.is_empty() && rotate.iter().all(|l| l.to_lowercase().contains("rotate")), "{rotate:?}");
|
||||
assert_eq!(matches("no such tool anywhere", group("organize")), None);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn right_to_left_labels_match_a_query_typed_in_logical_order() {
|
||||
let shown = crate::bidi::display_rtl("تنظيم الصفحات");
|
||||
assert_ne!(shown, "تنظيم الصفحات", "the catalogue reverses the words for display");
|
||||
assert!(label_hit("Organize pages", &shown, "تنظيم الصفحات", true));
|
||||
assert!(label_hit("Organize pages", &shown, "الصفحات", true), "one word");
|
||||
assert!(!label_hit("Organize pages", &shown, "تنظيم الملفات", true), "every word must be there");
|
||||
assert!(!label_hit("Organize pages", &shown, "تنظيم الصفحات", false), "left-to-right labels stay exact");
|
||||
}
|
||||
}
|
||||
|
||||
@@ -2515,6 +2515,10 @@ impl PdfCraftApp {
|
||||
let mode = Mode::parse(value).unwrap_or(Mode::AllTools);
|
||||
self.mode_override = Some(mode);
|
||||
self.mode = mode;
|
||||
// As switch_mode: another workspace puts the expanded All tools view away.
|
||||
if mode != Mode::AllTools {
|
||||
self.all_tools_full = false;
|
||||
}
|
||||
}
|
||||
("default-mode", _) => {
|
||||
self.default_mode = Mode::parse(value).ok_or("default-mode must be all, read, edit, convert or sign")?;
|
||||
|
||||
@@ -169,6 +169,27 @@ fn collapse_and_escape_close_the_expanded_all_tools() {
|
||||
assert!(!h.state().all_tools_full);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn escape_in_a_dialog_over_the_expanded_all_tools_leaves_the_view_open() {
|
||||
let mut h = harness(|app| app.set_option("tools", "full").unwrap());
|
||||
h.state_mut().dialog = Some(pdfcraft_ui_egui::Dialog::About);
|
||||
h.run_steps(3);
|
||||
h.key_press(egui::Key::Escape);
|
||||
h.run_steps(3);
|
||||
assert!(h.state().all_tools_full, "Escape belonged to the dialog");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn another_workspace_from_the_mode_option_puts_the_expanded_all_tools_away() {
|
||||
let mut app = PdfCraftApp::new();
|
||||
app.set_option("tools", "full").unwrap();
|
||||
app.set_option("mode", "all").unwrap();
|
||||
assert!(app.all_tools_full, "All tools itself keeps the view");
|
||||
app.set_option("mode", "read").unwrap();
|
||||
app.set_option("mode", "all").unwrap();
|
||||
assert!(!app.all_tools_full, "coming back shows the sidebar, as the mode bar does");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn opening_a_pdf_shows_comments_and_bookmarks() {
|
||||
let mut h = harness(|app| app.open_bytes("fixture.pdf", None, FIXTURE.to_vec()).expect("fixture opens"));
|
||||
|
||||
Reference in New Issue
Block a user