From 214c25063c2704aff380824665604f620a5b6930 Mon Sep 17 00:00:00 2001 From: GenericConfluent Date: Thu, 13 Nov 2025 10:57:49 -0700 Subject: [PATCH 1/5] feat(shortcuts): WIP load existing shortcut into create context drawer (pop-os/cosmic-settings#1566) An initial quick and dirty attempt to allow the descriptions and actions of custom shortcuts to be edited after create. This needs some more work to behave properly with the keybind replace dialog. We hook into `ShowShortcut` and set `self.add_shortcut.active = true` to have the correct context drawer shown. The extra code in the other branch of `custom::Page::context_drawer` should also probably be removed. --- .../pages/input/keyboard/shortcuts/custom.rs | 32 +++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/cosmic-settings/src/pages/input/keyboard/shortcuts/custom.rs b/cosmic-settings/src/pages/input/keyboard/shortcuts/custom.rs index 96dcff9e0..62beaf230 100644 --- a/cosmic-settings/src/pages/input/keyboard/shortcuts/custom.rs +++ b/cosmic-settings/src/pages/input/keyboard/shortcuts/custom.rs @@ -210,8 +210,36 @@ impl Page { } Message::Shortcut(message) => { - if let ShortcutMessage::ShowShortcut(..) = message { - self.add_shortcut.active = false; + if let ShortcutMessage::ShowShortcut(id, _description) = &message { + self.add_shortcut.active = true; + // If we are getting ShowShortcut shortcut should probably exist. + // But if it doesn't we'll just skip filling. + if let Some(shortcut_model) = self.model.shortcut_models.get(*id) { + // FIXME: Should Cow these to get rid of the clones + self.add_shortcut.name = shortcut_model.description.clone(); + + if let Action::Spawn(task) = &shortcut_model.action { + self.add_shortcut.task = task.clone(); + } + + self.add_shortcut.keys.clear(); + for (_, shortcut_binding) in &shortcut_model.bindings { + if shortcut_binding.binding.is_set() { + self.add_shortcut.keys.insert(( + shortcut_binding.binding.to_string(), + widget::Id::unique(), + )); + } + } + + if self.add_shortcut.keys.is_empty() { + self.add_shortcut + .keys + .insert((String::default(), widget::Id::unique())); + } + + self.add_shortcut.binding = Binding::default(); + } } return self.model.update(message); From 98cac85b14b521a13fce78cc2f4e32ff4fcb03d6 Mon Sep 17 00:00:00 2001 From: GenericConfluent Date: Thu, 13 Nov 2025 11:42:45 -0700 Subject: [PATCH 2/5] fix(shortcuts): prevent redundant replacement dialog for existing actions (pop-os/cosmic-settings#1566) Note that this commit renamed the `editing` field on `AddShortcut` to `capturing_key` and added a new field `editing_action` which is the index of the `ShortcutModel`. The logic under `ShortcutMessage::ShowShortcut` in `custom.rs` probably belongs in `AddShortcut::enable`. --- .../pages/input/keyboard/shortcuts/custom.rs | 52 ++++++++++++------- 1 file changed, 33 insertions(+), 19 deletions(-) diff --git a/cosmic-settings/src/pages/input/keyboard/shortcuts/custom.rs b/cosmic-settings/src/pages/input/keyboard/shortcuts/custom.rs index 62beaf230..e99c23167 100644 --- a/cosmic-settings/src/pages/input/keyboard/shortcuts/custom.rs +++ b/cosmic-settings/src/pages/input/keyboard/shortcuts/custom.rs @@ -75,7 +75,8 @@ pub enum Message { #[derive(Default)] struct AddShortcut { pub active: bool, - pub editing: Option, + pub editing_action: Option, + pub capturing_key: Option, pub name: String, pub task: String, pub keys: Slab<(String, widget::Id)>, @@ -87,6 +88,7 @@ impl AddShortcut { self.active = true; self.name.clear(); self.task.clear(); + self.editing_action = None; if self.keys.is_empty() { self.keys.insert((String::new(), widget::Id::unique())); @@ -111,10 +113,10 @@ impl Page { Message::KeyEditing(id, enable) => { if enable { - self.add_shortcut.editing = Some(id); + self.add_shortcut.capturing_key = Some(id); return iced_winit::platform_specific::commands::keyboard_shortcuts_inhibit::inhibit_shortcuts(true).discard(); - } else if self.add_shortcut.editing == Some(id) { - self.add_shortcut.editing = None; + } else if self.add_shortcut.capturing_key == Some(id) { + self.add_shortcut.capturing_key = None; return Task::batch(vec![ widget::text_input::focus(widget::Id::unique()), @@ -148,7 +150,7 @@ impl Page { } if let Some((index, binding)) = addable_bindings.first() { - self.add_shortcut.editing = Some(*index); + self.add_shortcut.capturing_key = Some(*index); return Task::batch(vec![ widget::text_input::focus(binding.clone()), iced_winit::platform_specific::commands::keyboard_shortcuts_inhibit::inhibit_shortcuts(false).discard(), @@ -156,7 +158,7 @@ impl Page { } else { // make a new empty binding if none exist let new_id = widget::Id::unique(); - self.add_shortcut.editing = Some( + self.add_shortcut.capturing_key = Some( self.add_shortcut .keys .insert((String::new(), new_id.clone())), @@ -170,7 +172,7 @@ impl Page { Message::EditCombination => { if let Some((slab_index, (_, id))) = self.add_shortcut.keys.iter().next() { - self.add_shortcut.editing = Some(slab_index); + self.add_shortcut.capturing_key = Some(slab_index); return Task::batch(vec![ widget::text_input::focus(id.clone()), widget::text_input::select_all(id.clone()), @@ -212,6 +214,7 @@ impl Page { Message::Shortcut(message) => { if let ShortcutMessage::ShowShortcut(id, _description) = &message { self.add_shortcut.active = true; + self.add_shortcut.editing_action = Some(*id); // If we are getting ShowShortcut shortcut should probably exist. // But if it doesn't we'll just skip filling. if let Some(shortcut_model) = self.model.shortcut_models.get(*id) { @@ -288,7 +291,7 @@ impl Page { let Some(k) = self .add_shortcut .keys - .get_mut(self.add_shortcut.editing.unwrap()) + .get_mut(self.add_shortcut.capturing_key.unwrap()) else { return iced_winit::platform_specific::commands::keyboard_shortcuts_inhibit::inhibit_shortcuts(false).discard(); }; @@ -321,7 +324,7 @@ impl Page { if let Some(k) = self .add_shortcut .keys - .get_mut(self.add_shortcut.editing.unwrap()) + .get_mut(self.add_shortcut.capturing_key.unwrap()) { k.0 = self.add_shortcut.binding.to_string(); } @@ -329,7 +332,7 @@ impl Page { } Message::KeyReleased(keycode, _, _) => { // if the currently selected shortcut matches, finish selecting shortcut - if self.add_shortcut.editing.is_some() + if self.add_shortcut.capturing_key.is_some() && self.add_shortcut.active && self.add_shortcut.binding.key.is_some() && self @@ -353,7 +356,7 @@ impl Page { let Some(k) = self .add_shortcut .keys - .get_mut(self.add_shortcut.editing.unwrap()) + .get_mut(self.add_shortcut.capturing_key.unwrap()) else { return iced_winit::platform_specific::commands::keyboard_shortcuts_inhibit::inhibit_shortcuts(false).discard(); }; @@ -378,7 +381,7 @@ impl Page { } Message::KeyPressed(keycode, unmodified_keysym, location, modifiers) => { if unmodified_keysym == Key::Named(Named::Escape) && modifiers.is_empty() { - self.add_shortcut.editing = None; + self.add_shortcut.capturing_key = None; return Task::batch(vec![ widget::text_input::focus(widget::Id::unique()), iced_winit::platform_specific::commands::keyboard_shortcuts_inhibit::inhibit_shortcuts(false).discard(), @@ -394,7 +397,7 @@ impl Page { if let Some(k) = self .add_shortcut .keys - .get_mut(self.add_shortcut.editing.unwrap()) + .get_mut(self.add_shortcut.capturing_key.unwrap()) { k.0 = self.add_shortcut.binding.to_string(); } @@ -424,7 +427,7 @@ impl Page { } let new_id = widget::Id::unique(); - self.add_shortcut.editing = Some( + self.add_shortcut.capturing_key = Some( self.add_shortcut .keys .insert((String::new(), new_id.clone())), @@ -475,7 +478,7 @@ impl Page { let key_combination = widget::editable_input( fl!("type-key-combination"), text, - self.add_shortcut.editing == Some(id), + self.add_shortcut.capturing_key == Some(id), move |enable| Message::KeyEditing(id, enable), ) .on_focus(Message::KeyEditing(id, true)) @@ -508,9 +511,20 @@ impl Page { fn add_shortcut(&mut self, mut binding: Binding) { if let Some(action) = self.model.config_contains(&binding) { - let action_str = super::localize_action(&action); - self.replace_dialog.push((binding, action, action_str)); - return; + // NOTE: When not editing any action clearly the action is different + // than the action we're editing. + if self + .add_shortcut + .editing_action + .as_ref() + .and_then(|idx| self.model.shortcut_models.get(*idx)) + .map(|editing| editing.action != action) + .unwrap_or(true) + { + let action_str = super::localize_action(&action); + self.replace_dialog.push((binding, action, action_str)); + return; + } } binding.description = Some(self.add_shortcut.name.clone()); let new_action = Action::Spawn(self.add_shortcut.task.clone()); @@ -611,7 +625,7 @@ impl page::Page for Page { cosmic::iced::Subscription::batch(vec![ if self.add_shortcut.active - && self.add_shortcut.editing.is_some() + && self.add_shortcut.capturing_key.is_some() && self.replace_dialog.is_empty() { listen_with(|event, _, _| match event { From 196e626ee556c7f0b7aa2b76e56c03fea3c6c498 Mon Sep 17 00:00:00 2001 From: GenericConfluent Date: Thu, 13 Nov 2025 12:02:58 -0700 Subject: [PATCH 3/5] refactor(shortcuts): pull out field population for custom shortcut (pop-os/cosmic-settings#1566) Next up is deleting keybinds and fixing a bug where the updated keybinds aren't correctly stored when editing. Also need to change the context drawer title so it isn't add when modifying. --- .../pages/input/keyboard/shortcuts/custom.rs | 61 ++++++++++--------- 1 file changed, 32 insertions(+), 29 deletions(-) diff --git a/cosmic-settings/src/pages/input/keyboard/shortcuts/custom.rs b/cosmic-settings/src/pages/input/keyboard/shortcuts/custom.rs index e99c23167..206f88007 100644 --- a/cosmic-settings/src/pages/input/keyboard/shortcuts/custom.rs +++ b/cosmic-settings/src/pages/input/keyboard/shortcuts/custom.rs @@ -84,11 +84,14 @@ struct AddShortcut { } impl AddShortcut { - pub fn enable(&mut self) { + pub fn enable(&mut self, editing: Option) { self.active = true; + self.editing_action = editing; + + // If we are getting ShowShortcut shortcut should probably exist. + // But if it doesn't we'll just skip filling. self.name.clear(); self.task.clear(); - self.editing_action = None; if self.keys.is_empty() { self.keys.insert((String::new(), widget::Id::unique())); @@ -100,6 +103,29 @@ impl AddShortcut { self.keys[0].0.clear(); } } + + pub fn populate(&mut self, shortcut_model: &ShortcutModel) { + // FIXME: Should Cow these to get rid of the clones + self.name = shortcut_model.description.clone(); + + if let Action::Spawn(task) = &shortcut_model.action { + self.task = task.clone(); + } + + self.keys.clear(); + for (_, shortcut_binding) in &shortcut_model.bindings { + if shortcut_binding.binding.is_set() { + self.keys + .insert((shortcut_binding.binding.to_string(), widget::Id::unique())); + } + } + + if self.keys.is_empty() { + self.keys.insert((String::default(), widget::Id::unique())); + } + + self.binding = Binding::default(); + } } impl Page { @@ -213,35 +239,12 @@ impl Page { Message::Shortcut(message) => { if let ShortcutMessage::ShowShortcut(id, _description) = &message { - self.add_shortcut.active = true; - self.add_shortcut.editing_action = Some(*id); + self.add_shortcut.enable(Some(*id)); + // If we are getting ShowShortcut shortcut should probably exist. // But if it doesn't we'll just skip filling. if let Some(shortcut_model) = self.model.shortcut_models.get(*id) { - // FIXME: Should Cow these to get rid of the clones - self.add_shortcut.name = shortcut_model.description.clone(); - - if let Action::Spawn(task) = &shortcut_model.action { - self.add_shortcut.task = task.clone(); - } - - self.add_shortcut.keys.clear(); - for (_, shortcut_binding) in &shortcut_model.bindings { - if shortcut_binding.binding.is_set() { - self.add_shortcut.keys.insert(( - shortcut_binding.binding.to_string(), - widget::Id::unique(), - )); - } - } - - if self.add_shortcut.keys.is_empty() { - self.add_shortcut - .keys - .insert((String::default(), widget::Id::unique())); - } - - self.add_shortcut.binding = Binding::default(); + self.add_shortcut.populate(shortcut_model); } } @@ -250,7 +253,7 @@ impl Page { Message::ShortcutContext => { let name_id = self.name_id.clone(); - self.add_shortcut.enable(); + self.add_shortcut.enable(None); return Task::batch(vec![ cosmic::task::message(crate::app::Message::OpenContextDrawer(self.entity)), // XX hack: wait a bit before focusing the input to avoid it being ignored before it exists From 1f2f8b33d3d49504bb6409743abf6a449fd28b4e Mon Sep 17 00:00:00 2001 From: GenericConfluent Date: Thu, 13 Nov 2025 14:53:02 -0700 Subject: [PATCH 4/5] feat(shortcuts): save modified custom shortcut on drawer close (pop-os/cosmic-settings#1566) Seems to mostly work aside from not being able to delete bindings from a shortcut. Maybe we want to save immediately on every edit but that is a matter of discussion for the PR. I encountered an issue of not being able to use the bindings that are being created, but I checked the config file and things are definetly being saved properly so I assume it has to do with the older cosmic-comp version I'm running. --- .../pages/input/keyboard/shortcuts/custom.rs | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/cosmic-settings/src/pages/input/keyboard/shortcuts/custom.rs b/cosmic-settings/src/pages/input/keyboard/shortcuts/custom.rs index 206f88007..ea1794643 100644 --- a/cosmic-settings/src/pages/input/keyboard/shortcuts/custom.rs +++ b/cosmic-settings/src/pages/input/keyboard/shortcuts/custom.rs @@ -512,6 +512,33 @@ impl Page { .into() } + /// Save all bindings for a custom shortcut when in edit mode + fn save_custom_shortcut(&mut self) { + if let Some(editing_id) = self.add_shortcut.editing_action { + if let Some(model) = self.model.shortcut_models.get(editing_id) { + // Throw away the old bindings. + for (_, old_binding) in &model.bindings { + if old_binding.binding.is_set() { + self.model.config_remove(&old_binding.binding); + } + } + + let new_action = Action::Spawn(self.add_shortcut.task.clone()); + for (_, (binding_str, _)) in &self.add_shortcut.keys { + if let Ok(mut new_binding) = Binding::from_str(binding_str) { + if new_binding.is_set() { + new_binding.description = Some(self.add_shortcut.name.clone()); + self.model.config_add(new_action.clone(), new_binding); + } + } + } + + _ = self.model.on_enter(); + } + } + } + + /// Add a single binding to a custom shortcut (used in create mode during key capture) fn add_shortcut(&mut self, mut binding: Binding) { if let Some(action) = self.model.config_contains(&binding) { // NOTE: When not editing any action clearly the action is different @@ -601,6 +628,12 @@ impl page::Page for Page { } fn on_context_drawer_close(&mut self) -> Task { + // This saves our various edits if applicable. Note that by contrast when creating our + // bindings are saved as they are created. + if self.add_shortcut.editing_action.is_some() { + self.save_custom_shortcut(); + } + self.model.on_context_drawer_close(); Task::none() } From 67be733c125645340a6da883e908b988aeca0de3 Mon Sep 17 00:00:00 2001 From: GenericConfluent Date: Thu, 13 Nov 2025 16:58:40 -0700 Subject: [PATCH 5/5] feat(shortcuts): delete keybind from custom create drawer (pop-os/cosmic-settings#1566) --- .../pages/input/keyboard/shortcuts/custom.rs | 43 ++++++++++++------- 1 file changed, 28 insertions(+), 15 deletions(-) diff --git a/cosmic-settings/src/pages/input/keyboard/shortcuts/custom.rs b/cosmic-settings/src/pages/input/keyboard/shortcuts/custom.rs index ea1794643..f6475b532 100644 --- a/cosmic-settings/src/pages/input/keyboard/shortcuts/custom.rs +++ b/cosmic-settings/src/pages/input/keyboard/shortcuts/custom.rs @@ -11,7 +11,7 @@ use cosmic::iced::keyboard::key::Named; use cosmic::iced::keyboard::{Key, Location, Modifiers}; use cosmic::iced::{Alignment, Length}; use cosmic::iced_winit; -use cosmic::widget::{self, button, icon}; +use cosmic::widget::{self, button, icon, settings}; use cosmic::{Apply, Element, Task}; use cosmic_settings_config::Binding; use cosmic_settings_config::shortcuts::{Action, Shortcuts}; @@ -47,6 +47,8 @@ pub enum Message { AddKeybinding, /// Add a new custom shortcut to the config AddShortcut, + /// Delete a key binding + DeleteKeybinding(usize), /// Update the Task text input TaskInput(String), /// Toggle editing of the key text input @@ -93,15 +95,9 @@ impl AddShortcut { self.name.clear(); self.task.clear(); - if self.keys.is_empty() { - self.keys.insert((String::new(), widget::Id::unique())); - } else { - while self.keys.len() > 1 { - self.keys.remove(self.keys.len() - 1); - } - - self.keys[0].0.clear(); - } + // Clear all keys and add one empty binding + self.keys.clear(); + self.keys.insert((String::new(), widget::Id::unique())); } pub fn populate(&mut self, shortcut_model: &ShortcutModel) { @@ -196,6 +192,17 @@ impl Page { } } + Message::DeleteKeybinding(id) => { + self.add_shortcut.keys.remove(id); + + // Ensure at least one binding input exists + if self.add_shortcut.keys.is_empty() { + self.add_shortcut + .keys + .insert((String::new(), widget::Id::unique())); + } + } + Message::EditCombination => { if let Some((slab_index, (_, id))) = self.add_shortcut.keys.iter().next() { self.add_shortcut.capturing_key = Some(slab_index); @@ -489,11 +496,17 @@ impl Page { .on_input(move |input| Message::KeyInput(id, input)) .on_submit(|_| Message::AddKeybinding) .padding([0, 12]) - .id(widget_id.clone()) - .apply(widget::container) - .padding([8, 24]); + .id(widget_id.clone()); + + let delete_button = widget::button::icon(icon::from_name("edit-delete-symbolic")) + .on_press(Message::DeleteKeybinding(id)); + + let row = settings::item_row(vec![key_combination.into(), delete_button.into()]) + .align_y(Alignment::Center) + .apply(widget::container) + .padding([8, 0]); - column.add(key_combination) + column.add(row) }, ); @@ -629,7 +642,7 @@ impl page::Page for Page { fn on_context_drawer_close(&mut self) -> Task { // This saves our various edits if applicable. Note that by contrast when creating our - // bindings are saved as they are created. + // bindings are saved as they are created. if self.add_shortcut.editing_action.is_some() { self.save_custom_shortcut(); }