Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom file picker width max min and factor #1089

Closed
wants to merge 1 commit into from
Closed

Custom file picker width max min and factor #1089

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions book/src/configuration.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,11 @@ To override global configuration parameters, create a `config.toml` file located
|`git-global` | Enables reading global .gitignore, whose path is specified in git's config: `core.excludefile` option. | true
|`git-exclude` | Enables reading `.git/info/exclude` files. | true
|`max-depth` | Set with an integer value for maximum depth to recurse. | Defaults to `None`.
|`width-factor` | File picker width, treated as a factor. | `0.9` |
|`height-factor` | File picker height, treated as a factor. | `0.9` |
|`preview-width-factor` | File picker preview width, treated as a factor. | `0.5` |
|`preview-min-width` | File picker preview minimum width in columns. | `60` |
|`file-min-width` | File picker filenames minimum width in columns when preview is showed. | `20` |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filenames minimum width seemed confusing to me?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i know my English sucks ;-), any suggestion?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File picker preview minimum width in columns?

Copy link
Contributor Author

@Rational-Curiosity Rational-Curiosity Nov 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

+----------------+-------------------+
| file-min-width | preview-min-width |
| filenames...   | code...           |
+----------------+-------------------+

file-min-width is the other side.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how to explain this too, hopefully someone can help.


## LSP

Expand Down
42 changes: 33 additions & 9 deletions helix-term/src/ui/picker.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ use tui::widgets::Widget;

use std::{
borrow::Cow,
cmp,
collections::HashMap,
io::Read,
path::{Path, PathBuf},
Expand All @@ -28,7 +29,6 @@ use helix_view::{
Document, Editor,
};

pub const MIN_SCREEN_WIDTH_FOR_PREVIEW: u16 = 80;
/// Biggest file size to preview in bytes
pub const MAX_FILE_SIZE_FOR_PREVIEW: u64 = 10 * 1024 * 1024;

Expand Down Expand Up @@ -159,16 +159,32 @@ impl<T: 'static> Component for FilePicker<T> {
// |picker | | |
// | | | |
// +---------+ +---------+
let render_preview = area.width > MIN_SCREEN_WIDTH_FOR_PREVIEW;
let area = inner_rect(area);
let area = inner_rect(
area,
cx.editor.config.file_picker.width_factor,
cx.editor.config.file_picker.height_factor,
Comment on lines +164 to +165
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pass in &cx.editor.config.file_picker instead

);
let render_preview = area.width
> cx.editor.config.file_picker.preview_min_width
+ cx.editor.config.file_picker.file_min_width;
Comment on lines +168 to +169
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit repetitive, add let config = &cx.editor.config.file_picker at the top of render

// -- Render the frame:
// clear area
let background = cx.editor.theme.get("ui.background");
let text = cx.editor.theme.get("ui.text");
surface.clear_with(area, background);

let picker_width = if render_preview {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be less confusing to have the config as preview-width instead of picker-width since I would assume picker-width to mean the width of the entire window (including the preview and the picker).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But preview is the right side not always showed. Maybe we have to choose a name for the left side of the picker. Suggestions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then I think we can have two different options - file-picker.width and file-picker.preview-width. width can control the whole picker + preview window size and preview-width can control the size of the preview inside this window.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done :-)

area.width / 2
let preview_width = (area.width as f32
* cx.editor.config.file_picker.preview_width_factor)
.round() as u16;
pickfire marked this conversation as resolved.
Show resolved Hide resolved
cmp::max(
area.width
- cmp::max(
preview_width,
cx.editor.config.file_picker.preview_min_width,
),
cx.editor.config.file_picker.file_min_width,
)
Comment on lines +180 to +187
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than cmp::max(a, b) a.max(b) can be used. In this case a.max(b).max(c).

} else {
area.width
};
Expand Down Expand Up @@ -388,10 +404,10 @@ impl<T> Picker<T> {
// - on input change:
// - score all the names in relation to input

fn inner_rect(area: Rect) -> Rect {
fn inner_rect(area: Rect, width_factor: f32, height_factor: f32) -> Rect {
let margin = Margin {
vertical: area.height * 10 / 100,
horizontal: area.width * 10 / 100,
vertical: (area.height as f32 * (1.0 - width_factor)).round() as u16,
horizontal: (area.width as f32 * (1.0 - height_factor)).round() as u16,
Comment on lines +409 to +410
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using floats feels wrong to me, why not specify a percentage from 0-100?

};
area.inner(&margin)
}
Expand Down Expand Up @@ -453,7 +469,11 @@ impl<T: 'static> Component for Picker<T> {

fn render(&mut self, area: Rect, surface: &mut Surface, cx: &mut Context) {
let area = if self.render_centered {
inner_rect(area)
inner_rect(
area,
cx.editor.config.file_picker.width_factor,
cx.editor.config.file_picker.height_factor,
)
} else {
area
};
Expand Down Expand Up @@ -534,7 +554,11 @@ impl<T: 'static> Component for Picker<T> {

fn cursor(&self, area: Rect, editor: &Editor) -> (Option<Position>, CursorKind) {
// TODO: this is mostly duplicate code
let area = inner_rect(area);
let area = inner_rect(
area,
editor.config.file_picker.width_factor,
editor.config.file_picker.height_factor,
);
let block = Block::default().borders(Borders::ALL);
// calculate the inner area inside the box
let inner = block.inner(area);
Expand Down
17 changes: 17 additions & 0 deletions helix-view/src/editor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,16 @@ pub struct FilePickerConfig {
/// WalkBuilder options
/// Maximum Depth to recurse directories in file picker and global search. Defaults to `None`.
pub max_depth: Option<usize>,
/// File picker width, treated as a factor. Defaults to 0.9.
pub width_factor: f32,
/// File picker height, treated as a factor. Defaults to 0.9.
pub height_factor: f32,
/// File picker preview width, treated as a factor. Defaults to 0.5.
pub preview_width_factor: f32,
/// File picker preview minimum width in columns. Default to 60.
pub preview_min_width: u16,
/// File picker filenames minimum width in columns when preview is showed. Default to 20.
pub file_min_width: u16,
}

impl Default for FilePickerConfig {
Expand All @@ -72,6 +82,13 @@ impl Default for FilePickerConfig {
git_global: true,
git_exclude: true,
max_depth: None,
width_factor: 0.9,
height_factor: 0.9,
preview_width_factor: 0.5,
// Default values `preview_min_width` and `file_min_width`
// should total up to 80 to fit 80 column terminals.
preview_min_width: 60,
file_min_width: 20,
Comment on lines +90 to +91
Copy link
Contributor

@pickfire pickfire Nov 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add a comment that both of this should total up to 80.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Out of curiosity, what is the reason? preview-min-width = 25, file-min-width = 20 works, and preview-min-width = 80, file-min-width = 30 works too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that's why you put 80? Like previously I saw a number 80 being removed, and you replaced it with the sum of these two.

At the very least I think we should still make it look nice for 80x24 terminals.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done :-)

}
}
}
Expand Down