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.

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

File picker config #988

Merged
merged 18 commits into from
Nov 20, 2021
Merged

Conversation

dannasessha
Copy link
Contributor

Fixes #280

Connects settings from user defined config in cargo.toml to file picker by adding a second parameter to file_picker().

This is an extensible pattern for more user defined file picker configuration options, including the option to see or ignore dotfiles, etc.

Currently sets config for file picker to hide or display files listed in .gitignore.

Sets default preference to see files listed in .gitignore.

@dannasessha
Copy link
Contributor Author

May be relevant to #983.

@dannasessha
Copy link
Contributor Author

#648 May also be relevant.

helix-term/src/ui/mod.rs Outdated Show resolved Hide resolved
@dannasessha dannasessha changed the title DRAFT: File picker config File picker config Nov 9, 2021
@@ -61,6 +61,7 @@ pub struct Config {
pub completion_trigger_len: u8,
/// Whether to display infoboxes. Defaults to true.
pub auto_info: bool,
pub hide_gitignore: bool,
Copy link
Member

Choose a reason for hiding this comment

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

Let's match the name on walk builder: git_ignore. This also needs to be documented in the book/

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 think I have addressed these changes on commit 45c131c.

@@ -93,13 +93,14 @@ pub fn regex_prompt(
)
}

pub fn file_picker(root: PathBuf) -> FilePicker<PathBuf> {
pub fn file_picker(root: PathBuf, config: &helix_view::editor::Config) -> FilePicker<PathBuf> {
use ignore::{types::TypesBuilder, WalkBuilder};
use std::time;

// We want to exclude files that the editor can't handle yet
let mut type_builder = TypesBuilder::new();
let mut walk_builder = WalkBuilder::new(&root);
Copy link
Member

Choose a reason for hiding this comment

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

There's another WalkBuilder in commands.rs used for global search, we should pass this option there too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, thank you for review, I will look into this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In commit 339325f I added a variable to allow for passing into .git_ignore() argument without fiddling with explicit lifetimes, and then inserted this call right after WalkBuilder::new(...) is called.

The remaining changes were all due to formatting, using Helix with rust-analyser.

book/src/configuration.md Outdated Show resolved Hide resolved
@@ -92,6 +94,7 @@ impl Default for Config {
idle_timeout: Duration::from_millis(400),
completion_trigger_len: 2,
auto_info: true,
git_ignore: false,
Copy link
Member

Choose a reason for hiding this comment

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

I don't know where @pickfire's comment went but this should indeed be true to match the current default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, thank you for your note.

I am seeing with this, and also the next comment as saying you'd like to define defaults based on the ignore crate, specifically leaving in the default filters.

@dannasessha dannasessha changed the title File picker config WIP: File picker config Nov 12, 2021
Self {
// could simply use .standard_filters if these are uniform.
// Enables ignoring hidden files.
hidden: false,
Copy link
Member

Choose a reason for hiding this comment

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

The defaults should match the WalkBuilder defaults.

hidden: true, ignore: true, git-ignore: true, git-global: true, git-exclude: true, follow-links: false, max-depth: None

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 will set defaults to match the ignore crate's in my forthcoming commits.

@@ -23,6 +23,15 @@ To override global configuration parameters, create a `config.toml` file located
| `idle-timeout` | Time in milliseconds since last keypress before idle timers trigger. Used for autocompletion, set to 0 for instant. | `400` |
| `completion-trigger-len` | The min-length of word under cursor to trigger autocompletion | `2` |
| `auto-info` | Whether to display infoboxes | `true` |
| `file-picker` | Sets options for file picker and global search. Multiple pairs in an inline table. Details below. | `{hidden = true, parents = true, ignore = false, git-ignore = true, git-global = true, git-exclude = true } |
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to move this to an "[editor.file-picker] section" like the "[editor] section" above and have the same key, description, default table.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I will make a commit for this today. Thank you!

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 think that the commit 80ba0e0 has the changes you were looking for!

@dannasessha dannasessha changed the title WIP: File picker config File picker config Nov 14, 2021
Comment on lines 52 to 68
// IgnoreOptions
// Enables ignoring hidden files.
hidden: true,
// Enables reading ignore files from parent directories.
parents: true,
// Enables reading `.ignore` files.
ignore: true,
// Enables reading `.gitignore` files.
/// Whether to hide files in .gitignore from displaying in file picker. Defaults to false.
git_ignore: true,
// Enables reading global .gitignore, whose path is specified in git's config: `core.excludefile` option.
git_global: true,
// Enables reading `.git/info/exclude` files.
git_exclude: true,
// WalkBuilder options
// Maximum Depth to recurse.
max_depth: None,
Copy link
Member

Choose a reason for hiding this comment

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

These comments should really be doc comments (///) on the FilePickerConfig struct, that way they show in crate docs.

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, I saw that was the way with previous config, now that makes sense. I'll make an adjustment.

Comment on lines 100 to 101
/// Whether to hide files in .gitignore from displaying in file picker. Defaults to false.
//pub git_ignore: bool,
Copy link
Member

Choose a reason for hiding this comment

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

This should be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very good.

Comment on lines +1420 to +1425
match all_matches_sx_cl
.send((line_num as usize - 1, dent.path().to_path_buf()))
{
Ok(_) => Ok(true),
Err(_) => Ok(false),
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
match all_matches_sx_cl
.send((line_num as usize - 1, dent.path().to_path_buf()))
{
Ok(_) => Ok(true),
Err(_) => Ok(false),
}
Ok(all_matches_sx_cl
.send((line_num as usize - 1, dent.path().to_path_buf()))
.is_ok())

This can probably be simplified, but probably needs reformatting.

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 do not believe I changed this logic in the course of this PR--the formatting changed due to the addition of the fluent-style calls above (lines 1392-1398).

Here are the same lines in master.

Comment on lines 39 to 75
pub struct FilePickerConfig {
/// IgnoreOptions
/// Enables ignoring hidden files.
/// Whether to hide, that is, to stop hidden files from displaying in file picker. Defaults to false.
pub hidden: bool,
/// Enables reading ignore files from parent directories.
pub parents: bool,
/// Enables reading `.ignore` files.
/// Whether to hide files listed in .ignore from displaying in file picker. Defaults to false.
pub ignore: bool,
/// Enables reading `.gitignore` files.
/// Whether to hide files in .gitignore from displaying in file picker. Defaults to false.
pub git_ignore: bool,
/// Enables reading global .gitignore, whose path is specified in git's config: `core.excludefile` option.
/// Whether to hide files in global .gitignore from displaying in file picker. Defaults to false.
pub git_global: bool,
/// Enables reading `.git/info/exclude` files.
/// Whether to hide files in .git/info/exclude from displaying in file picker. Defaults to false.
pub git_exclude: bool,
/// WalkBuilder options
/// Maximum Depth to recurse.
pub max_depth: Option<usize>,
}

impl Default for FilePickerConfig {
fn default() -> Self {
Self {
hidden: true,
parents: true,
ignore: true,
git_ignore: true,
git_global: true,
git_exclude: true,
max_depth: None,
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The comments seemed confusing to me. It's good to put the defaults in comments, but it doesn't seemed to match the defaults it code.

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 made some updates to these comments and to the book, hope they are more clear and accurate!

Copy link
Member

Choose a reason for hiding this comment

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

I think we should get rid of mentioning defaults in comments altogether since it could get outdated, and for the book we could do something like #1119.

@archseer archseer merged commit 6a4d969 into helix-editor:master Nov 20, 2021
@fraunos
Copy link

fraunos commented Nov 20, 2021

Does this change allow to set gitignore file picker behaviour in config.toml only? Is there some flag/option in file picker during file picking itself?

@dannasessha
Copy link
Contributor Author

Does this change allow to set gitignore file picker behaviour in config.toml only? Is there some flag/option in file picker during file picking itself?

I am personally very interested in the potential direction of making the file picker more interactive (change views/config, sort, move through directories or even tree-type views) as well as more informative (metadata). I would love to have a robust discussion of this, here or elsewhere, see what community interest is as well as compatibility with the vision for helix.

@sudormrfbin
Copy link
Member

@dannasessha I also had some thoughts about formatting in the picker, it's best to open an issue or discussion to discuss properly.

@dannasessha
Copy link
Contributor Author

Does this change allow to set gitignore file picker behaviour in config.toml only? Is there some flag/option in file picker during file picking itself?

@dannasessha I also had some thoughts about formatting in the picker, it's best to open an issue or discussion to discuss properly.

I think #1163 is the main spot right for talk about the file picker now!

@AjBreidenbach
Copy link

AjBreidenbach commented Jun 18, 2024

see #10221 for current discussion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

File picker should have an option to display ignored files
7 participants