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

simpler file modification indicator #2831

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions book/src/configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ hidden = false
| `auto-info` | Whether to display infoboxes | `true` |
| `true-color` | Set to `true` to override automatic detection of terminal truecolor support in the event of a false negative. | `false` |
| `rulers` | List of column positions at which to display the rulers. Can be overridden by language specific `rulers` in `languages.toml` file. | `[]` |
| `file-modification-indicator` | The string to be displayed in the statusbar when the file has been modified. | `"[+]"` |
| `color-modes` | Whether to color the mode indicator with different colors depending on the mode itself | `false` |

### `[editor.statusline]` Section
Expand Down
10 changes: 8 additions & 2 deletions helix-term/src/commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ use grep_searcher::{sinks, BinaryDetection, SearcherBuilder};
use ignore::{DirEntry, WalkBuilder, WalkState};
use tokio_stream::wrappers::UnboundedReceiverStream;

const DEFAULT_CURRENT_BUFFER_INDICATOR: &str = "*";

pub struct Context<'a> {
pub register: Option<char>,
pub count: Option<NonZeroUsize>,
Expand Down Expand Up @@ -2208,6 +2210,8 @@ fn buffer_picker(cx: &mut Context) {
path: Option<PathBuf>,
is_modified: bool,
is_current: bool,
modified_indicator: String,
current_indicator: String,
}

impl ui::menu::Item for BufferMeta {
Expand All @@ -2225,10 +2229,10 @@ fn buffer_picker(cx: &mut Context) {

let mut flags = Vec::new();
if self.is_modified {
flags.push("+");
flags.push(self.modified_indicator.to_string());
Copy link
Member

Choose a reason for hiding this comment

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

Note that this changes how the default looks: we used + but instead [+] will be used now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes - this requires some discussion (c.f. #2831 (comment))

}
if self.is_current {
flags.push("*");
flags.push(self.current_indicator.to_string());
Comment on lines +2232 to +2235
Copy link
Member

Choose a reason for hiding this comment

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

Just push &self.current_indicator, ``&self.modified_indicatorto avoidto_string()`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we do this then we have to modify the join on line 2241 so that it gets a Vec<String> instead of a Vec<&String>. Happy to make this change since I'm not sure which approach is more idiomatic. (Also, do you recommend an into() for this or a map()?)

}

let flag = if flags.is_empty() {
Expand All @@ -2245,6 +2249,8 @@ fn buffer_picker(cx: &mut Context) {
path: doc.path().cloned(),
is_modified: doc.is_modified(),
is_current: doc.id() == current,
modified_indicator: cx.editor.config().file_modification_indicator.clone(),
current_indicator: DEFAULT_CURRENT_BUFFER_INDICATOR.to_string(),
Comment on lines +2252 to +2253
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this is going to be a bunch of allocations per document on the list...

Is customizability here really that necessary? From what I can tell vim simply always uses [+] and I can't imagine a lot of users will change this.

Copy link
Contributor Author

@sbromberger sbromberger Jul 22, 2022

Choose a reason for hiding this comment

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

Hmm, this is going to be a bunch of allocations per document on the list...

Could you explain this further? isn't it just one (few-byte) string allocation per open document? As far as I can tell it's mapped to open documents on line 2260 and only in the buffer picker, but I'm undoubtedly missing something.

Is customizability here really that necessary?

It's certainly not necessary, but it's a nice quality-of-life that I miss from my existing nvim setup that shouldn't impact too many people. (I use a Δ to indicate changed files and leave +/-/~ for git indications.)

It also gets const strings out of the middle of multiple code blocks and centralized, which I think is very important. Right now if you want to change a symbol there are multiple places you need to go to do it.

};

let picker = FilePicker::new(
Expand Down
9 changes: 7 additions & 2 deletions helix-term/src/ui/statusline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -325,10 +325,15 @@ where
.as_ref()
.map(|p| p.to_string_lossy())
.unwrap_or_else(|| SCRATCH_BUFFER_NAME.into());
let file_modification_indicator = &context.editor.config().file_modification_indicator;
format!(
" {}{} ",
"{}{}",
path,
if context.doc.is_modified() { "[+]" } else { "" }
if context.doc.is_modified() {
file_modification_indicator
} else {
""
}
)
};

Expand Down
5 changes: 5 additions & 0 deletions helix-view/src/editor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ use serde::{ser::SerializeMap, Deserialize, Deserializer, Serialize, Serializer}

use arc_swap::access::{DynAccess, DynGuard};

const DEFAULT_FILE_MODIFICATION_INDICATOR: &str = "[+]";

fn deserialize_duration_millis<'de, D>(deserializer: D) -> Result<Duration, D::Error>
where
D: serde::Deserializer<'de>,
Expand Down Expand Up @@ -161,6 +163,8 @@ pub struct Config {
pub rulers: Vec<u16>,
#[serde(default)]
pub whitespace: WhitespaceConfig,
/// String for file modification indicator
pub file_modification_indicator: String,
/// Vertical indent width guides.
pub indent_guides: IndentGuidesConfig,
/// Whether to color modes with different colors. Defaults to `false`.
Expand Down Expand Up @@ -490,6 +494,7 @@ impl Default for Config {
lsp: LspConfig::default(),
rulers: Vec::new(),
whitespace: WhitespaceConfig::default(),
file_modification_indicator: DEFAULT_FILE_MODIFICATION_INDICATOR.to_string(),
indent_guides: IndentGuidesConfig::default(),
color_modes: false,
}
Expand Down