-
-
Notifications
You must be signed in to change notification settings - Fork 613
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 9 commits
beb71fb
8e307d3
fb935b7
819b4ca
cbc388d
299015a
9ea0d0d
cde05b1
e8440c2
e54cab7
4bb1279
bafd07c
759c26a
c2e58ee
a1adeb1
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, 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.
This only applies when attempting to write a file.
So believe this point is fairly moot when it comes to That being said it does have some consideration for I don't really imagine anyone wanting to do that, but it seems superior to have the option than not to. 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. Yes, for It worked before and does not work now. I'm sorry to say, but I don't think I'd want to approve this. If this were to fix a huge issue, okay, but this only addresses a (very) peculiar concern on windows. |
||
{ | ||
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.")) | ||
KlassyKat marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
#[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")); | ||
} | ||
} |
Uh oh!
There was an error while loading. Please reload this page.