diff --git a/CHANGELOG.md b/CHANGELOG.md index 558a445..4f42281 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,9 @@ All notable changes to this project will be documented in this file. - The Site Manager has its Browse button back for choosing a private key file, and it opens where the current path points rather than at your home folder. ### Fixed +- Backspace, Alt+Left, and Alt+Up in a file pane now go to the parent directory. Those keys were bound to a list event that never reported which key was pressed, so they did nothing in either pane. Ctrl+Up and Ctrl+[ do the same (Command+Up and Command+[ on a Mac, matching Finder). +- The exit sound was cut off as the program closed. Closing now waits for it to finish. +- Uploading a file always asked whether to replace it, even when nothing of that name was on the server. The prompt now appears only when the remote folder already has that name, matching downloads. - Installing an update no longer asks you to close Portkey Drop first, and no longer leaves the app sitting there with nothing happening. Setup checks whether the app is running, and it was being started while the app was still on screen. Portkey Drop now closes itself and Setup opens on its own first page, the way the portable and macOS updates already worked. Quitting for an update is also final now: the download's progress window could keep the program alive after its window had closed, which left the update waiting for a program that never went away. - Leaving Portkey Drop while a transfer was running froze the window instead of closing it. Quitting closed the connection, and that waited for the transfer to let go of it -- on the window's own thread, so nothing repainted until the transfer finished. The connection is now closed out of the way of the window. - Downloading a folder no longer freezes the window for the length of the transfer. The connection check behind the status bar and tray tooltip waited for the connection to be free, and a transfer holds it for as long as its work takes -- the whole listing of a folder, then each file in turn -- so the window stopped repainting and Windows offered to close it. The check no longer waits: a connection busy with a transfer is reported as connected. diff --git a/README.md b/README.md index ba312e9..7f55175 100644 --- a/README.md +++ b/README.md @@ -33,7 +33,7 @@ Portkey Drop includes a built-in default sound pack with short cues for transfer | Ctrl+Shift+T | Transfer queue | | Ctrl+Enter | Connect using the quick connect bar | | Enter | Open directory / download file | -| Backspace | Parent directory | +| Backspace, Alt+Left, Alt+Up, Ctrl+Up, Ctrl+[ | Parent directory | | Delete | Delete selected | | F2 | Rename selected | diff --git a/crates/portkeydrop-core/src/local_files.rs b/crates/portkeydrop-core/src/local_files.rs index 977ab75..f8bd63f 100644 --- a/crates/portkeydrop-core/src/local_files.rs +++ b/crates/portkeydrop-core/src/local_files.rs @@ -165,26 +165,45 @@ pub fn mkdir_local(parent: &Path, name: &str) -> std::io::Result { /// Appends ` (1)`, ` (2)`, ... before the extension, matching what browsers and /// file managers do. pub fn unique_local_path(path: &Path) -> PathBuf { - if !path.exists() { - return path.to_path_buf(); - } let parent = path.parent().unwrap_or_else(|| Path::new(".")); - let stem = path - .file_stem() - .map(|s| s.to_string_lossy().into_owned()) - .unwrap_or_default(); - let extension = path - .extension() - .map(|s| format!(".{}", s.to_string_lossy())) + let name = path + .file_name() + .map(|name| name.to_string_lossy().into_owned()) .unwrap_or_default(); + parent.join(unique_file_name(&name, |candidate| { + parent.join(candidate).exists() + })) +} +/// A file name that does not collide with names `exists` reports as taken. +/// +/// Same numbering as [`unique_local_path`], so upload-and-keep-both and +/// download-and-keep-both produce the same kind of name. +pub fn unique_file_name(name: &str, exists: impl Fn(&str) -> bool) -> String { + if !exists(name) { + return name.to_string(); + } + let (stem, extension) = split_file_name(name); for counter in 1..10_000 { - let candidate = parent.join(format!("{stem} ({counter}){extension}")); - if !candidate.exists() { + let candidate = format!("{stem} ({counter}){extension}"); + if !exists(&candidate) { return candidate; } } - path.to_path_buf() + name.to_string() +} + +/// Split `name` into stem and extension, including the dot on the extension. +/// +/// A leading dot is part of the stem (`.hidden` stays `.hidden (1)`, not +/// ` (1).hidden`). `Path::file_stem` on Windows treats that as an extension. +fn split_file_name(name: &str) -> (&str, String) { + match name.rsplit_once('.') { + Some((stem, ext)) if !stem.is_empty() && !ext.is_empty() && !ext.contains('/') => { + (stem, format!(".{ext}")) + } + _ => (name, String::new()), + } } #[cfg(test)] @@ -320,4 +339,15 @@ mod tests { std::fs::write(&path, b"x").unwrap(); assert_eq!(unique_local_path(&path), dir.path().join("README (1)")); } + + #[test] + fn a_leading_dot_stays_on_the_stem() { + let taken = |name: &str| name == ".hidden"; + assert_eq!(unique_file_name(".hidden", taken), ".hidden (1)"); + assert_eq!(unique_file_name("notes.txt", |_| false), "notes.txt"); + assert_eq!( + unique_file_name("notes.txt", |name| name == "notes.txt"), + "notes (1).txt" + ); + } } diff --git a/crates/portkeydrop-core/src/soundpacks/mod.rs b/crates/portkeydrop-core/src/soundpacks/mod.rs index e509143..a1fb499 100644 --- a/crates/portkeydrop-core/src/soundpacks/mod.rs +++ b/crates/portkeydrop-core/src/soundpacks/mod.rs @@ -16,7 +16,7 @@ use std::path::{Path, PathBuf}; pub use install::{is_safe_archive_name, InstallError, PackInstaller}; pub use manifest::{PackManifest, SoundEntry}; -pub use player::{can_decode, play_sound_file, SoundPlayer}; +pub use player::{can_decode, play_sound_file, wait_for_playback, SoundPlayer, EXIT_SOUND_TIMEOUT}; /// Directory name of the built-in pack. pub const DEFAULT_PACK: &str = "default"; diff --git a/crates/portkeydrop-core/src/soundpacks/player.rs b/crates/portkeydrop-core/src/soundpacks/player.rs index ff52f82..c7004f5 100644 --- a/crates/portkeydrop-core/src/soundpacks/player.rs +++ b/crates/portkeydrop-core/src/soundpacks/player.rs @@ -3,13 +3,21 @@ //! Sounds are feedback, never a gate on anything: if the audio device is //! missing, busy, or the file will not decode, playback reports `false` and the //! app carries on. Nothing here blocks the caller — a transfer must not wait on -//! a chime. +//! a chime. The exception is [`wait_for_playback`], used on exit so the closing +//! sound is not cut off when the process dies. use std::path::{Path, PathBuf}; use std::sync::{Mutex, OnceLock}; +use std::time::{Duration, Instant}; use rodio::{Decoder, OutputStream, OutputStreamHandle, Sink}; +/// Longest closing will wait for the exit sound before giving up. +/// +/// Playback is otherwise fire-and-forget; this cap is so a hung audio +/// device cannot trap the process. +pub const EXIT_SOUND_TIMEOUT: Duration = Duration::from_secs(8); + use super::{resolve_sound, DEFAULT_PACK}; /// The process-wide audio output. @@ -113,6 +121,27 @@ pub fn play_sound_file(path: &Path, volume: f32) -> bool { true } +/// Wait until every started sound has finished, or `timeout` elapses. +/// +/// Used on exit so the closing chime is not cut off when the process dies. +/// Other playback stays fire-and-forget: a transfer must not wait on a chime. +pub fn wait_for_playback(timeout: Duration) { + let started = Instant::now(); + while started.elapsed() < timeout { + let playing = match active_sinks().lock() { + Ok(mut sinks) => { + sinks.retain(|sink| !sink.empty()); + !sinks.is_empty() + } + Err(_) => return, + }; + if !playing { + return; + } + std::thread::sleep(Duration::from_millis(20)); + } +} + /// Plays event sounds from the active pack. #[derive(Debug, Clone)] pub struct SoundPlayer { @@ -291,4 +320,21 @@ mod tests { std::fs::write(&path, b"definitely not audio").unwrap(); assert!(!play_sound_file(&path, 1.0)); } + + #[test] + fn waiting_with_nothing_playing_returns_immediately() { + let start = Instant::now(); + wait_for_playback(Duration::from_secs(5)); + assert!( + start.elapsed() < Duration::from_millis(500), + "idle wait took {:?}", + start.elapsed() + ); + } + + #[test] + fn the_exit_wait_is_long_enough_for_a_chime_and_short_enough_not_to_trap() { + assert!(EXIT_SOUND_TIMEOUT >= Duration::from_secs(2)); + assert!(EXIT_SOUND_TIMEOUT <= Duration::from_secs(10)); + } } diff --git a/crates/portkeydrop/src/ui/file_pane.rs b/crates/portkeydrop/src/ui/file_pane.rs index bde8779..b3bfb62 100644 --- a/crates/portkeydrop/src/ui/file_pane.rs +++ b/crates/portkeydrop/src/ui/file_pane.rs @@ -192,6 +192,12 @@ impl FilePane { self.state.borrow().path.clone() } + /// Whether the listing includes an entry with this name, including hidden + /// files the list is not showing. + pub fn contains_name(&self, name: &str) -> bool { + self.state.borrow().contains_name(name) + } + /// The path as typed into the path bar. pub fn typed_path(&self) -> String { self.path_bar.get_value().trim().to_string() diff --git a/crates/portkeydrop/src/ui/ids.rs b/crates/portkeydrop/src/ui/ids.rs index 9a8c992..31470af 100644 --- a/crates/portkeydrop/src/ui/ids.rs +++ b/crates/portkeydrop/src/ui/ids.rs @@ -174,6 +174,30 @@ pub const SHORTCUTS: &[Shortcut] = &[ description: "Go to the parent directory", binding: Binding::Control, }, + Shortcut { + section: "Navigation", + keys: "Alt+Left", + description: "Go to the parent directory", + binding: Binding::Control, + }, + Shortcut { + section: "Navigation", + keys: "Alt+Up", + description: "Go to the parent directory", + binding: Binding::Control, + }, + Shortcut { + section: "Navigation", + keys: "Ctrl+Up", + description: "Go to the parent directory", + binding: Binding::Control, + }, + Shortcut { + section: "Navigation", + keys: "Ctrl+[", + description: "Go to the parent directory", + binding: Binding::Control, + }, Shortcut { section: "Navigation", keys: "Ctrl+H", @@ -416,7 +440,17 @@ mod tests { fn keys_that_must_not_be_frame_wide_are_control_bound() { // Delete and F2 as menubar accelerators would fire inside every text // field in the window, so they belong to the file lists. - for keys in ["Delete", "F2", "Backspace", "Escape", "Shift+F10"] { + for keys in [ + "Delete", + "F2", + "Backspace", + "Alt+Left", + "Alt+Up", + "Ctrl+Up", + "Ctrl+[", + "Escape", + "Shift+F10", + ] { let shortcut = SHORTCUTS .iter() .find(|shortcut| shortcut.keys == keys) diff --git a/crates/portkeydrop/src/ui/keys.rs b/crates/portkeydrop/src/ui/keys.rs index 484fef8..1038942 100644 --- a/crates/portkeydrop/src/ui/keys.rs +++ b/crates/portkeydrop/src/ui/keys.rs @@ -30,9 +30,46 @@ pub const F6: i32 = F1 + 5; /// F10, used with Shift for the context menu. pub const F10: i32 = F1 + 9; +/// Left arrow. The arrows run consecutively from here. +pub const LEFT: i32 = 314; +/// Up arrow. +pub const UP: i32 = LEFT + 1; +/// Right arrow. +pub const RIGHT: i32 = LEFT + 2; +/// Down arrow. +pub const DOWN: i32 = LEFT + 3; + +/// `[`. Finder uses Command+[ for back; the same chord with Ctrl is used +/// on the other platforms so the help window can say Ctrl everywhere. +pub const OPEN_BRACKET: i32 = b'[' as i32; + /// Enter, on the numeric keypad. pub const NUMPAD_ENTER: i32 = 385; +/// What a key in a file list should do. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ListCommand { + /// Delete the selection. + Delete, + /// Rename the selection. + Rename, + /// Go to the parent directory. + Parent, +} + +/// Modifier keys on a file-list key event. +/// +/// `cmd` is wxWidgets' command key: Command on macOS, Control on Windows +/// and Linux. Using that, rather than Alt on every platform, is what makes +/// Finder's Command+Up reach the same command as Explorer's Alt+Up. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub struct KeyMods { + /// Option on macOS, Alt elsewhere. + pub alt: bool, + /// Command on macOS, Control elsewhere. + pub cmd: bool, +} + /// Whether a key code means "activate this item". /// /// Both Enter keys count: a numeric keypad's Enter reports a different code, @@ -41,6 +78,24 @@ pub fn is_enter(code: i32) -> bool { code == RETURN || code == NUMPAD_ENTER } +/// The command a file-list key should run, if any. +/// +/// Parent directory is several chords because each platform's file manager +/// teaches a different one: Backspace and Alt+Left/Up on Windows and Linux, +/// Command+Up and Command+[ on macOS. They must not be frame-wide accelerators +/// or they would fire inside the path bar — and Command+Up in a text field +/// is "go to the start of the document". +pub fn list_command(code: i32, mods: KeyMods) -> Option { + match (code, mods.alt, mods.cmd) { + (DELETE, false, false) => Some(ListCommand::Delete), + (F2, false, false) => Some(ListCommand::Rename), + (BACK, false, false) => Some(ListCommand::Parent), + (LEFT, true, false) | (UP, true, false) => Some(ListCommand::Parent), + (UP, false, true) | (OPEN_BRACKET, false, true) => Some(ListCommand::Parent), + _ => None, + } +} + #[cfg(test)] mod tests { use super::*; @@ -62,6 +117,14 @@ mod tests { assert_eq!(F10, 349); } + #[test] + fn the_arrow_keys_run_consecutively_from_left() { + assert_eq!(LEFT, 314); + assert_eq!(UP, 315); + assert_eq!(RIGHT, 316); + assert_eq!(DOWN, 317); + } + #[test] fn every_key_this_app_binds_is_distinct() { // Two keys sharing a value would silently merge two commands. @@ -76,6 +139,11 @@ mod tests { F5, F6, F10, + LEFT, + UP, + RIGHT, + DOWN, + OPEN_BRACKET, NUMPAD_ENTER, ]; let mut sorted = codes.to_vec(); @@ -109,4 +177,70 @@ mod tests { assert_eq!(classify(BACK), "parent"); assert_eq!(classify(SPACE), "other"); } + + fn none() -> KeyMods { + KeyMods::default() + } + + fn alt() -> KeyMods { + KeyMods { + alt: true, + cmd: false, + } + } + + fn cmd() -> KeyMods { + KeyMods { + alt: false, + cmd: true, + } + } + + #[test] + fn the_file_list_keys_run_their_commands() { + assert_eq!(list_command(DELETE, none()), Some(ListCommand::Delete)); + assert_eq!(list_command(F2, none()), Some(ListCommand::Rename)); + assert_eq!(list_command(BACK, none()), Some(ListCommand::Parent)); + assert_eq!(list_command(LEFT, alt()), Some(ListCommand::Parent)); + assert_eq!(list_command(UP, alt()), Some(ListCommand::Parent)); + } + + #[test] + fn macos_parent_chords_use_the_command_key() { + // Finder: Command+Up is enclosing folder, Command+[ is back. + // wxWidgets reports that key as cmd, not alt. + assert_eq!(list_command(UP, cmd()), Some(ListCommand::Parent)); + assert_eq!(list_command(OPEN_BRACKET, cmd()), Some(ListCommand::Parent)); + assert_eq!(OPEN_BRACKET, 91); + } + + #[test] + fn unmodified_arrows_stay_with_the_list() { + // Left and Up without a modifier move the cursor; treating them as + // parent would make the list unnavigable. + assert_eq!(list_command(LEFT, none()), None); + assert_eq!(list_command(UP, none()), None); + assert_eq!(list_command(RIGHT, alt()), None); + assert_eq!(list_command(DOWN, alt()), None); + assert_eq!(list_command(BACK, alt()), None); + assert_eq!(list_command(SPACE, none()), None); + } + + #[test] + fn command_left_is_not_parent() { + // Command+Left is beginning-of-line in text and is not Finder's + // enclosing-folder shortcut. Command+Backspace is Move to Trash. + assert_eq!(list_command(LEFT, cmd()), None); + assert_eq!(list_command(BACK, cmd()), None); + assert_eq!( + list_command( + UP, + KeyMods { + alt: true, + cmd: true + } + ), + None + ); + } } diff --git a/crates/portkeydrop/src/ui/main_frame.rs b/crates/portkeydrop/src/ui/main_frame.rs index 333fb0c..d1eeba6 100644 --- a/crates/portkeydrop/src/ui/main_frame.rs +++ b/crates/portkeydrop/src/ui/main_frame.rs @@ -279,6 +279,11 @@ impl MainFrame { &ids::labelled("&Home Directory", ids::ID_HOME_DIR), "Go to the home directory", ) + .append_item( + ids::ID_PARENT_DIR, + "&Parent Directory", + "Go to the parent directory (Backspace, Alt+Left, Alt+Up, or Ctrl+Up in a file pane)", + ) .append_check_item( ids::ID_SHOW_HIDDEN, "Show Hi&dden Files", @@ -559,16 +564,22 @@ impl MainFrame { pane.list .on_item_activated(move |_| this.activate_selection(side)); - // Delete, F2, and Backspace belong to the list, not to the menubar: - // as frame-wide accelerators they would fire inside text fields too. + // Delete, F2, and the parent-directory keys belong to the list, not + // to the menubar: as frame-wide accelerators they would fire inside + // text fields too. Command+Up in a text field is "start of document". + // + // ListCtrl::on_key_down binds EVT_LIST_KEY_DOWN. That event is not a + // wxKeyEvent, so wxDragon's get_key_code() always returns 0 and none + // of those keys match. EVT_KEY_DOWN is a real key event; EVT_CHAR is + // the fallback when the native list swallows KEY_DOWN for Backspace. let this = self.clone(); - pane.list - .on_key_down(move |event| match event.get_key_code().unwrap_or(0) { - keys::DELETE => this.delete_selection(), - keys::F2 => this.rename_selection(), - keys::BACK => this.go_parent_in(side), - _ => {} - }); + pane.list.bind_internal(EventType::KEY_DOWN, move |event| { + this.handle_pane_key(side, &WindowEventData::new(event)); + }); + let this = self.clone(); + pane.list.bind_internal(EventType::CHAR, move |event| { + this.handle_pane_key(side, &WindowEventData::new(event)); + }); // Announce the row under the cursor; screen readers read the focused // row themselves, but the status bar mirrors it for everyone else. @@ -1109,6 +1120,28 @@ impl MainFrame { }); } + fn handle_pane_key(&self, side: Side, event: &WindowEventData) { + let command = match event { + WindowEventData::Keyboard(key) => keys::list_command( + key.get_key_code().unwrap_or(0), + keys::KeyMods { + alt: key.alt_down(), + cmd: key.cmd_down(), + }, + ), + _ => { + event.skip(true); + return; + } + }; + match command { + Some(keys::ListCommand::Delete) => self.delete_selection(), + Some(keys::ListCommand::Rename) => self.rename_selection(), + Some(keys::ListCommand::Parent) => self.go_parent_in(side), + None => event.skip(true), + } + } + fn go_parent(&self) { self.go_parent_in(self.active_side()); } @@ -1608,6 +1641,12 @@ impl MainFrame { state.play_sound("exit"); } state.clear_client(); + drop(state); + // Playback is fire-and-forget, and the audio device dies with the + // process. Wait here so the closing chime is not cut off. + portkeydrop_core::soundpacks::wait_for_playback( + portkeydrop_core::soundpacks::EXIT_SOUND_TIMEOUT, + ); } } diff --git a/crates/portkeydrop/src/ui/operations.rs b/crates/portkeydrop/src/ui/operations.rs index 8977f0a..cd8a798 100644 --- a/crates/portkeydrop/src/ui/operations.rs +++ b/crates/portkeydrop/src/ui/operations.rs @@ -132,7 +132,7 @@ impl MainFrame { remote_dir }; - let destination = protocols::path::join(&remote_dir, &file.name); + let mut destination = protocols::path::join(&remote_dir, &file.name); let size = if file.is_dir { 0 @@ -142,12 +142,22 @@ impl MainFrame { .unwrap_or(file.size) }; - let overwrite = match self.resolve_conflict(overwrite_mode, &file.name, "uploaded", batch) { - Conflict::Skip => return false, - - Conflict::Overwrite => true, - - Conflict::Fail => false, + // Same gate as downloads: only ask when the destination is already + // there. Asking on every upload made the prompt look broken. + let overwrite = if self.pane(Side::Remote).contains_name(&file.name) { + match self.resolve_conflict(overwrite_mode, &file.name, "uploaded", batch) { + Conflict::Skip => return false, + Conflict::Overwrite => true, + Conflict::Fail => { + let unique = local_files::unique_file_name(&file.name, |name| { + self.pane(Side::Remote).contains_name(name) + }); + destination = protocols::path::join(&remote_dir, &unique); + false + } + } + } else { + false }; self.state.borrow().transfers.submit_upload( diff --git a/crates/portkeydrop/src/ui/view.rs b/crates/portkeydrop/src/ui/view.rs index 60bda3d..d1b95c6 100644 --- a/crates/portkeydrop/src/ui/view.rs +++ b/crates/portkeydrop/src/ui/view.rs @@ -139,6 +139,14 @@ impl PaneState { self.all_files.get(*self.visible.get(row)?) } + /// Whether the last listing included an entry with this name. + /// + /// Hidden entries count: an upload that would collide with a file the + /// list is not showing is still a collision. + pub fn contains_name(&self, name: &str) -> bool { + self.all_files.iter().any(|file| file.name == name) + } + /// The files at the given display rows. pub fn files_at(&self, rows: &[usize]) -> Vec { rows.iter() @@ -428,6 +436,16 @@ mod tests { assert_eq!(pane.visible_count(), 0); } + #[test] + fn a_listing_knows_whether_a_name_is_already_there() { + let pane = pane(); + assert!(pane.contains_name("docs")); + assert!(pane.contains_name("zebra.txt")); + // Hidden files still collide, even when the list is not showing them. + assert!(pane.contains_name(".hidden")); + assert!(!pane.contains_name("notes.txt")); + } + #[test] fn rows_map_back_to_their_files() { let pane = pane();