-
-
Notifications
You must be signed in to change notification settings - Fork 609
Feat/respect xdg spec on all os #2627
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
base: master
Are you sure you want to change the base?
Changes from all commits
beb71fb
8e307d3
fb935b7
819b4ca
cbc388d
299015a
9ea0d0d
cde05b1
e8440c2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -49,7 +49,6 @@ pub fn process_cmdline() -> Result<CliArgs> { | |
.map_or_else(|| PathBuf::from("theme.ron"), PathBuf::from); | ||
|
||
let confpath = get_app_config_path()?; | ||
fs::create_dir_all(&confpath)?; | ||
let theme = confpath.join(arg_theme); | ||
|
||
let notify_watcher: bool = | ||
|
@@ -149,28 +148,104 @@ fn setup_logging(path_override: Option<PathBuf>) -> Result<()> { | |
Ok(()) | ||
} | ||
|
||
fn get_path_from_candidates( | ||
candidates: impl IntoIterator<Item = Option<PathBuf>>, | ||
) -> Result<PathBuf> { | ||
let mut target_dir = None; | ||
|
||
// Filter into existing directories | ||
for potential_dir in | ||
candidates.into_iter().flatten().filter(|p| p.is_dir()) | ||
Comment on lines
+157
to
+158
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, one more remark: At least for XDG, |
||
{ | ||
let search_path = potential_dir.join("gitui"); | ||
|
||
// Prefer preexisting gitui directory | ||
if search_path.is_dir() { | ||
target_dir = Some(search_path); | ||
break; | ||
} | ||
|
||
// Fallback to first existing directory | ||
target_dir.get_or_insert(search_path); | ||
} | ||
|
||
target_dir.ok_or_else(|| { | ||
anyhow!("failed to find valid path within candidates") | ||
}) | ||
} | ||
|
||
fn get_app_cache_path() -> Result<PathBuf> { | ||
let mut path = dirs::cache_dir() | ||
.ok_or_else(|| anyhow!("failed to find os cache dir."))?; | ||
let cache_dir_candidates = [ | ||
env::var_os("XDG_CACHE_HOME").map(PathBuf::from), | ||
dirs::cache_dir(), | ||
]; | ||
|
||
let cache_dir = get_path_from_candidates(cache_dir_candidates) | ||
.map_err(|_| anyhow!("failed to find os cache dir."))?; | ||
|
||
path.push("gitui"); | ||
fs::create_dir_all(&path)?; | ||
Ok(path) | ||
fs::create_dir_all(&cache_dir)?; | ||
Ok(cache_dir) | ||
} | ||
|
||
pub fn get_app_config_path() -> Result<PathBuf> { | ||
let mut path = if cfg!(target_os = "macos") { | ||
dirs::home_dir().map(|h| h.join(".config")) | ||
} else { | ||
dirs::config_dir() | ||
// List of potential config directories in order of priority | ||
let config_dir_candidates = [ | ||
env::var_os("XDG_CONFIG_HOME").map(PathBuf::from), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At least according to spec, |
||
// This is in the list since it was the hardcoded behavior on macos before | ||
// I expect this to be what most people have XDG_CONFIG_HOME set to already | ||
// But explicitly including this will avoid breaking anyone's existing config | ||
dirs::home_dir().map(|p| p.join(".config")), | ||
dirs::config_dir(), | ||
]; | ||
KlassyKat marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
get_path_from_candidates(config_dir_candidates) | ||
.map_err(|_| anyhow!("failed to find os config dir.")) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not really the "os config dir" we were looking for, right? |
||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use std::fs; | ||
|
||
use super::{app, get_path_from_candidates}; | ||
use tempfile::tempdir; | ||
|
||
#[test] | ||
fn verify_app() { | ||
app().debug_assert(); | ||
} | ||
.ok_or_else(|| anyhow!("failed to find os config dir."))?; | ||
|
||
path.push("gitui"); | ||
Ok(path) | ||
} | ||
#[test] | ||
fn test_config_dir_candidates_from_preexisting() { | ||
let temp_dummy_1 = tempdir().expect("should create temp dir"); | ||
let temp_dummy_2 = tempdir().expect("should create temp dir"); | ||
let temp_target = tempdir().expect("should create temp dir"); | ||
let temp_goal = temp_target.path().join("gitui"); | ||
|
||
fs::create_dir_all(&temp_goal) | ||
.expect("should create temp target directory"); | ||
|
||
let candidates = [ | ||
Some(temp_dummy_1.path().to_path_buf()), | ||
Some(temp_target.path().to_path_buf()), | ||
Some(temp_dummy_2.path().to_path_buf()), | ||
]; | ||
let result = get_path_from_candidates(candidates) | ||
.expect("should find the included target"); | ||
assert_eq!(result, temp_goal); | ||
} | ||
|
||
#[test] | ||
fn test_config_dir_candidates_no_preexisting() { | ||
let temp_dummy_1 = tempdir().expect("should create temp dir"); | ||
let temp_dummy_2 = tempdir().expect("should create temp dir"); | ||
|
||
#[test] | ||
fn verify_app() { | ||
app().debug_assert(); | ||
let candidates = [ | ||
Some(temp_dummy_1.path().to_path_buf()), | ||
Some(temp_dummy_2.path().to_path_buf()), | ||
]; | ||
|
||
let result = get_path_from_candidates(candidates) | ||
.expect("should return first candidate"); | ||
assert_eq!(result, temp_dummy_1.path().join("gitui")); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.