Add tests on inventory task sorting

crates/project/src/task_inventory.rs

@@ -5,8 +5,8 @@ use std::{any::TypeId, path::Path, sync::Arc};
 use collections::{HashMap, VecDeque};
 use gpui::{AppContext, Context, Model, ModelContext, Subscription};
 use itertools::Itertools;
-use task::{Source, Task, TaskId};
+use task::{Task, TaskId, TaskSource};
-use util::post_inc;
+use util::{post_inc, NumericPrefixWithSuffix};
 
 /// Inventory tracks available tasks for a given project.
 pub struct Inventory {
@@ -15,7 +15,7 @@ pub struct Inventory {
 }
 
 struct SourceInInventory {
-    source: Model<Box<dyn Source>>,
+    source: Model<Box<dyn TaskSource>>,
     _subscription: Subscription,
     type_id: TypeId,
 }
@@ -29,7 +29,7 @@ impl Inventory {
     }
 
     /// Registers a new tasks source, that would be fetched for available tasks.
-    pub fn add_source(&mut self, source: Model<Box<dyn Source>>, cx: &mut ModelContext<Self>) {
+    pub fn add_source(&mut self, source: Model<Box<dyn TaskSource>>, cx: &mut ModelContext<Self>) {
         let _subscription = cx.observe(&source, |_, _, cx| {
             cx.notify();
         });
@@ -43,7 +43,7 @@ impl Inventory {
         cx.notify();
     }
 
-    pub fn source<T: Source>(&self) -> Option<Model<Box<dyn Source>>> {
+    pub fn source<T: TaskSource>(&self) -> Option<Model<Box<dyn TaskSource>>> {
         let target_type_id = std::any::TypeId::of::<T>();
         self.sources.iter().find_map(
             |SourceInInventory {
@@ -77,6 +77,8 @@ impl Inventory {
         } else {
             HashMap::default()
         };
+        let not_used_score = post_inc(&mut lru_score);
+
         self.sources
             .iter()
             .flat_map(|source| {
@@ -89,16 +91,20 @@ impl Inventory {
                     tasks_by_usage
                         .get(&task.id())
                         .copied()
-                        .unwrap_or_else(|| post_inc(&mut lru_score))
+                        .unwrap_or(not_used_score)
                 } else {
-                    post_inc(&mut lru_score)
+                    not_used_score
                 };
                 (task, usages)
             })
             .sorted_unstable_by(|(task_a, usages_a), (task_b, usages_b)| {
-                usages_a
+                usages_a.cmp(usages_b).then({
-                    .cmp(usages_b)
+                    NumericPrefixWithSuffix::from_numeric_prefixed_str(task_a.name())
-                    .then(task_a.name().cmp(task_b.name()))
+                        .cmp(&NumericPrefixWithSuffix::from_numeric_prefixed_str(
+                            task_b.name(),
+                        ))
+                        .then(task_a.name().cmp(task_b.name()))
+                })
             })
             .map(|(task, _)| task)
             .collect()
@@ -114,6 +120,7 @@ impl Inventory {
         })
     }
 
+    /// Registers task "usage" as being scheduled – to be used for LRU sorting when listing all tasks.
     pub fn task_scheduled(&mut self, id: TaskId) {
         self.last_scheduled_tasks.push_back(id);
         if self.last_scheduled_tasks.len() > 5_000 {
@@ -124,10 +131,234 @@ impl Inventory {
 
 #[cfg(test)]
 mod tests {
+    use std::path::PathBuf;
+
+    use gpui::TestAppContext;
+
     use super::*;
 
-    #[test]
+    #[gpui::test]
-    fn todo_kb() {
+    fn test_task_list_sorting(cx: &mut TestAppContext) {
-        todo!("TODO kb LRU tests")
+        let inventory = cx.update(Inventory::new);
+        let initial_tasks = list_task_names(&inventory, None, true, cx);
+        assert!(
+            initial_tasks.is_empty(),
+            "No tasks expected for empty inventory, but got {initial_tasks:?}"
+        );
+        let initial_tasks = list_task_names(&inventory, None, false, cx);
+        assert!(
+            initial_tasks.is_empty(),
+            "No tasks expected for empty inventory, but got {initial_tasks:?}"
+        );
+
+        inventory.update(cx, |inventory, cx| {
+            inventory.add_source(TestSource::new(vec!["3_task".to_string()], cx), cx);
+        });
+        inventory.update(cx, |inventory, cx| {
+            inventory.add_source(
+                TestSource::new(
+                    vec![
+                        "1_task".to_string(),
+                        "2_task".to_string(),
+                        "1_a_task".to_string(),
+                    ],
+                    cx,
+                ),
+                cx,
+            );
+        });
+
+        let expected_initial_state = [
+            "1_a_task".to_string(),
+            "1_task".to_string(),
+            "2_task".to_string(),
+            "3_task".to_string(),
+        ];
+        assert_eq!(
+            list_task_names(&inventory, None, false, cx),
+            &expected_initial_state,
+            "Task list without lru sorting, should be sorted alphanumerically"
+        );
+        assert_eq!(
+            list_task_names(&inventory, None, true, cx),
+            &expected_initial_state,
+            "Tasks with equal amount of usages should be sorted alphanumerically"
+        );
+
+        register_task_used(&inventory, "2_task", cx);
+        assert_eq!(
+            list_task_names(&inventory, None, false, cx),
+            &expected_initial_state,
+            "Task list without lru sorting, should be sorted alphanumerically"
+        );
+        assert_eq!(
+            list_task_names(&inventory, None, true, cx),
+            vec![
+                "2_task".to_string(),
+                "1_a_task".to_string(),
+                "1_task".to_string(),
+                "3_task".to_string()
+            ],
+        );
+
+        register_task_used(&inventory, "1_task", cx);
+        register_task_used(&inventory, "1_task", cx);
+        register_task_used(&inventory, "1_task", cx);
+        register_task_used(&inventory, "3_task", cx);
+        assert_eq!(
+            list_task_names(&inventory, None, false, cx),
+            &expected_initial_state,
+            "Task list without lru sorting, should be sorted alphanumerically"
+        );
+        assert_eq!(
+            list_task_names(&inventory, None, true, cx),
+            vec![
+                "3_task".to_string(),
+                "1_task".to_string(),
+                "2_task".to_string(),
+                "1_a_task".to_string(),
+            ],
+        );
+
+        inventory.update(cx, |inventory, cx| {
+            inventory.add_source(
+                TestSource::new(vec!["10_hello".to_string(), "11_hello".to_string()], cx),
+                cx,
+            );
+        });
+        let expected_updated_state = [
+            "1_a_task".to_string(),
+            "1_task".to_string(),
+            "2_task".to_string(),
+            "3_task".to_string(),
+            "10_hello".to_string(),
+            "11_hello".to_string(),
+        ];
+        assert_eq!(
+            list_task_names(&inventory, None, false, cx),
+            &expected_updated_state,
+            "Task list without lru sorting, should be sorted alphanumerically"
+        );
+        assert_eq!(
+            list_task_names(&inventory, None, true, cx),
+            vec![
+                "3_task".to_string(),
+                "1_task".to_string(),
+                "2_task".to_string(),
+                "1_a_task".to_string(),
+                "10_hello".to_string(),
+                "11_hello".to_string(),
+            ],
+        );
+
+        register_task_used(&inventory, "11_hello", cx);
+        assert_eq!(
+            list_task_names(&inventory, None, false, cx),
+            &expected_updated_state,
+            "Task list without lru sorting, should be sorted alphanumerically"
+        );
+        assert_eq!(
+            list_task_names(&inventory, None, true, cx),
+            vec![
+                "11_hello".to_string(),
+                "3_task".to_string(),
+                "1_task".to_string(),
+                "2_task".to_string(),
+                "1_a_task".to_string(),
+                "10_hello".to_string(),
+            ],
+        );
+    }
+
+    #[derive(Debug, Clone, PartialEq, Eq)]
+    struct TestTask {
+        id: TaskId,
+        name: String,
+    }
+
+    impl Task for TestTask {
+        fn id(&self) -> &TaskId {
+            &self.id
+        }
+
+        fn name(&self) -> &str {
+            &self.name
+        }
+
+        fn cwd(&self) -> Option<&Path> {
+            None
+        }
+
+        fn exec(&self, _cwd: Option<PathBuf>) -> Option<task::SpawnInTerminal> {
+            None
+        }
+    }
+
+    struct TestSource {
+        tasks: Vec<TestTask>,
+    }
+
+    impl TestSource {
+        fn new(
+            task_names: impl IntoIterator<Item = String>,
+            cx: &mut AppContext,
+        ) -> Model<Box<dyn TaskSource>> {
+            cx.new_model(|_| {
+                Box::new(Self {
+                    tasks: task_names
+                        .into_iter()
+                        .enumerate()
+                        .map(|(i, name)| TestTask {
+                            id: TaskId(format!("task_{i}_{name}")),
+                            name,
+                        })
+                        .collect(),
+                }) as Box<dyn TaskSource>
+            })
+        }
+    }
+
+    impl TaskSource for TestSource {
+        fn tasks_for_path(
+            &mut self,
+            _path: Option<&Path>,
+            _cx: &mut ModelContext<Box<dyn TaskSource>>,
+        ) -> Vec<Arc<dyn Task>> {
+            self.tasks
+                .clone()
+                .into_iter()
+                .map(|task| Arc::new(task) as Arc<dyn Task>)
+                .collect()
+        }
+
+        fn as_any(&mut self) -> &mut dyn std::any::Any {
+            self
+        }
+    }
+
+    fn list_task_names(
+        inventory: &Model<Inventory>,
+        path: Option<&Path>,
+        lru: bool,
+        cx: &mut TestAppContext,
+    ) -> Vec<String> {
+        inventory.update(cx, |inventory, cx| {
+            inventory
+                .list_tasks(path, lru, cx)
+                .into_iter()
+                .map(|task| task.name().to_string())
+                .collect()
+        })
+    }
+
+    fn register_task_used(inventory: &Model<Inventory>, task_name: &str, cx: &mut TestAppContext) {
+        inventory.update(cx, |inventory, cx| {
+            let task = inventory
+                .list_tasks(None, false, cx)
+                .into_iter()
+                .find(|task| task.name() == task_name)
+                .unwrap_or_else(|| panic!("Failed to find task with name {task_name}"));
+            inventory.task_scheduled(task.id().clone());
+        });
     }
 }

crates/project_panel/src/project_panel.rs

@@ -1182,11 +1182,15 @@ impl ProjectPanel {
                                     let num_and_remainder_a = Path::new(component_a.as_os_str())
                                         .file_stem()
                                         .and_then(|s| s.to_str())
-                                        .and_then(NumericPrefixWithSuffix::from_str)?;
+                                        .and_then(
+                                            NumericPrefixWithSuffix::from_numeric_prefixed_str,
+                                        )?;
                                     let num_and_remainder_b = Path::new(component_b.as_os_str())
                                         .file_stem()
                                         .and_then(|s| s.to_str())
-                                        .and_then(NumericPrefixWithSuffix::from_str)?;
+                                        .and_then(
+                                            NumericPrefixWithSuffix::from_numeric_prefixed_str,
+                                        )?;
 
                                     num_and_remainder_a.partial_cmp(&num_and_remainder_b)
                                 });

crates/task/src/lib.rs

@@ -56,13 +56,13 @@ pub trait Task {
 ///
 /// Implementations of this trait could be e.g. [`StaticSource`] that parses tasks from a .json files and provides process templates to be spawned;
 /// another one could be a language server providing lenses with tests or build server listing all targets for a given project.
-pub trait Source: Any {
+pub trait TaskSource: Any {
     /// A way to erase the type of the source, processing and storing them generically.
     fn as_any(&mut self) -> &mut dyn Any;
     /// Collects all tasks available for scheduling, for the path given.
     fn tasks_for_path(
         &mut self,
         path: Option<&Path>,
-        cx: &mut ModelContext<Box<dyn Source>>,
+        cx: &mut ModelContext<Box<dyn TaskSource>>,
     ) -> Vec<Arc<dyn Task>>;
 }

crates/task/src/oneshot_source.rs

@@ -2,7 +2,7 @@
 
 use std::sync::Arc;
 
-use crate::{Source, SpawnInTerminal, Task, TaskId};
+use crate::{SpawnInTerminal, Task, TaskId, TaskSource};
 use gpui::{AppContext, Context, Model};
 
 /// A storage and source of tasks generated out of user command prompt inputs.
@@ -54,8 +54,8 @@ impl Task for OneshotTask {
 
 impl OneshotSource {
     /// Initializes the oneshot source, preparing to store user prompts.
-    pub fn new(cx: &mut AppContext) -> Model<Box<dyn Source>> {
+    pub fn new(cx: &mut AppContext) -> Model<Box<dyn TaskSource>> {
-        cx.new_model(|_| Box::new(Self { tasks: Vec::new() }) as Box<dyn Source>)
+        cx.new_model(|_| Box::new(Self { tasks: Vec::new() }) as Box<dyn TaskSource>)
     }
 
     /// Spawns a certain task based on the user prompt.
@@ -66,7 +66,7 @@ impl OneshotSource {
     }
 }
 
-impl Source for OneshotSource {
+impl TaskSource for OneshotSource {
     fn as_any(&mut self) -> &mut dyn std::any::Any {
         self
     }
@@ -74,7 +74,7 @@ impl Source for OneshotSource {
     fn tasks_for_path(
         &mut self,
         _path: Option<&std::path::Path>,
-        _cx: &mut gpui::ModelContext<Box<dyn Source>>,
+        _cx: &mut gpui::ModelContext<Box<dyn TaskSource>>,
     ) -> Vec<Arc<dyn Task>> {
         self.tasks.clone()
     }

crates/task/src/static_source.rs

@@ -12,7 +12,7 @@ use schemars::{gen::SchemaSettings, JsonSchema};
 use serde::{Deserialize, Serialize};
 use util::ResultExt;
 
-use crate::{Source, SpawnInTerminal, Task, TaskId};
+use crate::{SpawnInTerminal, Task, TaskId, TaskSource};
 use futures::channel::mpsc::UnboundedReceiver;
 
 /// A single config file entry with the deserialized task definition.
@@ -152,12 +152,12 @@ impl StaticSource {
     pub fn new(
         tasks_file_tracker: UnboundedReceiver<String>,
         cx: &mut AppContext,
-    ) -> Model<Box<dyn Source>> {
+    ) -> Model<Box<dyn TaskSource>> {
         let definitions = TrackedFile::new(DefinitionProvider::default(), tasks_file_tracker, cx);
         cx.new_model(|cx| {
             let _subscription = cx.observe(
                 &definitions,
-                |source: &mut Box<(dyn Source + 'static)>, new_definitions, cx| {
+                |source: &mut Box<(dyn TaskSource + 'static)>, new_definitions, cx| {
                     if let Some(static_source) = source.as_any().downcast_mut::<Self>() {
                         static_source.tasks = new_definitions
                             .read(cx)
@@ -181,11 +181,11 @@ impl StaticSource {
     }
 }
 
-impl Source for StaticSource {
+impl TaskSource for StaticSource {
     fn tasks_for_path(
         &mut self,
         _: Option<&Path>,
-        _: &mut ModelContext<Box<dyn Source>>,
+        _: &mut ModelContext<Box<dyn TaskSource>>,
     ) -> Vec<Arc<dyn Task>> {
         self.tasks
             .clone()

crates/util/src/util.rs

@@ -497,9 +497,9 @@ impl<T: Ord + Clone> RangeExt<T> for RangeInclusive<T> {
 pub struct NumericPrefixWithSuffix<'a>(i32, &'a str);
 
 impl<'a> NumericPrefixWithSuffix<'a> {
-    pub fn from_str(str: &'a str) -> Option<Self> {
+    pub fn from_numeric_prefixed_str(str: &'a str) -> Option<Self> {
         let mut chars = str.chars();
-        let prefix: String = chars.by_ref().take_while(|c| c.is_digit(10)).collect();
+        let prefix: String = chars.by_ref().take_while(|c| c.is_ascii_digit()).collect();
         let remainder = chars.as_str();
 
         match prefix.parse::<i32>() {
@@ -514,7 +514,7 @@ impl Ord for NumericPrefixWithSuffix<'_> {
         let NumericPrefixWithSuffix(num_a, remainder_a) = self;
         let NumericPrefixWithSuffix(num_b, remainder_b) = other;
         num_a
-            .cmp(&num_b)
+            .cmp(num_b)
             .then_with(|| UniCase::new(remainder_a).cmp(&UniCase::new(remainder_b)))
     }
 }
@@ -569,7 +569,7 @@ mod tests {
     fn test_numeric_prefix_with_suffix() {
         let mut sorted = vec!["1-abc", "10", "11def", "2", "21-abc"];
         sorted.sort_by_key(|s| {
-            NumericPrefixWithSuffix::from_str(s).unwrap_or_else(|| {
+            NumericPrefixWithSuffix::from_numeric_prefixed_str(s).unwrap_or_else(|| {
                 panic!("Cannot convert string `{s}` into NumericPrefixWithSuffix")
             })
         });
@@ -577,7 +577,7 @@ mod tests {
 
         for numeric_prefix_less in ["numeric_prefix_less", "aaa", "~™£"] {
             assert_eq!(
-                NumericPrefixWithSuffix::from_str(numeric_prefix_less),
+                NumericPrefixWithSuffix::from_numeric_prefixed_str(numeric_prefix_less),
                 None,
                 "String without numeric prefix `{numeric_prefix_less}` should not be converted into NumericPrefixWithSuffix"
             )