From 95e0d5ed74ced18bb69e0e501a22c586df9df8f2 Mon Sep 17 00:00:00 2001
From: Piotr Osiewicz <24362066+osiewicz@users.noreply.github.com>
Date: Tue, 14 May 2024 11:22:09 +0200
Subject: [PATCH] tasks: Reorganize task modal (#11752)
Release Notes:
- Improved tasks modal by highlighting a distinction between a task
template and concrete task instance and surfacing available keybindings
more prominently. Task templates are now always available in the modal,
even if there's already a history entry with the same label.
- Changed default key binding for "picker::UseSelectedQuery" to `opt-e`.
---
Cargo.lock | 1 +
assets/icons/history_rerun.svg | 5 ++
assets/keymaps/default-macos.json | 11 ++-
crates/languages/src/bash.rs | 1 -
crates/languages/src/python.rs | 1 -
crates/project/src/task_inventory.rs | 47 +++++++---
crates/task/src/task_template.rs | 14 ---
crates/tasks_ui/Cargo.toml | 1 +
crates/tasks_ui/src/modal.rs | 124 ++++++++++++++++++++++++---
crates/ui/src/components/icon.rs | 2 +
10 files changed, 160 insertions(+), 47 deletions(-)
create mode 100644 assets/icons/history_rerun.svg
diff --git a/Cargo.lock b/Cargo.lock
index d1072953b7074291f7b6ffdcd9ad0ab3c56846ef..44de9e533d611cc398c68cb78b800ce3f40e6b24 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -10088,6 +10088,7 @@ dependencies = [
"fuzzy",
"gpui",
"language",
+ "menu",
"picker",
"project",
"schemars",
diff --git a/assets/icons/history_rerun.svg b/assets/icons/history_rerun.svg
new file mode 100644
index 0000000000000000000000000000000000000000..530465fe750ac43e496672df13250c2be97d9428
--- /dev/null
+++ b/assets/icons/history_rerun.svg
@@ -0,0 +1,5 @@
+
diff --git a/assets/keymaps/default-macos.json b/assets/keymaps/default-macos.json
index d60c6326bb53d8e96dfb77beb01c33ff6df15537..0d03767d8f5a21196b73803202824ac8bdc64116 100644
--- a/assets/keymaps/default-macos.json
+++ b/assets/keymaps/default-macos.json
@@ -19,9 +19,6 @@
"cmd-escape": "menu::Cancel",
"ctrl-escape": "menu::Cancel",
"ctrl-c": "menu::Cancel",
- "shift-enter": "picker::UseSelectedQuery",
- "alt-enter": ["picker::ConfirmInput", { "secondary": false }],
- "cmd-alt-enter": ["picker::ConfirmInput", { "secondary": true }],
"cmd-shift-w": "workspace::CloseWindow",
"shift-escape": "workspace::ToggleZoom",
"cmd-o": "workspace::Open",
@@ -630,6 +627,14 @@
"ctrl-backspace": "tab_switcher::CloseSelectedItem"
}
},
+ {
+ "context": "Picker",
+ "bindings": {
+ "alt-e": "picker::UseSelectedQuery",
+ "alt-enter": ["picker::ConfirmInput", { "secondary": false }],
+ "cmd-alt-enter": ["picker::ConfirmInput", { "secondary": true }]
+ }
+ },
{
"context": "Terminal",
"bindings": {
diff --git a/crates/languages/src/bash.rs b/crates/languages/src/bash.rs
index 5cdcf86a581276a13e91b8830941ebf6b767f88e..5bca10aee4639a2209c0c75d85fe6f449d1d3684 100644
--- a/crates/languages/src/bash.rs
+++ b/crates/languages/src/bash.rs
@@ -6,7 +6,6 @@ pub(super) fn bash_task_context() -> ContextProviderWithTasks {
TaskTemplate {
label: "execute selection".to_owned(),
command: VariableName::SelectedText.template_value(),
- ignore_previously_resolved: true,
..TaskTemplate::default()
},
TaskTemplate {
diff --git a/crates/languages/src/python.rs b/crates/languages/src/python.rs
index 858dafb71629efadbfc7bc6c63c4d9effd508ca6..0f5e0367f55bb104f3059a947513e6191f91713c 100644
--- a/crates/languages/src/python.rs
+++ b/crates/languages/src/python.rs
@@ -187,7 +187,6 @@ pub(super) fn python_task_context() -> ContextProviderWithTasks {
label: "execute selection".to_owned(),
command: "python3".to_owned(),
args: vec!["-c".to_owned(), VariableName::SelectedText.template_value()],
- ignore_previously_resolved: true,
..TaskTemplate::default()
},
TaskTemplate {
diff --git a/crates/project/src/task_inventory.rs b/crates/project/src/task_inventory.rs
index c4bf8dd7f545afbf0e7ca717150bc05720f4376d..ca48700a69ca9103db5bfb4c0d71ce4d8c5643b8 100644
--- a/crates/project/src/task_inventory.rs
+++ b/crates/project/src/task_inventory.rs
@@ -8,7 +8,7 @@ use std::{
use collections::{btree_map, BTreeMap, VecDeque};
use gpui::{AppContext, Context, Model, ModelContext};
-use itertools::{Either, Itertools};
+use itertools::Itertools;
use language::Language;
use task::{
static_source::StaticSource, ResolvedTask, TaskContext, TaskId, TaskTemplate, VariableName,
@@ -188,7 +188,6 @@ impl Inventory {
.last_scheduled_tasks
.iter()
.rev()
- .filter(|(_, task)| !task.original_task().ignore_previously_resolved)
.filter(|(task_kind, _)| {
if matches!(task_kind, TaskSourceKind::Language { .. }) {
Some(task_kind) == task_source_kind.as_ref()
@@ -256,38 +255,46 @@ impl Inventory {
tasks_by_label
},
);
- tasks_by_label = currently_resolved_tasks.into_iter().fold(
+ tasks_by_label = currently_resolved_tasks.iter().fold(
tasks_by_label,
|mut tasks_by_label, (source, task, lru_score)| {
- match tasks_by_label.entry((source, task.resolved_label.clone())) {
+ match tasks_by_label.entry((source.clone(), task.resolved_label.clone())) {
btree_map::Entry::Occupied(mut o) => {
let (previous_task, _) = o.get();
let new_template = task.original_task();
- if new_template.ignore_previously_resolved
- || new_template != previous_task.original_task()
- {
- o.insert((task, lru_score));
+ if new_template != previous_task.original_task() {
+ o.insert((task.clone(), *lru_score));
}
}
btree_map::Entry::Vacant(v) => {
- v.insert((task, lru_score));
+ v.insert((task.clone(), *lru_score));
}
}
tasks_by_label
},
);
- tasks_by_label
+ let resolved = tasks_by_label
.into_iter()
.map(|((kind, _), (task, lru_score))| (kind, task, lru_score))
- .sorted_unstable_by(task_lru_comparator)
- .partition_map(|(kind, task, lru_score)| {
+ .sorted_by(task_lru_comparator)
+ .filter_map(|(kind, task, lru_score)| {
if lru_score < not_used_score {
- Either::Left((kind, task))
+ Some((kind, task))
} else {
- Either::Right((kind, task))
+ None
}
})
+ .collect();
+
+ (
+ resolved,
+ currently_resolved_tasks
+ .into_iter()
+ .sorted_unstable_by(task_lru_comparator)
+ .map(|(kind, task, _)| (kind, task))
+ .collect(),
+ )
}
/// Returns the last scheduled task, if any of the sources contains one with the matching id.
@@ -334,6 +341,7 @@ fn task_lru_comparator(
&task_b.resolved_label,
))
.then(task_a.resolved_label.cmp(&task_b.resolved_label))
+ .then(kind_a.cmp(kind_b))
})
}
@@ -530,6 +538,7 @@ mod tests {
assert_eq!(
resolved_task_names(&inventory, None, cx),
vec![
+ "2_task".to_string(),
"2_task".to_string(),
"1_a_task".to_string(),
"1_task".to_string(),
@@ -548,6 +557,9 @@ mod tests {
assert_eq!(
resolved_task_names(&inventory, None, cx),
vec![
+ "3_task".to_string(),
+ "1_task".to_string(),
+ "2_task".to_string(),
"3_task".to_string(),
"1_task".to_string(),
"2_task".to_string(),
@@ -578,6 +590,9 @@ mod tests {
assert_eq!(
resolved_task_names(&inventory, None, cx),
vec![
+ "3_task".to_string(),
+ "1_task".to_string(),
+ "2_task".to_string(),
"3_task".to_string(),
"1_task".to_string(),
"2_task".to_string(),
@@ -595,6 +610,10 @@ mod tests {
assert_eq!(
resolved_task_names(&inventory, None, cx),
vec![
+ "11_hello".to_string(),
+ "3_task".to_string(),
+ "1_task".to_string(),
+ "2_task".to_string(),
"11_hello".to_string(),
"3_task".to_string(),
"1_task".to_string(),
diff --git a/crates/task/src/task_template.rs b/crates/task/src/task_template.rs
index f2e974a3a2f1829c382687ffe49b083e241d7b86..efd9556366c274496a294c96d23c7a70821f9c13 100644
--- a/crates/task/src/task_template.rs
+++ b/crates/task/src/task_template.rs
@@ -39,20 +39,6 @@ pub struct TaskTemplate {
/// Whether to allow multiple instances of the same task to be run, or rather wait for the existing ones to finish.
#[serde(default)]
pub allow_concurrent_runs: bool,
- // Tasks like "execute the selection" better have the constant labels (to avoid polluting the history with temporary tasks),
- // and always use the latest context with the latest selection.
- //
- // Current impl will always pick previously spawned tasks on full label conflict in the tasks modal and terminal tabs, never
- // getting the latest selection for them.
- // This flag inverts the behavior, effectively removing all previously spawned tasks from history,
- // if their full labels are the same as the labels of the newly resolved tasks.
- // Such tasks are still re-runnable, and will use the old context in that case (unless the rerun task forces this).
- //
- // Current approach is relatively hacky, a better way is understand when the new resolved tasks needs a rerun,
- // and replace the historic task accordingly.
- #[doc(hidden)]
- #[serde(default)]
- pub ignore_previously_resolved: bool,
/// What to do with the terminal pane and tab, after the command was started:
/// * `always` — always show the terminal pane, add and focus the corresponding task's tab in it (default)
/// * `never` — avoid changing current terminal pane focus, but still add/reuse the task's tab there
diff --git a/crates/tasks_ui/Cargo.toml b/crates/tasks_ui/Cargo.toml
index ca6af81ccd4f593200b14027f59829e029ca4847..d92437d017b329c2aa3e003552f17793a5e2515e 100644
--- a/crates/tasks_ui/Cargo.toml
+++ b/crates/tasks_ui/Cargo.toml
@@ -13,6 +13,7 @@ editor.workspace = true
file_icons.workspace = true
fuzzy.workspace = true
gpui.workspace = true
+menu.workspace = true
picker.workspace = true
project.workspace = true
task.workspace = true
diff --git a/crates/tasks_ui/src/modal.rs b/crates/tasks_ui/src/modal.rs
index 1a67278620d3142d5a5c12387ed59ca7b9fd22a4..6ed524972efe526d601a455e7c114357b56fc55e 100644
--- a/crates/tasks_ui/src/modal.rs
+++ b/crates/tasks_ui/src/modal.rs
@@ -3,17 +3,18 @@ use std::sync::Arc;
use crate::active_item_selection_properties;
use fuzzy::{StringMatch, StringMatchCandidate};
use gpui::{
- impl_actions, rems, AppContext, DismissEvent, EventEmitter, FocusableView, InteractiveElement,
- Model, ParentElement, Render, SharedString, Styled, Subscription, View, ViewContext,
- VisualContext, WeakView,
+ impl_actions, rems, AnyElement, AppContext, DismissEvent, EventEmitter, FocusableView,
+ InteractiveElement, Model, ParentElement, Render, SharedString, Styled, Subscription, View,
+ ViewContext, VisualContext, WeakView,
};
use picker::{highlighted_match_with_paths::HighlightedText, Picker, PickerDelegate};
use project::{Inventory, TaskSourceKind};
use task::{ResolvedTask, TaskContext, TaskTemplate};
use ui::{
- div, v_flex, ButtonCommon, ButtonSize, Clickable, Color, FluentBuilder as _, Icon, IconButton,
- IconButtonShape, IconName, IconSize, ListItem, ListItemSpacing, RenderOnce, Selectable,
- Tooltip, WindowContext,
+ div, h_flex, v_flex, ActiveTheme, Button, ButtonCommon, ButtonSize, Clickable, Color,
+ FluentBuilder as _, Icon, IconButton, IconButtonShape, IconName, IconSize, IntoElement,
+ KeyBinding, LabelSize, ListItem, ListItemSpacing, RenderOnce, Selectable, Tooltip,
+ WindowContext,
};
use util::ResultExt;
use workspace::{tasks::schedule_resolved_task, ModalView, Workspace};
@@ -87,7 +88,7 @@ impl TasksModalDelegate {
selected_index: 0,
prompt: String::default(),
task_context,
- placeholder_text: Arc::from("Run a task..."),
+ placeholder_text: Arc::from("Find a task, or run a command"),
}
}
@@ -352,12 +353,37 @@ impl PickerDelegate for TasksModalDelegate {
TaskSourceKind::Language { name } => file_icons::FileIcons::get(cx)
.get_type_icon(&name.to_lowercase())
.map(|icon_path| Icon::from_path(icon_path)),
+ }
+ .map(|icon| icon.color(Color::Muted).size(IconSize::Small));
+ let history_run_icon = if Some(ix) <= self.divider_index {
+ Some(
+ Icon::new(IconName::HistoryRerun)
+ .color(Color::Muted)
+ .size(IconSize::Small)
+ .into_any_element(),
+ )
+ } else {
+ Some(
+ v_flex()
+ .flex_none()
+ .size(IconSize::Small.rems())
+ .into_any_element(),
+ )
};
Some(
ListItem::new(SharedString::from(format!("tasks-modal-{ix}")))
- .inset(true)
+ .inset(false)
+ .start_slot::(icon)
+ .end_slot::(history_run_icon)
.spacing(ListItemSpacing::Sparse)
+ // .map(|this| {
+ // if Some(ix) <= self.divider_index {
+ // this.start_slot(Icon::new(IconName::HistoryRerun).size(IconSize::Small))
+ // } else {
+ // this.start_slot(v_flex().flex_none().size(IconSize::Small.rems()))
+ // }
+ // })
.when_some(tooltip_label, |list_item, item_label| {
list_item.tooltip(move |_| item_label.clone())
})
@@ -392,11 +418,7 @@ impl PickerDelegate for TasksModalDelegate {
} else {
item
};
- if let Some(icon) = icon {
- item.end_slot(icon)
- } else {
- item
- }
+ item
})
.selected(selected)
.child(highlighted_location.render(cx)),
@@ -429,6 +451,80 @@ impl PickerDelegate for TasksModalDelegate {
Vec::new()
}
}
+ fn render_footer(&self, cx: &mut ViewContext>) -> Option {
+ let is_recent_selected = self.divider_index >= Some(self.selected_index);
+ let current_modifiers = cx.modifiers();
+ Some(
+ h_flex()
+ .w_full()
+ .h_8()
+ .p_2()
+ .justify_between()
+ .rounded_b_md()
+ .bg(cx.theme().colors().ghost_element_selected)
+ .children(
+ KeyBinding::for_action(&picker::UseSelectedQuery, cx).map(|keybind| {
+ let edit_entry_label = if is_recent_selected {
+ "Edit task"
+ } else if !self.matches.is_empty() {
+ "Edit template"
+ } else {
+ "Rerun last task"
+ };
+
+ Button::new("edit-current-task", edit_entry_label)
+ .label_size(LabelSize::Small)
+ .key_binding(keybind)
+ }),
+ )
+ .map(|this| {
+ if current_modifiers.alt || self.matches.is_empty() {
+ this.children(
+ KeyBinding::for_action(
+ &picker::ConfirmInput {
+ secondary: current_modifiers.secondary(),
+ },
+ cx,
+ )
+ .map(|keybind| {
+ let spawn_oneshot_label = if current_modifiers.secondary() {
+ "Spawn oneshot without history"
+ } else {
+ "Spawn oneshot"
+ };
+
+ Button::new("spawn-onehshot", spawn_oneshot_label)
+ .label_size(LabelSize::Small)
+ .key_binding(keybind)
+ }),
+ )
+ } else if current_modifiers.secondary() {
+ this.children(KeyBinding::for_action(&menu::SecondaryConfirm, cx).map(
+ |keybind| {
+ let label = if is_recent_selected {
+ "Rerun without history"
+ } else {
+ "Spawn without history"
+ };
+ Button::new("spawn", label)
+ .label_size(LabelSize::Small)
+ .key_binding(keybind)
+ },
+ ))
+ } else {
+ this.children(KeyBinding::for_action(&menu::Confirm, cx).map(|keybind| {
+ let run_entry_label =
+ if is_recent_selected { "Rerun" } else { "Spawn" };
+
+ Button::new("spawn", run_entry_label)
+ .label_size(LabelSize::Small)
+ .key_binding(keybind)
+ }))
+ }
+ })
+ .into_any_element(),
+ )
+ }
}
#[cfg(test)]
@@ -787,7 +883,7 @@ mod tests {
let tasks_picker = open_spawn_tasks(&workspace, cx);
assert_eq!(
task_names(&tasks_picker, cx),
- vec!["TypeScript task from file /dir/a1.ts", "Another task from file /dir/a1.ts", "Task without variables"],
+ vec!["TypeScript task from file /dir/a1.ts", "TypeScript task from file /dir/a1.ts", "Another task from file /dir/a1.ts", "Task without variables"],
"After spawning the task and getting it into the history, it should be up in the sort as recently used"
);
tasks_picker.update(cx, |_, cx| {
diff --git a/crates/ui/src/components/icon.rs b/crates/ui/src/components/icon.rs
index 2215d3dd75826bc07e6155808bc285b912397d45..77710a230977230c951fea7282b72a31e5238ff1 100644
--- a/crates/ui/src/components/icon.rs
+++ b/crates/ui/src/components/icon.rs
@@ -182,6 +182,7 @@ pub enum IconName {
ZedXCopilot,
ZedAssistant,
PullRequest,
+ HistoryRerun,
}
impl IconName {
@@ -295,6 +296,7 @@ impl IconName {
IconName::ZedXCopilot => "icons/zed_x_copilot.svg",
IconName::ZedAssistant => "icons/zed_assistant.svg",
IconName::PullRequest => "icons/pull_request.svg",
+ IconName::HistoryRerun => "icons/history_rerun.svg",
}
}
}