Add title box to `Picker` #8155

ArchUsr64 · 2023-09-03T13:28:33Z

Fixes #8139
The current implementation of the picker doesn't display any context information about what it's displaying.
This PR adds a title field to the Picker struct and implements the necessary logic for proper rendering of the title box based on the config option PickerTitle.

pub struct Picker<T: Item> {
...
    /// Title for the picker box
    title: String,
}

pub enum PickerTitle {
    /// Don't render the title
    #[default]
    Never,
    /// Render on top of picker box
    Center,
    /// Render inline with the picker's border
    Inline,
    /// Render inline with the picker's border
    InlineBorder,
    /// Render title in the prompt line
    Prompt,
}

This changes the rendering of a picker as follows:

                `inline`                 `inline-border`           `center`

                                                                  +-------+
                                  +-------+                       | title |
         +-title-+-+ +---------+  | title +-+ +---------+  +------+--+ +--+------+
         | prompt  | | preview |  | prompt  | | preview |  | prompt  | | preview |
         |---------| |         |  |---------| |         |  |---------| |         |
         | picker  | |         |  | picker  | |         |  | picker  | |         |
         |         | |         |  |         | |         |  |         | |         |
         |         | |         |  |         | |         |  |         | |         |
         |         | |         |  |         | |         |  |         | |         |
         |         | |         |  |         | |         |  |         | |         |
         |         | |         |  |         | |         |  |         | |         |
         +---------+ +---------+  +---------+ +---------+  +---------+ +---------+

The title is exposed to the caller via Picker::new() and Picker::with_stream() functions.
I have added the following as the titles for now:

Caller	Title	Dynamic
`global_search`	Global Search: '`user_entered_regex`' [`case_sensitive`]	✔️
`buffer_picker`	Buffer List
`jumplist_picker`	Jumplist List
`command_palette`	Command Palette
`file_picker`	Files in: `current working directory`	✔️
`sym_picker`	Symbol Picker
`diag_picker`	Diagnostics: [`errors` E] [`warnings` W] [`informations` I] [`hints` H]	✔️
`lsp_workspace_command`	LSP Workspace Commands
`goto_impl`	Goto Picker
`thread_picker`	Thread Picker
`dap_launch`	DAP Picker
`dap_switch_stack_frame`	DAP Stack Frame Picker

Screenshots

Changes to Picker:

File Picker:

Global Search Picker:

Diagnostics Picker:

Videos

Dynamic Titles:

2023-09-04_14-13-16.mp4

Rendering Options:

2023-09-09_23-07-40.mp4

Preivously, if the length of the string was even The string would underflow and the vertical bar on the line with title string would underflow

The title now shows the number of diagnostics based on severity like follows: "Diagnostics: [5 E] [0 W] [1 I] [0 H]"

The title now shows the `user_entered_regex` and if the search is case sensitive or not taking `smart_case` into account

pascalkuthe · 2023-09-03T13:46:57Z

I am not a fan of adding a title it feels unnecessary and tahkesup more vertical space. With #7265 there will be a heading for each column so at that point it gets too much

ArchUsr64 · 2023-09-03T14:11:10Z

I am not a fan of adding a title it feels unnecessary and tahkesup more vertical space. With #7265 there will be a heading for each column so at that point it gets too much

I would agree with you that the title would be redundant if it just displayed some static information but for something like global search, the additional context the title provides is quite helpful, have a look:

As for the concerns regarding limited vertical space, I'm working on an implementation that just adds title to the top of existing picker instead of shrinking the picker to make room for the title box.
There could also be a config option that disables title rendering for those who prefer the old style.

EpocSquadron · 2023-09-03T17:55:28Z

Could we not display the title inline with the bounding box at top, similar to what we do with the title for keymap modes?

ArchUsr64 · 2023-09-04T03:13:44Z

Could we not display the title inline with the bounding box at top, similar to what we do with the title for keymap modes?

This is how that could look like, I'm not sure I prefer either of them over the title box being at the center, though not wasting extra space is a plus.

This reverts commit b75bd3e.

EpocSquadron · 2023-09-04T16:02:33Z

I quite like the look of the second option, but from a minimal point of view the first is best. Curious what others think.

ArchUsr64 · 2023-09-04T16:22:22Z

I quite like the look of the second option, but from a minimal point of view the first is best. Curious what others think.

Me too though you might wanna have a look at my original post which I have since updated with the new changes to title rendering.

Would a configuration option be better? Something like picker.title_style with options of center, inline, inline_border and none?

zen3ger · 2023-09-05T11:59:27Z

Since the promp/commandline at the bottom is always empty when a picker is opened, can't we just put this information there? It wouldn't take up any extra space and would still be there for those that need it.

ArchUsr64 · 2023-09-05T15:25:36Z

Since the promp/commandline at the bottom is always empty when a picker is opened, can't we just put this information there? It wouldn't take up any extra space and would still be there for those that need it.

I don't think the title being at prompt would be very intuitive for pickers that are primarily launched via keymaps, like the file picker but I could see it working for the global search picker.

EpocSquadron · 2023-09-05T17:16:36Z

It’s definitely not intuitive to put it in the prompt, and I wouldn’t want it inconsistent from picker to picker . Global search will eventually not use the prompt anyway, once interactive global search gets done.

I say (without much authority) we go for the configurable but on by default approach, similar to how autoinfo is there by default but can be configured off by those who prefer not to have the noise. A big draw of the editor is contextual help or intuitive/unsurprising behavior.

As for which style to use, since the works already done I don’t see any harm in keeping both versions as part of configuration. My bias is again to use the prettier version as the default.

ArchUsr64 · 2023-09-09T17:57:46Z

I say (without much authority) we go for the configurable but on by default approach, similar to how autoinfo is there by default but can be configured off by those who prefer not to have the noise. A big draw of the editor is contextual help or intuitive/unsurprising behavior.

I agree, it's probably the best to let users decide for themselves. For now, I have implemented the configuration options for all the suggestions presented above namely: center, inline, inline-border and prompt. Though I have set the default to never for now.
The top post has been updated with all the relevant details if you wanna have a look.

poliorcetics · 2023-09-12T11:20:24Z

book/src/configuration.md

+
+| Key | Description | Default |
+|--|--|---------|
+| `picker-title` | Renders a title for the opening picker. Can be `never`, `center`, `inline`, `inline-border` or `prompt` | `never` |


The formulation here can make a non-fluent reader think that the title will be never, center, ...

What do you think about:

Suggested change

| `picker-title` | Renders a title for the opening picker. Can be `never`, `center`, `inline`, `inline-border` or `prompt` | `never` |

| `title-format` | How the picker's title is displayed. Can be `never`, `center`, `inline`, `inline-border` or `prompt` | `never` |

poliorcetics · 2023-09-12T11:22:36Z

helix-term/src/commands.rs

+                        let picker_title = format!(
+                            "Global Search: '{}' {}",
+                            regex,
+                            if case_sensitive {
+                                "[Case Sensitive]"
+                            } else {
+                                "[Case Insensitive]"
+                            }
+                        );


With a long regex, this will be quite unreadable, the length of the displayed regex should be limited to the N (to determine) first chars of it.

poliorcetics · 2023-09-12T11:24:53Z

helix-term/src/commands/lsp.rs


+    let picker_title = format!(
+        "Diagnostics: [{} E] [{} W] [{} I] [{} H]",


I think just: {} E, {} W, {} I, {} H will be enough and have less sigils to read. Commas are easier to skip than [] too

poliorcetics · 2023-09-12T11:26:19Z

helix-term/src/ui/mod.rs

-            cx.editor.set_error(err);
-        }
-    })
+    let picker_title = format!("Files in {:?}", root.as_os_str());


This will also have the length problem and should probably be collapsed, only expand the last N directories, if it reaches the width limit of the windows

poliorcetics · 2023-09-12T11:27:53Z

helix-term/src/ui/picker.rs

+                surface.set_string(
+                    // Add two for spacing for border box and margin
+                    area.x + 2,
+                    area.y + 1,
+                    &self.title,
+                    text_style,
+                );


Yes I think the picker title will overflow if it is too long, I remember hitting this with inline diagnostics

pascalkuthe · 2024-04-13T17:52:33Z

I am going to close this PR. This also already has conflicts and will become very hard to rebase once #9647 is merged. I don't think we want to add this feature in this form. This is a mimor visual feature where I just don't think it's worth or necessary to invest the effort.

Thank you for contributing.

ArchUsr64 added 9 commits September 3, 2023 16:18

Added title field to Picker struct

651ea81

Moved picker by TITLE_BOX_HEIGHT offset

b75bd3e

Implemented rendering for title box

530a382

Added margin for title

b491792

Fixed title string underflow

37438e8

Preivously, if the length of the string was even The string would underflow and the vertical bar on the line with title string would underflow

Added context aware title for LSP Diagnostics

cdeec0e

The title now shows the number of diagnostics based on severity like follows: "Diagnostics: [5 E] [0 W] [1 I] [0 H]"

Added search path to file_picker's title

7a2b2c3

Added context aware title for Global Search

7892ffc

The title now shows the `user_entered_regex` and if the search is case sensitive or not taking `smart_case` into account

Fixed confusing ASCII diagram

01184ca

ArchUsr64 added 7 commits September 4, 2023 11:07

Moved title box to the top

ea8a066

This reverts commit b75bd3e.

Added bounds check for title rendering

783f2ac

Fixed improper styling for border characters

e0d7732

Fixed clippy warnings

b0a9d11

Added configuration option to disable title rendering

3da7965

Documented the new picker.title option

c387c84

Changed title text style to Bold

88b1d3b

ArchUsr64 marked this pull request as ready for review September 4, 2023 08:52

ArchUsr64 added 4 commits September 9, 2023 22:12

Added other rendering modes to PickerTitle

6d09a12

Moved render area calculation to render_title

a7aab4a

Added title rendering for other modes

813ea79

Added bounds check

2707358

Added documentation for picker-title option

2577c8e

poliorcetics reviewed Sep 12, 2023

View reviewed changes

paholg mentioned this pull request Sep 21, 2023

Provide file-picker navigation #6116

Closed

zaucy mentioned this pull request Oct 17, 2023

add picker config #8561

Closed

pascalkuthe closed this Apr 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add title box to `Picker` #8155

Add title box to `Picker` #8155

ArchUsr64 commented Sep 3, 2023 •

edited

Loading

pascalkuthe commented Sep 3, 2023

ArchUsr64 commented Sep 3, 2023 •

edited

Loading

EpocSquadron commented Sep 3, 2023

ArchUsr64 commented Sep 4, 2023

EpocSquadron commented Sep 4, 2023

ArchUsr64 commented Sep 4, 2023

zen3ger commented Sep 5, 2023

ArchUsr64 commented Sep 5, 2023

EpocSquadron commented Sep 5, 2023

ArchUsr64 commented Sep 9, 2023

poliorcetics Sep 12, 2023

poliorcetics Sep 12, 2023

poliorcetics Sep 12, 2023

poliorcetics Sep 12, 2023

poliorcetics Sep 12, 2023

pascalkuthe commented Apr 13, 2024

	\| `picker-title` \| Renders a title for the opening picker. Can be `never`, `center`, `inline`, `inline-border` or `prompt` \| `never` \|
	\| `title-format` \| How the picker's title is displayed. Can be `never`, `center`, `inline`, `inline-border` or `prompt` \| `never` \|


		let picker_title = format!(
		"Diagnostics: [{} E] [{} W] [{} I] [{} H]",

Add title box to Picker #8155

Add title box to Picker #8155

Conversation

ArchUsr64 commented Sep 3, 2023 • edited Loading

Screenshots

Changes to Picker:

File Picker:

Global Search Picker:

Diagnostics Picker:

Videos

Dynamic Titles:

Rendering Options:

pascalkuthe commented Sep 3, 2023

ArchUsr64 commented Sep 3, 2023 • edited Loading

EpocSquadron commented Sep 3, 2023

ArchUsr64 commented Sep 4, 2023

EpocSquadron commented Sep 4, 2023

ArchUsr64 commented Sep 4, 2023

zen3ger commented Sep 5, 2023

ArchUsr64 commented Sep 5, 2023

EpocSquadron commented Sep 5, 2023

ArchUsr64 commented Sep 9, 2023

poliorcetics Sep 12, 2023

Choose a reason for hiding this comment

poliorcetics Sep 12, 2023

Choose a reason for hiding this comment

poliorcetics Sep 12, 2023

Choose a reason for hiding this comment

poliorcetics Sep 12, 2023

Choose a reason for hiding this comment

poliorcetics Sep 12, 2023

Choose a reason for hiding this comment

pascalkuthe commented Apr 13, 2024

Add title box to `Picker` #8155

Add title box to `Picker` #8155

ArchUsr64 commented Sep 3, 2023 •

edited

Loading

ArchUsr64 commented Sep 3, 2023 •

edited

Loading