From 385b398808b38dc7837e7e4516a9137c5b9feed0 Mon Sep 17 00:00:00 2001
From: Michael Davis <mcarsondavis@gmail.com>
Date: Sun, 3 Sep 2023 14:47:17 -0500
Subject: [PATCH] Add column configurations for existing pickers

This removes the menu::Item implementations for picker item types and
adds `Vec<Column<T, D>>` configurations.
---
 helix-term/src/commands.rs       | 429 ++++++++++++++++---------------
 helix-term/src/commands/dap.rs   |  54 ++--
 helix-term/src/commands/lsp.rs   | 274 +++++++++++---------
 helix-term/src/commands/typed.rs |   7 +-
 helix-term/src/ui/menu.rs        |  14 +-
 helix-term/src/ui/mod.rs         |  10 +-
 6 files changed, 398 insertions(+), 390 deletions(-)

diff --git a/helix-term/src/commands.rs b/helix-term/src/commands.rs
index 5600a1e4..f4d2c634 100644
--- a/helix-term/src/commands.rs
+++ b/helix-term/src/commands.rs
@@ -10,10 +10,7 @@ use helix_stdx::{
 };
 use helix_vcs::{FileChange, Hunk};
 pub use lsp::*;
-use tui::{
-    text::Span,
-    widgets::{Cell, Row},
-};
+use tui::text::Span;
 pub use typed::*;
 
 use helix_core::{
@@ -61,8 +58,7 @@ use crate::{
     compositor::{self, Component, Compositor},
     filter_picker_entry,
     job::Callback,
-    keymap::ReverseKeymap,
-    ui::{self, menu::Item, overlay::overlaid, Picker, Popup, Prompt, PromptEvent},
+    ui::{self, overlay::overlaid, Picker, PickerColumn, Popup, Prompt, PromptEvent},
 };
 
 use crate::job::{self, Jobs};
@@ -2246,32 +2242,15 @@ fn global_search(cx: &mut Context) {
         path: PathBuf,
         /// 0 indexed lines
         line_num: usize,
+        line_content: String,
     }
 
     impl FileResult {
-        fn new(path: &Path, line_num: usize) -> Self {
+        fn new(path: &Path, line_num: usize, line_content: String) -> Self {
             Self {
                 path: path.to_path_buf(),
                 line_num,
-            }
-        }
-    }
-
-    impl ui::menu::Item for FileResult {
-        type Data = Option<PathBuf>;
-
-        fn format(&self, current_path: &Self::Data) -> Row {
-            let relative_path = helix_stdx::path::get_relative_path(&self.path)
-                .to_string_lossy()
-                .into_owned();
-            if current_path
-                .as_ref()
-                .map(|p| p == &self.path)
-                .unwrap_or(false)
-            {
-                format!("{} (*)", relative_path).into()
-            } else {
-                relative_path.into()
+                line_content,
             }
         }
     }
@@ -2317,8 +2296,28 @@ fn global_search(cx: &mut Context) {
                     return;
                 }
 
-                // TODO
-                let columns = vec![];
+                let columns = vec![
+                    PickerColumn::new(
+                        "path",
+                        |item: &FileResult, current_path: &Option<PathBuf>| {
+                            let relative_path = helix_stdx::path::get_relative_path(&item.path)
+                                .to_string_lossy()
+                                .into_owned();
+                            if current_path
+                                .as_ref()
+                                .map(|p| p == &item.path)
+                                .unwrap_or(false)
+                            {
+                                format!("{} (*)", relative_path).into()
+                            } else {
+                                relative_path.into()
+                            }
+                        },
+                    ),
+                    PickerColumn::new("contents", |item: &FileResult, _| {
+                        item.line_content.as_str().into()
+                    }),
+                ];
                 let (picker, injector) = Picker::stream(columns, current_path);
 
                 let dedup_symlinks = file_picker_config.deduplicate_links;
@@ -2345,77 +2344,79 @@ fn global_search(cx: &mut Context) {
                         .max_depth(file_picker_config.max_depth)
                         .filter_entry(move |entry| {
                             filter_picker_entry(entry, &absolute_root, dedup_symlinks)
-                        });
-
-                    walk_builder
-                        .add_custom_ignore_filename(helix_loader::config_dir().join("ignore"));
-                    walk_builder.add_custom_ignore_filename(".helix/ignore");
-
-                    walk_builder.build_parallel().run(|| {
-                        let mut searcher = searcher.clone();
-                        let matcher = matcher.clone();
-                        let injector = injector_.clone();
-                        let documents = &documents;
-                        Box::new(move |entry: Result<DirEntry, ignore::Error>| -> WalkState {
-                            let entry = match entry {
-                                Ok(entry) => entry,
-                                Err(_) => return WalkState::Continue,
-                            };
-
-                            match entry.file_type() {
-                                Some(entry) if entry.is_file() => {}
-                                // skip everything else
-                                _ => return WalkState::Continue,
-                            };
-
-                            let mut stop = false;
-                            let sink = sinks::UTF8(|line_num, _| {
-                                stop = injector
-                                    .push(FileResult::new(entry.path(), line_num as usize - 1))
-                                    .is_err();
-
-                                Ok(!stop)
-                            });
-                            let doc = documents.iter().find(|&(doc_path, _)| {
-                                doc_path
-                                    .as_ref()
-                                    .map_or(false, |doc_path| doc_path == entry.path())
-                            });
-
-                            let result = if let Some((_, doc)) = doc {
-                                // there is already a buffer for this file
-                                // search the buffer instead of the file because it's faster
-                                // and captures new edits without requiring a save
-                                if searcher.multi_line_with_matcher(&matcher) {
-                                    // in this case a continous buffer is required
-                                    // convert the rope to a string
-                                    let text = doc.to_string();
-                                    searcher.search_slice(&matcher, text.as_bytes(), sink)
-                                } else {
-                                    searcher.search_reader(
-                                        &matcher,
-                                        RopeReader::new(doc.slice(..)),
-                                        sink,
-                                    )
-                                }
-                            } else {
-                                searcher.search_path(&matcher, entry.path(), sink)
-                            };
-
-                            if let Err(err) = result {
-                                log::error!(
-                                    "Global search error: {}, {}",
-                                    entry.path().display(),
-                                    err
-                                );
-                            }
-                            if stop {
-                                WalkState::Quit
-                            } else {
-                                WalkState::Continue
-                            }
                         })
-                    });
+                        .add_custom_ignore_filename(helix_loader::config_dir().join("ignore"))
+                        .add_custom_ignore_filename(".helix/ignore")
+                        .build_parallel()
+                        .run(|| {
+                            let mut searcher = searcher.clone();
+                            let matcher = matcher.clone();
+                            let injector = injector_.clone();
+                            let documents = &documents;
+                            Box::new(move |entry: Result<DirEntry, ignore::Error>| -> WalkState {
+                                let entry = match entry {
+                                    Ok(entry) => entry,
+                                    Err(_) => return WalkState::Continue,
+                                };
+
+                                match entry.file_type() {
+                                    Some(entry) if entry.is_file() => {}
+                                    // skip everything else
+                                    _ => return WalkState::Continue,
+                                };
+
+                                let mut stop = false;
+                                let sink = sinks::UTF8(|line_num, line_content| {
+                                    stop = injector
+                                        .push(FileResult::new(
+                                            entry.path(),
+                                            line_num as usize - 1,
+                                            line_content.to_string(),
+                                        ))
+                                        .is_err();
+
+                                    Ok(!stop)
+                                });
+                                let doc = documents.iter().find(|&(doc_path, _)| {
+                                    doc_path
+                                        .as_ref()
+                                        .map_or(false, |doc_path| doc_path == entry.path())
+                                });
+
+                                let result = if let Some((_, doc)) = doc {
+                                    // there is already a buffer for this file
+                                    // search the buffer instead of the file because it's faster
+                                    // and captures new edits without requiring a save
+                                    if searcher.multi_line_with_matcher(&matcher) {
+                                        // in this case a continous buffer is required
+                                        // convert the rope to a string
+                                        let text = doc.to_string();
+                                        searcher.search_slice(&matcher, text.as_bytes(), sink)
+                                    } else {
+                                        searcher.search_reader(
+                                            &matcher,
+                                            RopeReader::new(doc.slice(..)),
+                                            sink,
+                                        )
+                                    }
+                                } else {
+                                    searcher.search_path(&matcher, entry.path(), sink)
+                                };
+
+                                if let Err(err) = result {
+                                    log::error!(
+                                        "Global search error: {}, {}",
+                                        entry.path().display(),
+                                        err
+                                    );
+                                }
+                                if stop {
+                                    WalkState::Quit
+                                } else {
+                                    WalkState::Continue
+                                }
+                            })
+                        });
                 });
 
                 cx.jobs.callback(async move {
@@ -2424,7 +2425,7 @@ fn global_search(cx: &mut Context) {
                             picker,
                             0,
                             injector,
-                            move |cx, FileResult { path, line_num }, action| {
+                            move |cx, FileResult { path, line_num, .. }, action| {
                                 let doc = match cx.editor.open(path, action) {
                                     Ok(id) => doc_mut!(cx.editor, &id),
                                     Err(e) => {
@@ -2456,7 +2457,7 @@ fn global_search(cx: &mut Context) {
                             },
                         )
                         .with_preview(
-                            |_editor, FileResult { path, line_num }| {
+                            |_editor, FileResult { path, line_num, .. }| {
                                 Some((path.clone().into(), Some((*line_num, *line_num))))
                             },
                         );
@@ -2897,31 +2898,6 @@ fn buffer_picker(cx: &mut Context) {
         focused_at: std::time::Instant,
     }
 
-    impl ui::menu::Item for BufferMeta {
-        type Data = ();
-
-        fn format(&self, _data: &Self::Data) -> Row {
-            let path = self
-                .path
-                .as_deref()
-                .map(helix_stdx::path::get_relative_path);
-            let path = match path.as_deref().and_then(Path::to_str) {
-                Some(path) => path,
-                None => SCRATCH_BUFFER_NAME,
-            };
-
-            let mut flags = String::new();
-            if self.is_modified {
-                flags.push('+');
-            }
-            if self.is_current {
-                flags.push('*');
-            }
-
-            Row::new([self.id.to_string(), flags, path.to_string()])
-        }
-    }
-
     let new_meta = |doc: &Document| BufferMeta {
         id: doc.id(),
         path: doc.path().cloned(),
@@ -2940,8 +2916,31 @@ fn buffer_picker(cx: &mut Context) {
     // mru
     items.sort_unstable_by_key(|item| std::cmp::Reverse(item.focused_at));
 
-    let columns = vec![];
-    let picker = Picker::new(columns, 0, items, (), |cx, meta, action| {
+    let columns = vec![
+        PickerColumn::new("id", |meta: &BufferMeta, _| meta.id.to_string().into()),
+        PickerColumn::new("flags", |meta: &BufferMeta, _| {
+            let mut flags = String::new();
+            if meta.is_modified {
+                flags.push('+');
+            }
+            if meta.is_current {
+                flags.push('*');
+            }
+            flags.into()
+        }),
+        PickerColumn::new("path", |meta: &BufferMeta, _| {
+            let path = meta
+                .path
+                .as_deref()
+                .map(helix_stdx::path::get_relative_path);
+            path.as_deref()
+                .and_then(Path::to_str)
+                .unwrap_or(SCRATCH_BUFFER_NAME)
+                .to_string()
+                .into()
+        }),
+    ];
+    let picker = Picker::new(columns, 2, items, (), |cx, meta, action| {
         cx.editor.switch(meta.id, action);
     })
     .with_preview(|editor, meta| {
@@ -2965,33 +2964,6 @@ fn jumplist_picker(cx: &mut Context) {
         is_current: bool,
     }
 
-    impl ui::menu::Item for JumpMeta {
-        type Data = ();
-
-        fn format(&self, _data: &Self::Data) -> Row {
-            let path = self
-                .path
-                .as_deref()
-                .map(helix_stdx::path::get_relative_path);
-            let path = match path.as_deref().and_then(Path::to_str) {
-                Some(path) => path,
-                None => SCRATCH_BUFFER_NAME,
-            };
-
-            let mut flags = Vec::new();
-            if self.is_current {
-                flags.push("*");
-            }
-
-            let flag = if flags.is_empty() {
-                "".into()
-            } else {
-                format!(" ({})", flags.join(""))
-            };
-            format!("{} {}{} {}", self.id, path, flag, self.text).into()
-        }
-    }
-
     for (view, _) in cx.editor.tree.views_mut() {
         for doc_id in view.jumps.iter().map(|e| e.0).collect::<Vec<_>>().iter() {
             let doc = doc_mut!(cx.editor, doc_id);
@@ -3018,10 +2990,37 @@ fn jumplist_picker(cx: &mut Context) {
         }
     };
 
-    let columns = vec![];
+    let columns = vec![
+        ui::PickerColumn::new("id", |item: &JumpMeta, _| item.id.to_string().into()),
+        ui::PickerColumn::new("path", |item: &JumpMeta, _| {
+            let path = item
+                .path
+                .as_deref()
+                .map(helix_stdx::path::get_relative_path);
+            path.as_deref()
+                .and_then(Path::to_str)
+                .unwrap_or(SCRATCH_BUFFER_NAME)
+                .to_string()
+                .into()
+        }),
+        ui::PickerColumn::new("flags", |item: &JumpMeta, _| {
+            let mut flags = Vec::new();
+            if item.is_current {
+                flags.push("*");
+            }
+
+            if flags.is_empty() {
+                "".into()
+            } else {
+                format!(" ({})", flags.join("")).into()
+            }
+        }),
+        ui::PickerColumn::new("contents", |item: &JumpMeta, _| item.text.as_str().into()),
+    ];
+
     let picker = Picker::new(
         columns,
-        0,
+        1, // path
         cx.editor
             .tree
             .views()
@@ -3061,33 +3060,6 @@ fn changed_file_picker(cx: &mut Context) {
         style_renamed: Style,
     }
 
-    impl Item for FileChange {
-        type Data = FileChangeData;
-
-        fn format(&self, data: &Self::Data) -> Row {
-            let process_path = |path: &PathBuf| {
-                path.strip_prefix(&data.cwd)
-                    .unwrap_or(path)
-                    .display()
-                    .to_string()
-            };
-
-            let (sign, style, content) = match self {
-                Self::Untracked { path } => ("[+]", data.style_untracked, process_path(path)),
-                Self::Modified { path } => ("[~]", data.style_modified, process_path(path)),
-                Self::Conflict { path } => ("[x]", data.style_conflict, process_path(path)),
-                Self::Deleted { path } => ("[-]", data.style_deleted, process_path(path)),
-                Self::Renamed { from_path, to_path } => (
-                    "[>]",
-                    data.style_renamed,
-                    format!("{} -> {}", process_path(from_path), process_path(to_path)),
-                ),
-            };
-
-            Row::new([Cell::from(Span::styled(sign, style)), Cell::from(content)])
-        }
-    }
-
     let cwd = helix_stdx::env::current_working_dir();
     if !cwd.exists() {
         cx.editor
@@ -3101,7 +3073,40 @@ fn changed_file_picker(cx: &mut Context) {
     let deleted = cx.editor.theme.get("diff.minus");
     let renamed = cx.editor.theme.get("diff.delta.moved");
 
+    let columns = vec![
+        PickerColumn::new("change", |change: &FileChange, data: &FileChangeData| {
+            match change {
+                FileChange::Untracked { .. } => Span::styled("+ untracked", data.style_untracked),
+                FileChange::Modified { .. } => Span::styled("~ modified", data.style_modified),
+                FileChange::Conflict { .. } => Span::styled("x conflict", data.style_conflict),
+                FileChange::Deleted { .. } => Span::styled("- deleted", data.style_deleted),
+                FileChange::Renamed { .. } => Span::styled("> renamed", data.style_renamed),
+            }
+            .into()
+        }),
+        PickerColumn::new("path", |change: &FileChange, data: &FileChangeData| {
+            let display_path = |path: &PathBuf| {
+                path.strip_prefix(&data.cwd)
+                    .unwrap_or(path)
+                    .display()
+                    .to_string()
+            };
+            match change {
+                FileChange::Untracked { path } => display_path(path),
+                FileChange::Modified { path } => display_path(path),
+                FileChange::Conflict { path } => display_path(path),
+                FileChange::Deleted { path } => display_path(path),
+                FileChange::Renamed { from_path, to_path } => {
+                    format!("{} -> {}", display_path(from_path), display_path(to_path))
+                }
+            }
+            .into()
+        }),
+    ];
+
     let picker = Picker::new(
+        columns,
+        1, // path
         Vec::new(),
         FileChangeData {
             cwd: cwd.clone(),
@@ -3139,35 +3144,6 @@ fn changed_file_picker(cx: &mut Context) {
     cx.push_layer(Box::new(overlaid(picker)));
 }
 
-impl ui::menu::Item for MappableCommand {
-    type Data = ReverseKeymap;
-
-    fn format(&self, keymap: &Self::Data) -> Row {
-        let fmt_binding = |bindings: &Vec<Vec<KeyEvent>>| -> String {
-            bindings.iter().fold(String::new(), |mut acc, bind| {
-                if !acc.is_empty() {
-                    acc.push(' ');
-                }
-                for key in bind {
-                    acc.push_str(&key.key_sequence_format());
-                }
-                acc
-            })
-        };
-
-        match self {
-            MappableCommand::Typable { doc, name, .. } => match keymap.get(name as &String) {
-                Some(bindings) => format!("{} ({}) [:{}]", doc, fmt_binding(bindings), name).into(),
-                None => format!("{} [:{}]", doc, name).into(),
-            },
-            MappableCommand::Static { doc, name, .. } => match keymap.get(*name) {
-                Some(bindings) => format!("{} ({}) [{}]", doc, fmt_binding(bindings), name).into(),
-                None => format!("{} [{}]", doc, name).into(),
-            },
-        }
-    }
-}
-
 pub fn command_palette(cx: &mut Context) {
     let register = cx.register;
     let count = cx.count;
@@ -3187,7 +3163,34 @@ pub fn command_palette(cx: &mut Context) {
                 }
             }));
 
-            let columns = vec![];
+            let columns = vec![
+                ui::PickerColumn::new("name", |item, _| match item {
+                    MappableCommand::Typable { name, .. } => format!(":{name}").into(),
+                    MappableCommand::Static { name, .. } => (*name).into(),
+                }),
+                ui::PickerColumn::new(
+                    "bindings",
+                    |item: &MappableCommand, keymap: &crate::keymap::ReverseKeymap| {
+                        keymap
+                            .get(item.name())
+                            .map(|bindings| {
+                                bindings.iter().fold(String::new(), |mut acc, bind| {
+                                    if !acc.is_empty() {
+                                        acc.push(' ');
+                                    }
+                                    for key in bind {
+                                        acc.push_str(&key.key_sequence_format());
+                                    }
+                                    acc
+                                })
+                            })
+                            .unwrap_or_default()
+                            .into()
+                    },
+                ),
+                ui::PickerColumn::new("doc", |item: &MappableCommand, _| item.doc().into()),
+            ];
+
             let picker = Picker::new(columns, 0, commands, keymap, move |cx, command, _action| {
                 let mut ctx = Context {
                     register,
diff --git a/helix-term/src/commands/dap.rs b/helix-term/src/commands/dap.rs
index da2b60da..14cb83d2 100644
--- a/helix-term/src/commands/dap.rs
+++ b/helix-term/src/commands/dap.rs
@@ -12,7 +12,7 @@ use helix_view::editor::Breakpoint;
 
 use serde_json::{to_value, Value};
 use tokio_stream::wrappers::UnboundedReceiverStream;
-use tui::{text::Spans, widgets::Row};
+use tui::text::Spans;
 
 use std::collections::HashMap;
 use std::future::Future;
@@ -22,38 +22,6 @@ use anyhow::{anyhow, bail};
 
 use helix_view::handlers::dap::{breakpoints_changed, jump_to_stack_frame, select_thread_id};
 
-impl ui::menu::Item for StackFrame {
-    type Data = ();
-
-    fn format(&self, _data: &Self::Data) -> Row {
-        self.name.as_str().into() // TODO: include thread_states in the label
-    }
-}
-
-impl ui::menu::Item for DebugTemplate {
-    type Data = ();
-
-    fn format(&self, _data: &Self::Data) -> Row {
-        self.name.as_str().into()
-    }
-}
-
-impl ui::menu::Item for Thread {
-    type Data = ThreadStates;
-
-    fn format(&self, thread_states: &Self::Data) -> Row {
-        format!(
-            "{} ({})",
-            self.name,
-            thread_states
-                .get(&self.id)
-                .map(|state| state.as_str())
-                .unwrap_or("unknown")
-        )
-        .into()
-    }
-}
-
 fn thread_picker(
     cx: &mut Context,
     callback_fn: impl Fn(&mut Editor, &dap::Thread) + Send + 'static,
@@ -73,7 +41,16 @@ fn thread_picker(
             let debugger = debugger!(editor);
 
             let thread_states = debugger.thread_states.clone();
-            let columns = vec![];
+            let columns = vec![
+                ui::PickerColumn::new("name", |item: &Thread, _| item.name.as_str().into()),
+                ui::PickerColumn::new("state", |item: &Thread, thread_states: &ThreadStates| {
+                    thread_states
+                        .get(&item.id)
+                        .map(|state| state.as_str())
+                        .unwrap_or("unknown")
+                        .into()
+                }),
+            ];
             let picker = Picker::new(
                 columns,
                 0,
@@ -273,7 +250,10 @@ pub fn dap_launch(cx: &mut Context) {
 
     let templates = config.templates.clone();
 
-    let columns = vec![];
+    let columns = vec![ui::PickerColumn::new(
+        "template",
+        |item: &DebugTemplate, _| item.name.as_str().into(),
+    )];
 
     cx.push_layer(Box::new(overlaid(Picker::new(
         columns,
@@ -745,7 +725,9 @@ pub fn dap_switch_stack_frame(cx: &mut Context) {
 
     let frames = debugger.stack_frames[&thread_id].clone();
 
-    let columns = vec![];
+    let columns = vec![ui::PickerColumn::new("frame", |item: &StackFrame, _| {
+        item.name.as_str().into() // TODO: include thread_states in the label
+    })];
     let picker = Picker::new(columns, 0, frames, (), move |cx, frame, _action| {
         let debugger = debugger!(cx.editor);
         // TODO: this should be simpler to find
diff --git a/helix-term/src/commands/lsp.rs b/helix-term/src/commands/lsp.rs
index bf9747a4..2292569b 100644
--- a/helix-term/src/commands/lsp.rs
+++ b/helix-term/src/commands/lsp.rs
@@ -9,10 +9,7 @@ use helix_lsp::{
     Client, LanguageServerId, OffsetEncoding,
 };
 use tokio_stream::StreamExt;
-use tui::{
-    text::{Span, Spans},
-    widgets::Row,
-};
+use tui::{text::Span, widgets::Row};
 
 use super::{align_view, push_jump, Align, Context, Editor};
 
@@ -62,69 +59,11 @@ macro_rules! language_server_with_feature {
     }};
 }
 
-impl ui::menu::Item for lsp::Location {
-    /// Current working directory.
-    type Data = PathBuf;
-
-    fn format(&self, cwdir: &Self::Data) -> Row {
-        // The preallocation here will overallocate a few characters since it will account for the
-        // URL's scheme, which is not used most of the time since that scheme will be "file://".
-        // Those extra chars will be used to avoid allocating when writing the line number (in the
-        // common case where it has 5 digits or less, which should be enough for a cast majority
-        // of usages).
-        let mut res = String::with_capacity(self.uri.as_str().len());
-
-        if self.uri.scheme() == "file" {
-            // With the preallocation above and UTF-8 paths already, this closure will do one (1)
-            // allocation, for `to_file_path`, else there will be two (2), with `to_string_lossy`.
-            let mut write_path_to_res = || -> Option<()> {
-                let path = self.uri.to_file_path().ok()?;
-                res.push_str(&path.strip_prefix(cwdir).unwrap_or(&path).to_string_lossy());
-                Some(())
-            };
-            write_path_to_res();
-        } else {
-            // Never allocates since we declared the string with this capacity already.
-            res.push_str(self.uri.as_str());
-        }
-
-        // Most commonly, this will not allocate, especially on Unix systems where the root prefix
-        // is a simple `/` and not `C:\` (with whatever drive letter)
-        write!(&mut res, ":{}", self.range.start.line + 1)
-            .expect("Will only failed if allocating fail");
-        res.into()
-    }
-}
-
 struct SymbolInformationItem {
     symbol: lsp::SymbolInformation,
     offset_encoding: OffsetEncoding,
 }
 
-impl ui::menu::Item for SymbolInformationItem {
-    /// Path to currently focussed document
-    type Data = Option<lsp::Url>;
-
-    fn format(&self, current_doc_path: &Self::Data) -> Row {
-        if current_doc_path.as_ref() == Some(&self.symbol.location.uri) {
-            self.symbol.name.as_str().into()
-        } else {
-            match self.symbol.location.uri.to_file_path() {
-                Ok(path) => {
-                    let get_relative_path = path::get_relative_path(path.as_path());
-                    format!(
-                        "{} ({})",
-                        &self.symbol.name,
-                        get_relative_path.to_string_lossy()
-                    )
-                    .into()
-                }
-                Err(_) => format!("{} ({})", &self.symbol.name, &self.symbol.location.uri).into(),
-            }
-        }
-    }
-}
-
 struct DiagnosticStyles {
     hint: Style,
     info: Style,
@@ -138,48 +77,6 @@ struct PickerDiagnostic {
     offset_encoding: OffsetEncoding,
 }
 
-impl ui::menu::Item for PickerDiagnostic {
-    type Data = (DiagnosticStyles, DiagnosticsFormat);
-
-    fn format(&self, (styles, format): &Self::Data) -> Row {
-        let mut style = self
-            .diag
-            .severity
-            .map(|s| match s {
-                DiagnosticSeverity::HINT => styles.hint,
-                DiagnosticSeverity::INFORMATION => styles.info,
-                DiagnosticSeverity::WARNING => styles.warning,
-                DiagnosticSeverity::ERROR => styles.error,
-                _ => Style::default(),
-            })
-            .unwrap_or_default();
-
-        // remove background as it is distracting in the picker list
-        style.bg = None;
-
-        let code = match self.diag.code.as_ref() {
-            Some(NumberOrString::Number(n)) => format!(" ({n})"),
-            Some(NumberOrString::String(s)) => format!(" ({s})"),
-            None => String::new(),
-        };
-
-        let path = match format {
-            DiagnosticsFormat::HideSourcePath => String::new(),
-            DiagnosticsFormat::ShowSourcePath => {
-                let path = path::get_truncated_path(&self.path);
-                format!("{}: ", path.to_string_lossy())
-            }
-        };
-
-        Spans::from(vec![
-            Span::raw(path),
-            Span::styled(&self.diag.message, style),
-            Span::styled(code, style),
-        ])
-        .into()
-    }
-}
-
 fn location_to_file_location(location: &lsp::Location) -> FileLocation {
     let path = location.uri.to_file_path().unwrap();
     let line = Some((
@@ -241,16 +138,82 @@ fn jump_to_position(
     }
 }
 
-type SymbolPicker = Picker<SymbolInformationItem, Option<lsp::Url>>;
+fn display_symbol_kind(kind: lsp::SymbolKind) -> &'static str {
+    match kind {
+        lsp::SymbolKind::FILE => "file",
+        lsp::SymbolKind::MODULE => "module",
+        lsp::SymbolKind::NAMESPACE => "namespace",
+        lsp::SymbolKind::PACKAGE => "package",
+        lsp::SymbolKind::CLASS => "class",
+        lsp::SymbolKind::METHOD => "method",
+        lsp::SymbolKind::PROPERTY => "property",
+        lsp::SymbolKind::FIELD => "field",
+        lsp::SymbolKind::CONSTRUCTOR => "construct",
+        lsp::SymbolKind::ENUM => "enum",
+        lsp::SymbolKind::INTERFACE => "interface",
+        lsp::SymbolKind::FUNCTION => "function",
+        lsp::SymbolKind::VARIABLE => "variable",
+        lsp::SymbolKind::CONSTANT => "constant",
+        lsp::SymbolKind::STRING => "string",
+        lsp::SymbolKind::NUMBER => "number",
+        lsp::SymbolKind::BOOLEAN => "boolean",
+        lsp::SymbolKind::ARRAY => "array",
+        lsp::SymbolKind::OBJECT => "object",
+        lsp::SymbolKind::KEY => "key",
+        lsp::SymbolKind::NULL => "null",
+        lsp::SymbolKind::ENUM_MEMBER => "enummem",
+        lsp::SymbolKind::STRUCT => "struct",
+        lsp::SymbolKind::EVENT => "event",
+        lsp::SymbolKind::OPERATOR => "operator",
+        lsp::SymbolKind::TYPE_PARAMETER => "typeparam",
+        _ => {
+            log::warn!("Unknown symbol kind: {:?}", kind);
+            ""
+        }
+    }
+}
+
+type SymbolPicker = Picker<SymbolInformationItem, ()>;
+
+fn sym_picker(symbols: Vec<SymbolInformationItem>, workspace: bool) -> SymbolPicker {
+    let symbol_kind_column = ui::PickerColumn::new("kind", |item: &SymbolInformationItem, _| {
+        display_symbol_kind(item.symbol.kind).into()
+    });
+
+    let columns = if workspace {
+        vec![
+            symbol_kind_column,
+            ui::PickerColumn::new("name", |item: &SymbolInformationItem, _| {
+                item.symbol.name.as_str().into()
+            })
+            .without_filtering(),
+            ui::PickerColumn::new("path", |item: &SymbolInformationItem, _| {
+                match item.symbol.location.uri.to_file_path() {
+                    Ok(path) => path::get_relative_path(path.as_path())
+                        .to_string_lossy()
+                        .to_string()
+                        .into(),
+                    Err(_) => item.symbol.location.uri.to_string().into(),
+                }
+            }),
+        ]
+    } else {
+        vec![
+            symbol_kind_column,
+            // Some symbols in the document symbol picker may have a URI that isn't
+            // the current file. It should be rare though, so we concatenate that
+            // URI in with the symbol name in this picker.
+            ui::PickerColumn::new("name", |item: &SymbolInformationItem, _| {
+                item.symbol.name.as_str().into()
+            }),
+        ]
+    };
 
-fn sym_picker(symbols: Vec<SymbolInformationItem>, current_path: Option<lsp::Url>) -> SymbolPicker {
-    // TODO: drop current_path comparison and instead use workspace: bool flag?
-    let columns = vec![];
     Picker::new(
         columns,
-        0,
+        1, // name column
         symbols,
-        current_path,
+        (),
         move |cx, item, action| {
             jump_to_location(
                 cx.editor,
@@ -270,7 +233,7 @@ enum DiagnosticsFormat {
     HideSourcePath,
 }
 
-type DiagnosticsPicker = Picker<PickerDiagnostic, (DiagnosticStyles, DiagnosticsFormat)>;
+type DiagnosticsPicker = Picker<PickerDiagnostic, DiagnosticStyles>;
 
 fn diag_picker(
     cx: &Context,
@@ -302,12 +265,50 @@ fn diag_picker(
         error: cx.editor.theme.get("error"),
     };
 
-    let columns = vec![];
+    let mut columns = vec![
+        ui::PickerColumn::new(
+            "severity",
+            |item: &PickerDiagnostic, styles: &DiagnosticStyles| {
+                match item.diag.severity {
+                    Some(DiagnosticSeverity::HINT) => Span::styled("HINT", styles.hint),
+                    Some(DiagnosticSeverity::INFORMATION) => Span::styled("INFO", styles.info),
+                    Some(DiagnosticSeverity::WARNING) => Span::styled("WARN", styles.warning),
+                    Some(DiagnosticSeverity::ERROR) => Span::styled("ERROR", styles.error),
+                    _ => Span::raw(""),
+                }
+                .into()
+            },
+        ),
+        ui::PickerColumn::new("code", |item: &PickerDiagnostic, _| {
+            match item.diag.code.as_ref() {
+                Some(NumberOrString::Number(n)) => n.to_string().into(),
+                Some(NumberOrString::String(s)) => s.as_str().into(),
+                None => "".into(),
+            }
+        }),
+        ui::PickerColumn::new("message", |item: &PickerDiagnostic, _| {
+            item.diag.message.as_str().into()
+        }),
+    ];
+    let mut primary_column = 2; // message
+
+    if format == DiagnosticsFormat::ShowSourcePath {
+        columns.insert(
+            // between message code and message
+            2,
+            ui::PickerColumn::new("path", |item: &PickerDiagnostic, _| {
+                let path = path::get_truncated_path(&item.path);
+                path.to_string_lossy().to_string().into()
+            }),
+        );
+        primary_column += 1;
+    }
+
     Picker::new(
         columns,
-        0,
+        primary_column,
         flat_diag,
-        (styles, format),
+        styles,
         move |cx,
               PickerDiagnostic {
                   path,
@@ -389,7 +390,6 @@ pub fn symbol_picker(cx: &mut Context) {
             }
         })
         .collect();
-    let current_url = doc.url();
 
     if futures.is_empty() {
         cx.editor
@@ -404,7 +404,7 @@ pub fn symbol_picker(cx: &mut Context) {
             symbols.append(&mut lsp_items);
         }
         let call = move |_editor: &mut Editor, compositor: &mut Compositor| {
-            let picker = sym_picker(symbols, current_url);
+            let picker = sym_picker(symbols, false);
             compositor.push(Box::new(overlaid(picker)))
         };
 
@@ -466,13 +466,12 @@ pub fn workspace_symbol_picker(cx: &mut Context) {
         .boxed()
     };
 
-    let current_url = doc.url();
     let initial_symbols = get_symbols("".to_owned(), cx.editor);
 
     cx.jobs.callback(async move {
         let symbols = initial_symbols.await?;
         let call = move |_editor: &mut Editor, compositor: &mut Compositor| {
-            let picker = sym_picker(symbols, current_url);
+            let picker = sym_picker(symbols, true);
             let dyn_picker = DynamicPicker::new(picker, Box::new(get_symbols));
             compositor.push(Box::new(overlaid(dyn_picker)))
         };
@@ -753,13 +752,6 @@ pub fn code_action(cx: &mut Context) {
     });
 }
 
-impl ui::menu::Item for lsp::Command {
-    type Data = ();
-    fn format(&self, _data: &Self::Data) -> Row {
-        self.title.as_str().into()
-    }
-}
-
 pub fn execute_lsp_command(
     editor: &mut Editor,
     language_server_id: LanguageServerId,
@@ -829,7 +821,37 @@ fn goto_impl(
         }
         [] => unreachable!("`locations` should be non-empty for `goto_impl`"),
         _locations => {
-            let columns = vec![];
+            let columns = vec![ui::PickerColumn::new(
+                "location",
+                |item: &lsp::Location, cwdir: &std::path::PathBuf| {
+                    // The preallocation here will overallocate a few characters since it will account for the
+                    // URL's scheme, which is not used most of the time since that scheme will be "file://".
+                    // Those extra chars will be used to avoid allocating when writing the line number (in the
+                    // common case where it has 5 digits or less, which should be enough for a cast majority
+                    // of usages).
+                    let mut res = String::with_capacity(item.uri.as_str().len());
+
+                    if item.uri.scheme() == "file" {
+                        // With the preallocation above and UTF-8 paths already, this closure will do one (1)
+                        // allocation, for `to_file_path`, else there will be two (2), with `to_string_lossy`.
+                        if let Ok(path) = item.uri.to_file_path() {
+                            res.push_str(
+                                &path.strip_prefix(cwdir).unwrap_or(&path).to_string_lossy(),
+                            );
+                        }
+                    } else {
+                        // Never allocates since we declared the string with this capacity already.
+                        res.push_str(item.uri.as_str());
+                    }
+
+                    // Most commonly, this will not allocate, especially on Unix systems where the root prefix
+                    // is a simple `/` and not `C:\` (with whatever drive letter)
+                    write!(&mut res, ":{}", item.range.start.line + 1)
+                        .expect("Will only failed if allocating fail");
+                    res.into()
+                },
+            )];
+
             let picker = Picker::new(columns, 0, locations, cwdir, move |cx, location, action| {
                 jump_to_location(cx.editor, location, offset_encoding, action)
             })
diff --git a/helix-term/src/commands/typed.rs b/helix-term/src/commands/typed.rs
index 232e5846..f65f9657 100644
--- a/helix-term/src/commands/typed.rs
+++ b/helix-term/src/commands/typed.rs
@@ -1404,7 +1404,12 @@ fn lsp_workspace_command(
         let callback = async move {
             let call: job::Callback = Callback::EditorCompositor(Box::new(
                 move |_editor: &mut Editor, compositor: &mut Compositor| {
-                    let columns = vec![];
+                    let columns = vec![ui::PickerColumn::new(
+                        "title",
+                        |(_ls_id, command): &(_, helix_lsp::lsp::Command), _| {
+                            command.title.as_str().into()
+                        },
+                    )];
                     let picker = ui::Picker::new(
                         columns,
                         0,
diff --git a/helix-term/src/ui/menu.rs b/helix-term/src/ui/menu.rs
index c5006f95..1520924c 100644
--- a/helix-term/src/ui/menu.rs
+++ b/helix-term/src/ui/menu.rs
@@ -1,4 +1,4 @@
-use std::{borrow::Cow, cmp::Reverse, path::PathBuf};
+use std::{borrow::Cow, cmp::Reverse};
 
 use crate::{
     compositor::{Callback, Component, Compositor, Context, Event, EventResult},
@@ -31,18 +31,6 @@ pub trait Item: Sync + Send + 'static {
     }
 }
 
-impl Item for PathBuf {
-    /// Root prefix to strip.
-    type Data = PathBuf;
-
-    fn format(&self, root_path: &Self::Data) -> Row {
-        self.strip_prefix(root_path)
-            .unwrap_or(self)
-            .to_string_lossy()
-            .into()
-    }
-}
-
 pub type MenuCallback<T> = Box<dyn Fn(&mut Editor, Option<&T>, MenuEvent)>;
 
 pub struct Menu<T: Item> {
diff --git a/helix-term/src/ui/mod.rs b/helix-term/src/ui/mod.rs
index 01b718d4..4f6b031d 100644
--- a/helix-term/src/ui/mod.rs
+++ b/helix-term/src/ui/mod.rs
@@ -219,7 +219,15 @@ pub fn file_picker(root: PathBuf, config: &helix_view::editor::Config) -> FilePi
     });
     log::debug!("file_picker init {:?}", Instant::now().duration_since(now));
 
-    let columns = vec![];
+    let columns = vec![PickerColumn::new(
+        "path",
+        |item: &PathBuf, root: &PathBuf| {
+            item.strip_prefix(root)
+                .unwrap_or(item)
+                .to_string_lossy()
+                .into()
+        },
+    )];
     let picker = Picker::new(
         columns,
         0,