fix path handling on windows (#762)

* this reduces memory overhead where nothing is folded up
* makes folding work with windows path seperators
This commit is contained in:
Stephan Dilly 2021-06-03 00:06:28 +02:00 committed by GitHub
parent 0e7ac4a14c
commit 869e4b7287
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
6 changed files with 167 additions and 154 deletions

View file

@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
## Fixed
- wrong file with same name shown in file tree ([#748](https://github.com/extrawurst/gitui/issues/748))
- filetree collapsing broken on windows ([#761](https://github.com/extrawurst/gitui/issues/761))
### Internal
- use git_repository_message [[@kosayoda](https://github.com/kosayoda)] ([#751](https://github.com/extrawurst/gitui/issues/751))

View file

@ -6,9 +6,6 @@ pub enum Error {
#[error("InvalidPath: `{0}`")]
InvalidPath(PathBuf),
#[error("InvalidFilePath: `{0}`")]
InvalidFilePath(String),
#[error("TryFromInt error:{0}")]
IntConversion(#[from] TryFromIntError),
}

View file

@ -2,7 +2,7 @@ use crate::{
error::Result, filetreeitems::FileTreeItems,
tree_iter::TreeIterator, TreeItemInfo,
};
use std::{collections::BTreeSet, usize};
use std::{collections::BTreeSet, path::Path, usize};
///
#[derive(Copy, Clone, Debug)]
@ -35,7 +35,7 @@ pub struct FileTree {
impl FileTree {
///
pub fn new(
list: &[&str],
list: &[&Path],
collapsed: &BTreeSet<&String>,
) -> Result<Self> {
let mut new_self = Self {
@ -318,12 +318,12 @@ impl FileTree {
mod test {
use crate::{FileTree, MoveSelection};
use pretty_assertions::assert_eq;
use std::collections::BTreeSet;
use std::{collections::BTreeSet, path::Path};
#[test]
fn test_selection() {
let items = vec![
"a/b", //
Path::new("a/b"), //
];
let mut tree =
@ -341,8 +341,8 @@ mod test {
#[test]
fn test_selection_skips_collapsed() {
let items = vec![
"a/b/c", //
"a/d", //
Path::new("a/b/c"), //
Path::new("a/d"), //
];
//0 a/
@ -364,8 +364,8 @@ mod test {
#[test]
fn test_selection_left_collapse() {
let items = vec![
"a/b/c", //
"a/d", //
Path::new("a/b/c"), //
Path::new("a/d"), //
];
//0 a/
@ -390,8 +390,8 @@ mod test {
#[test]
fn test_selection_left_parent() {
let items = vec![
"a/b/c", //
"a/d", //
Path::new("a/b/c"), //
Path::new("a/d"), //
];
//0 a/
@ -417,8 +417,8 @@ mod test {
#[test]
fn test_selection_right_expand() {
let items = vec![
"a/b/c", //
"a/d", //
Path::new("a/b/c"), //
Path::new("a/d"), //
];
//0 a/
@ -449,8 +449,8 @@ mod test {
#[test]
fn test_selection_top() {
let items = vec![
"a/b/c", //
"a/d", //
Path::new("a/b/c"), //
Path::new("a/d"), //
];
//0 a/
@ -470,9 +470,9 @@ mod test {
#[test]
fn test_visible_selection() {
let items = vec![
"a/b/c", //
"a/b/c2", //
"a/d", //
Path::new("a/b/c"), //
Path::new("a/b/c2"), //
Path::new("a/d"), //
];
//0 a/

View file

@ -6,7 +6,7 @@ use crate::{
use crate::{error::Result, treeitems_iter::TreeItemsIterator};
use std::{
collections::{BTreeSet, HashMap},
path::Path,
path::{Path, PathBuf},
usize,
};
@ -20,7 +20,7 @@ pub struct FileTreeItems {
impl FileTreeItems {
///
pub fn new(
list: &[&str],
list: &[&Path],
collapsed: &BTreeSet<&String>,
) -> Result<Self> {
let (mut items, paths) = Self::create_items(list, collapsed)?;
@ -34,7 +34,7 @@ impl FileTreeItems {
}
fn create_items<'a>(
list: &'a [&str],
list: &'a [&Path],
collapsed: &BTreeSet<&String>,
) -> Result<(Vec<FileTreeItem>, HashMap<&'a Path, usize>)> {
// scopetime::scope_time!("create_items");
@ -45,9 +45,8 @@ impl FileTreeItems {
for e in list {
{
let item_path = Path::new(e);
Self::push_dirs(
item_path,
e,
&mut items,
&mut paths_added,
collapsed,
@ -103,13 +102,10 @@ impl FileTreeItems {
}
}
//TODO: make non alloc
let path_string = Self::path_to_string(c)?;
let is_collapsed = collapsed.contains(&path_string);
nodes.push(FileTreeItem::new_path(
c,
path_string,
is_collapsed,
)?);
nodes.push(FileTreeItem::new_path(c, is_collapsed)?);
}
}
@ -121,6 +117,7 @@ impl FileTreeItems {
Ok(())
}
//TODO: return ref
fn path_to_string(p: &Path) -> Result<String> {
Ok(p.to_str()
.map_or_else(
@ -134,9 +131,8 @@ impl FileTreeItems {
if self.tree_items[index].kind().is_path() {
self.tree_items[index].collapse_path();
let path = format!(
"{}/",
self.tree_items[index].info().full_path()
let path = PathBuf::from(
self.tree_items[index].info().full_path_str(),
);
for i in index + 1..self.tree_items.len() {
@ -146,7 +142,8 @@ impl FileTreeItems {
item.collapse_path();
}
let item_path = &item.info().full_path();
let item_path =
Path::new(item.info().full_path_str());
if item_path.starts_with(&path) {
item.hide();
@ -161,18 +158,15 @@ impl FileTreeItems {
if self.tree_items[index].kind().is_path() {
self.tree_items[index].expand_path();
let full_path = format!(
"{}/",
self.tree_items[index].info().full_path()
let full_path = PathBuf::from(
self.tree_items[index].info().full_path_str(),
);
if recursive {
for i in index + 1..self.tree_items.len() {
let item = &mut self.tree_items[i];
if !item
.info()
.full_path()
if !Path::new(item.info().full_path_str())
.starts_with(&full_path)
{
break;
@ -187,7 +181,7 @@ impl FileTreeItems {
}
self.update_visibility(
Some(full_path.as_str()),
&Some(full_path),
index + 1,
false,
);
@ -196,16 +190,18 @@ impl FileTreeItems {
fn update_visibility(
&mut self,
prefix: Option<&str>,
prefix: &Option<PathBuf>,
start_idx: usize,
set_defaults: bool,
) {
// if we are in any subpath that is collapsed we keep skipping over it
let mut inner_collapsed: Option<String> = None;
let mut inner_collapsed: Option<PathBuf> = None;
for i in start_idx..self.tree_items.len() {
if let Some(ref collapsed_path) = inner_collapsed {
let p = self.tree_items[i].info().full_path();
let p = Path::new(
self.tree_items[i].info().full_path_str(),
);
if p.starts_with(collapsed_path) {
if set_defaults {
self.tree_items[i]
@ -219,15 +215,17 @@ impl FileTreeItems {
}
let item_kind = self.tree_items[i].kind().clone();
let item_path = self.tree_items[i].info().full_path();
let item_path =
Path::new(self.tree_items[i].info().full_path_str());
if matches!(item_kind, FileTreeItemKind::Path(PathCollapsed(collapsed)) if collapsed)
{
// we encountered an inner path that is still collapsed
inner_collapsed = Some(format!("{}/", &item_path));
inner_collapsed = Some(item_path.into());
}
if prefix
.as_ref()
.map_or(true, |prefix| item_path.starts_with(prefix))
{
self.tree_items[i].info_mut().set_visible(true);
@ -251,8 +249,8 @@ impl FileTreeItems {
while i < items.len() {
let item = &items[i];
if item.kind().is_path() {
let children =
paths.get(&Path::new(item.info().full_path()));
let children = paths
.get(&Path::new(item.info().full_path_str()));
if let Some(children) = children {
if *children == 1 {
@ -271,7 +269,7 @@ impl FileTreeItems {
let prefix = item_mut
.info()
.full_path()
.full_path_str()
.to_owned();
Self::unindent(items, &prefix, i + 1);
@ -291,7 +289,7 @@ impl FileTreeItems {
start: usize,
) {
for elem in items.iter_mut().skip(start) {
if elem.info().full_path().starts_with(prefix) {
if elem.info().full_path_str().starts_with(prefix) {
elem.info_mut().unindent();
} else {
return;
@ -308,7 +306,7 @@ mod tests {
#[test]
fn test_simple() {
let items = vec![
"file.txt", //
Path::new("file.txt"), //
];
let res =
@ -320,8 +318,8 @@ mod tests {
assert_eq!(res.tree_items[0].info().full_path(), items[0]);
let items = vec![
"file.txt", //
"file2.txt", //
Path::new("file.txt"), //
Path::new("file2.txt"), //
];
let res =
@ -329,10 +327,7 @@ mod tests {
assert_eq!(res.tree_items.len(), 2);
assert_eq!(res.tree_items.len(), res.len());
assert_eq!(
res.tree_items[1].info().path(),
items[1].to_string()
);
assert_eq!(res.tree_items[1].info().path(), items[1]);
}
#[test]
@ -392,14 +387,14 @@ mod tests {
#[test]
fn test_folder() {
let items = vec![
"a/file.txt", //
Path::new("a/file.txt"), //
];
let res = FileTreeItems::new(&items, &BTreeSet::new())
.unwrap()
.tree_items
.iter()
.map(|i| i.info().full_path().to_string())
.map(|i| i.info().full_path_str().to_string())
.collect::<Vec<_>>();
assert_eq!(
@ -411,7 +406,7 @@ mod tests {
#[test]
fn test_indent() {
let items = vec![
"a/b/file.txt", //
Path::new("a/b/file.txt"), //
];
let list =
@ -421,15 +416,15 @@ mod tests {
.iter()
.map(|i| (i.info().indent(), i.info().path()));
assert_eq!(res.next(), Some((0, "a/b")));
assert_eq!(res.next(), Some((1, "file.txt")));
assert_eq!(res.next(), Some((0, Path::new("a/b"))));
assert_eq!(res.next(), Some((1, Path::new("file.txt"))));
}
#[test]
fn test_indent_folder_file_name() {
let items = vec![
"a/b", //
"a.txt", //
Path::new("a/b"), //
Path::new("a.txt"), //
];
let list =
@ -437,7 +432,7 @@ mod tests {
let mut res = list
.tree_items
.iter()
.map(|i| (i.info().indent(), i.info().path()));
.map(|i| (i.info().indent(), i.info().path_str()));
assert_eq!(res.next(), Some((0, "a")));
assert_eq!(res.next(), Some((1, "b")));
@ -447,8 +442,8 @@ mod tests {
#[test]
fn test_folder_dup() {
let items = vec![
"a/file.txt", //
"a/file2.txt", //
Path::new("a/file.txt"), //
Path::new("a/file2.txt"), //
];
let tree =
@ -460,7 +455,7 @@ mod tests {
let res = tree
.tree_items
.iter()
.map(|i| i.info().full_path().to_string())
.map(|i| i.info().full_path_str().to_string())
.collect::<Vec<_>>();
assert_eq!(
@ -476,8 +471,8 @@ mod tests {
#[test]
fn test_collapse() {
let items = vec![
"a/file1.txt", //
"b/file2.txt", //
Path::new("a/file1.txt"), //
Path::new("b/file2.txt"), //
];
let mut tree =
@ -493,8 +488,8 @@ mod tests {
#[test]
fn test_iterate_collapsed() {
let items = vec![
"a/file1.txt", //
"b/file2.txt", //
Path::new("a/file1.txt"), //
Path::new("b/file2.txt"), //
];
let mut tree =
@ -520,8 +515,8 @@ mod tests {
#[test]
fn test_expand() {
let items = vec![
"a/b/c", //
"a/d", //
Path::new("a/b/c"), //
Path::new("a/d"), //
];
//0 a/
@ -564,8 +559,8 @@ mod tests {
#[test]
fn test_expand_bug() {
let items = vec![
"a/b/c", //
"a/b2/d", //
Path::new("a/b/c"), //
Path::new("a/b2/d"), //
];
//0 a/
@ -608,8 +603,8 @@ mod tests {
#[test]
fn test_collapse_too_much() {
let items = vec![
"a/b", //
"a2/c", //
Path::new("a/b"), //
Path::new("a2/c"), //
];
//0 a/
@ -638,8 +633,8 @@ mod tests {
#[test]
fn test_expand_with_collapsed_sub_parts() {
let items = vec![
"a/b/c", //
"a/d", //
Path::new("a/b/c"), //
Path::new("a/d"), //
];
//0 a/
@ -701,7 +696,7 @@ mod test_merging {
#[test]
fn test_merge_simple() {
let list = vec!["a/b/c"];
let list = vec![Path::new("a/b/c")];
let (mut items, paths) =
FileTreeItems::create_items(&list, &BTreeSet::new())
.unwrap();
@ -716,8 +711,8 @@ mod test_merging {
#[test]
fn test_merge_simple2() {
let list = vec![
"a/b/c", //
"a/b/d",
Path::new("a/b/c"), //
Path::new("a/b/d"), //
];
let (mut items, paths) =
FileTreeItems::create_items(&list, &BTreeSet::new())
@ -736,8 +731,8 @@ mod test_merging {
#[test]
fn test_merge_indent() {
let list = vec![
"a/b/c/d", //
"a/e/f",
Path::new("a/b/c/d"), //
Path::new("a/e/f"), //
];
//0:0 a/
@ -758,7 +753,7 @@ mod test_merging {
assert_eq!(*paths.get(&Path::new("a/b/c")).unwrap(), 1);
assert_eq!(*paths.get(&Path::new("a/e")).unwrap(), 1);
FileTreeItems::fold_paths(&mut items, dbg!(&paths));
FileTreeItems::fold_paths(&mut items, &paths);
let indents: Vec<u8> =
items.iter().map(|i| i.info().indent()).collect();
@ -768,8 +763,8 @@ mod test_merging {
#[test]
fn test_merge_single_paths() {
let items = vec![
"a/b/c", //
"a/b/d", //
Path::new("a/b/c"), //
Path::new("a/b/d"), //
];
//0 a/b/
@ -781,7 +776,7 @@ mod test_merging {
let mut it = tree
.iterate(0, 10)
.map(|(_, item)| item.info().full_path());
.map(|(_, item)| item.info().full_path_str());
assert_eq!(it.next().unwrap(), "a/b");
assert_eq!(it.next().unwrap(), "a/b/c");
@ -792,8 +787,8 @@ mod test_merging {
#[test]
fn test_merge_nothing() {
let items = vec![
"a/b/c", //
"a/b2/d", //
Path::new("a/b/c"), //
Path::new("a/b2/d"), //
];
//0 a/
@ -807,7 +802,7 @@ mod test_merging {
let mut it = tree
.iterate(0, 10)
.map(|(_, item)| item.info().full_path());
.map(|(_, item)| item.info().full_path_str());
assert_eq!(it.next().unwrap(), "a");
assert_eq!(it.next().unwrap(), "a/b");

View file

@ -1,5 +1,8 @@
use crate::error::{Error, Result};
use std::{convert::TryFrom, path::Path};
use crate::error::Result;
use std::{
convert::TryFrom,
path::{Path, PathBuf},
};
/// holds the information shared among all `FileTreeItem` in a `FileTree`
#[derive(Debug, Clone)]
@ -8,23 +11,20 @@ pub struct TreeItemInfo {
indent: u8,
/// currently visible depending on the folder collapse states
visible: bool,
/// just the last path element
path: String,
/// contains this paths last component and folded up paths added to it
/// if this is `None` nothing was folding into here
folded: Option<PathBuf>,
/// the full path
full_path: String,
full_path: PathBuf,
}
impl TreeItemInfo {
///
pub const fn new(
indent: u8,
path: String,
full_path: String,
) -> Self {
pub const fn new(indent: u8, full_path: PathBuf) -> Self {
Self {
indent,
visible: true,
path,
folded: None,
full_path,
}
}
@ -35,13 +35,36 @@ impl TreeItemInfo {
}
///
pub fn full_path(&self) -> &str {
&self.full_path
//TODO: remove
pub fn full_path_str(&self) -> &str {
self.full_path.to_str().unwrap_or_default()
}
///
pub fn path(&self) -> &str {
&self.path
pub fn full_path(&self) -> &Path {
self.full_path.as_path()
}
/// like `path` but as `&str`
pub fn path_str(&self) -> &str {
self.path().as_os_str().to_str().unwrap_or_default()
}
/// returns the last component of `full_path`
/// or the last components plus folded up children paths
pub fn path(&self) -> &Path {
self.folded.as_ref().map_or_else(
|| {
Path::new(
self.full_path
.components()
.last()
.and_then(|c| c.as_os_str().to_str())
.unwrap_or_default(),
)
},
|folding| folding.as_path(),
)
}
///
@ -91,64 +114,38 @@ pub struct FileTreeItem {
}
impl FileTreeItem {
pub fn new_file(path: &str) -> Result<Self> {
let item_path = Path::new(&path);
pub fn new_file(path: &Path) -> Result<Self> {
let item_path = PathBuf::from(path);
let indent = u8::try_from(
item_path.ancestors().count().saturating_sub(2),
)?;
let filename = item_path
.file_name()
.map_or_else(
|| Err(Error::InvalidFilePath(path.to_string())),
Ok,
)?
.to_string_lossy()
.to_string();
Ok(Self {
info: TreeItemInfo::new(
indent,
filename,
item_path.to_string_lossy().to_string(),
),
info: TreeItemInfo::new(indent, item_path),
kind: FileTreeItemKind::File,
})
}
pub fn new_path(
path: &Path,
path_string: String,
collapsed: bool,
) -> Result<Self> {
pub fn new_path(path: &Path, collapsed: bool) -> Result<Self> {
let indent =
u8::try_from(path.ancestors().count().saturating_sub(2))?;
let last_path_component =
path.components().last().map_or_else(
|| Err(Error::InvalidPath(path.to_path_buf())),
Ok,
)?;
let last_path_component = last_path_component
.as_os_str()
.to_string_lossy()
.to_string();
Ok(Self {
info: TreeItemInfo::new(
indent,
last_path_component,
path_string,
),
info: TreeItemInfo::new(indent, path.to_owned()),
kind: FileTreeItemKind::Path(PathCollapsed(collapsed)),
})
}
///
pub fn fold(&mut self, next: Self) {
self.info.path =
format!("{}/{}", self.info.path, next.info.path);
if let Some(folded) = self.info.folded.as_mut() {
*folded = folded.join(next.info.path());
} else {
self.info.folded =
Some(self.info.path().join(next.info.path()))
}
self.info.full_path = next.info.full_path;
}
@ -202,6 +199,32 @@ impl PartialOrd for FileTreeItem {
impl Ord for FileTreeItem {
fn cmp(&self, other: &Self) -> std::cmp::Ordering {
self.info.path.cmp(&other.info.path)
self.info.path().cmp(other.info.path())
}
}
#[cfg(test)]
mod tests {
use super::*;
use pretty_assertions::assert_eq;
#[test]
fn test_smoke() {
let mut a =
FileTreeItem::new_path(Path::new("a"), false).unwrap();
assert_eq!(a.info.full_path_str(), "a");
assert_eq!(a.info.path_str(), "a");
let b =
FileTreeItem::new_path(Path::new("a/b"), false).unwrap();
a.fold(b);
assert_eq!(a.info.full_path_str(), "a/b");
assert_eq!(
&a.info.folded.as_ref().unwrap(),
&Path::new("a/b")
);
assert_eq!(a.info.path(), Path::new("a/b"));
}
}

View file

@ -80,11 +80,8 @@ impl RevisionFilesComponent {
self.revision.map(|c| c == commit).unwrap_or_default();
if !same_id {
self.files = sync::tree_files(CWD, commit)?;
let filenames: Vec<&str> = self
.files
.iter()
.map(|f| f.path.to_str().unwrap_or_default())
.collect();
let filenames: Vec<&Path> =
self.files.iter().map(|f| f.path.as_path()).collect();
self.tree = FileTree::new(&filenames, &BTreeSet::new())?;
self.tree.collapse_but_root();
self.revision = Some(commit);
@ -108,7 +105,7 @@ impl RevisionFilesComponent {
theme: &SharedTheme,
selected: bool,
) -> Span<'a> {
let path = item.info().path();
let path = item.info().path_str();
let indent = item.info().indent();
let indent_str = if indent == 0 {
@ -136,7 +133,7 @@ impl RevisionFilesComponent {
self.tree.selected_file().map_or(false, |file| {
self.queue.borrow_mut().push_back(
InternalEvent::BlameFile(
file.full_path()
file.full_path_str()
.strip_prefix("./")
.unwrap_or_default()
.to_string(),
@ -151,7 +148,7 @@ impl RevisionFilesComponent {
if let Some(file) = self
.tree
.selected_file()
.map(|file| file.full_path().to_string())
.map(|file| file.full_path_str().to_string())
{
let path = Path::new(&file);
if let Some(item) =