GKaszewski/k-launcher
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(review): bugs, arch violations, design smells

Browse Source 
P1 bugs:
- unix_launcher: shell_split respects quoted args (was split_whitespace)
- plugin-host: 5s timeout on external plugin search
- ui: handle engine init panic, wire error state
- ui-egui: read window config instead of always using defaults
- plugin-url: use OpenPath action instead of SpawnProcess+xdg-open

Architecture:
- remove WindowConfig (mirror of WindowCfg); use WindowCfg directly
- remove on_select closure from SearchResult (domain leakage)
- remove LaunchAction::Custom; add Plugin::on_selected + SearchEngine::on_selected
- apps: record frecency via on_selected instead of embedded closure

Design smells:
- frecency: extract decay_factor helper, write outside mutex
- apps: remove cfg(test) cache_path hack; add new_for_test ctor
- apps: stable ResultId using name+exec to prevent collision
- files: stable ResultId using full path instead of index
- plugin-host: remove k-launcher-os-bridge dep (WindowConfig gone)
This commit is contained in:
Gabriel Kaszewski
2026-03-18 13:45:48 +01:00
parent 38860762c0
commit ff9b2b5712
18 changed files with 189 additions and 133 deletions
Download Patch File Download Diff File Expand all files Collapse all files

50
crates/plugins/plugin-apps/src/frecency.rs
View File

@@ -55,17 +55,17 @@ impl FrecencyStore {
.duration_since(UNIX_EPOCH)
.unwrap_or_default()
.as_secs();
let mut data = self.data.lock().unwrap();
let entry = data.entry(id.to_string()).or_insert(Entry {
count: 0,
last_used: 0,
});
entry.count += 1;
entry.last_used = now;
if let Some(parent) = self.path.parent() {
let _ = std::fs::create_dir_all(parent);
}
if let Ok(json) = serde_json::to_string(&*data) {
let json = {
let mut data = self.data.lock().unwrap();
let entry = data.entry(id.to_string()).or_insert(Entry { count: 0, last_used: 0 });
entry.count += 1;
entry.last_used = now;
serde_json::to_string(&*data).ok()
}; // lock released here
if let Some(json) = json {
if let Some(parent) = self.path.parent() {
let _ = std::fs::create_dir_all(parent);
}
let _ = std::fs::write(&self.path, json);
}
}
@@ -78,14 +78,7 @@ impl FrecencyStore {
.unwrap_or_default()
.as_secs();
let age_secs = now.saturating_sub(entry.last_used);
let decay = if age_secs < 3600 {
4
} else if age_secs < 86400 {
2
} else {
1
};
entry.count * decay
entry.count * decay_factor(age_secs)
}
pub fn top_ids(&self, n: usize) -> Vec<String> {
@@ -98,14 +91,7 @@ impl FrecencyStore {
.iter()
.map(|(id, entry)| {
let age_secs = now.saturating_sub(entry.last_used);
let decay = if age_secs < 3600 {
4
} else if age_secs < 86400 {
2
} else {
1
};
(id.clone(), entry.count * decay)
(id.clone(), entry.count * decay_factor(age_secs))
})
.collect();
scored.sort_by(|a, b| b.1.cmp(&a.1));
@@ -113,6 +99,16 @@ impl FrecencyStore {
}
}
fn decay_factor(age_secs: u64) -> u32 {
if age_secs < 3600 {
4
} else if age_secs < 86400 {
2
} else {
1
}
}
#[cfg(test)]
mod tests {
use super::*;

76
crates/plugins/plugin-apps/src/lib.rs
View File

@@ -76,7 +76,6 @@ struct CachedEntry {
category: Option<String>,
icon: Option<String>,
exec: String,
on_select: Arc<dyn Fn() + Send + Sync>,
}
// --- Serializable cache data (no closures) ---
@@ -92,24 +91,16 @@ struct CachedEntryData {
}
fn cache_path() -> Option<PathBuf> {
#[cfg(test)]
return None;
#[cfg(not(test))]
dirs::cache_dir().map(|d| d.join("k-launcher/apps.bin"))
}
fn load_from_path(path: &Path, frecency: &Arc<FrecencyStore>) -> Option<HashMap<String, CachedEntry>> {
fn load_from_path(path: &Path) -> Option<HashMap<String, CachedEntry>> {
let data = std::fs::read(path).ok()?;
let (entries_data, _): (Vec<CachedEntryData>, _) =
bincode::serde::decode_from_slice(&data, bincode::config::standard()).ok()?;
let map = entries_data
.into_iter()
.map(|e| {
let store = Arc::clone(frecency);
let record_id = e.id.clone();
let on_select: Arc<dyn Fn() + Send + Sync> = Arc::new(move || {
store.record(&record_id);
});
let cached = CachedEntry {
id: e.id.clone(),
name: AppName::new(e.name),
@@ -117,7 +108,6 @@ fn load_from_path(path: &Path, frecency: &Arc<FrecencyStore>) -> Option<HashMap<
category: e.category,
icon: e.icon,
exec: e.exec,
on_select,
};
(e.id, cached)
})
@@ -150,7 +140,7 @@ fn build_entries(source: &impl DesktopEntrySource, frecency: &Arc<FrecencyStore>
.entries()
.into_iter()
.map(|e| {
let id = format!("app-{}", e.name.as_str());
let id = format!("app-{}:{}", e.name.as_str(), e.exec.as_str());
let keywords_lc = e.keywords.iter().map(|k| k.to_lowercase()).collect();
#[cfg(target_os = "linux")]
let icon = e
@@ -160,18 +150,12 @@ fn build_entries(source: &impl DesktopEntrySource, frecency: &Arc<FrecencyStore>
#[cfg(not(target_os = "linux"))]
let icon: Option<String> = None;
let exec = e.exec.as_str().to_string();
let store = Arc::clone(frecency);
let record_id = id.clone();
let on_select: Arc<dyn Fn() + Send + Sync> = Arc::new(move || {
store.record(&record_id);
});
let cached = CachedEntry {
id: id.clone(),
keywords_lc,
category: e.category,
icon,
exec,
on_select,
name: e.name,
};
(id, cached)
@@ -188,16 +172,25 @@ pub struct AppsPlugin {
impl AppsPlugin {
pub fn new(source: impl DesktopEntrySource + 'static, frecency: Arc<FrecencyStore>) -> Self {
let cached = cache_path().and_then(|p| load_from_path(&p, &frecency));
Self::new_impl(source, frecency, cache_path())
}
fn new_impl(
source: impl DesktopEntrySource + 'static,
frecency: Arc<FrecencyStore>,
cp: Option<PathBuf>,
) -> Self {
let cached = cp.as_deref().and_then(load_from_path);
let entries = if let Some(from_cache) = cached {
// Serve cache immediately; refresh in background.
let map = Arc::new(RwLock::new(from_cache));
let entries_bg = Arc::clone(&map);
let frecency_bg = Arc::clone(&frecency);
let cp_bg = cp.clone();
std::thread::spawn(move || {
let fresh = build_entries(&source, &frecency_bg);
if let Some(path) = cache_path() {
if let Some(path) = cp_bg {
save_to_path(&path, &fresh);
}
*entries_bg.write().unwrap() = fresh;
@@ -206,14 +199,19 @@ impl AppsPlugin {
} else {
// No cache: build synchronously, then persist.
let initial = build_entries(&source, &frecency);
if let Some(path) = cache_path() {
save_to_path(&path, &initial);
if let Some(path) = &cp {
save_to_path(path, &initial);
}
Arc::new(RwLock::new(initial))
};
Self { entries, frecency }
}
#[cfg(test)]
fn new_for_test(source: impl DesktopEntrySource + 'static, frecency: Arc<FrecencyStore>) -> Self {
Self::new_impl(source, frecency, None)
}
}
fn initials(name_lc: &str) -> String {
@@ -267,6 +265,10 @@ impl Plugin for AppsPlugin {
"apps"
}
fn on_selected(&self, id: &ResultId) {
self.frecency.record(id.as_str());
}
async fn search(&self, query: &str) -> Vec<SearchResult> {
let entries = self.entries.read().unwrap();
if query.is_empty() {
@@ -284,7 +286,6 @@ impl Plugin for AppsPlugin {
icon: e.icon.clone(),
score: Score::new(score),
action: LaunchAction::SpawnProcess(e.exec.clone()),
on_select: Some(Arc::clone(&e.on_select)),
})
})
.collect();
@@ -307,7 +308,6 @@ impl Plugin for AppsPlugin {
icon: e.icon.clone(),
score: Score::new(score),
action: LaunchAction::SpawnProcess(e.exec.clone()),
on_select: Some(Arc::clone(&e.on_select)),
})
})
.collect()
@@ -381,7 +381,7 @@ mod tests {
#[tokio::test]
async fn apps_prefix_match() {
let p = AppsPlugin::new(
let p = AppsPlugin::new_for_test(
MockSource::with(vec![("Firefox", "firefox")]),
ephemeral_frecency(),
);
@@ -391,7 +391,7 @@ mod tests {
#[tokio::test]
async fn apps_no_match_returns_empty() {
let p = AppsPlugin::new(
let p = AppsPlugin::new_for_test(
MockSource::with(vec![("Firefox", "firefox")]),
ephemeral_frecency(),
);
@@ -400,7 +400,7 @@ mod tests {
#[tokio::test]
async fn apps_empty_query_no_frecency_returns_empty() {
let p = AppsPlugin::new(
let p = AppsPlugin::new_for_test(
MockSource::with(vec![("Firefox", "firefox")]),
ephemeral_frecency(),
);
@@ -414,7 +414,7 @@ mod tests {
}
#[test]
fn score_match_exact_beats_prefix_beats_abbrev_beats_substr() {
fn score_match_exact_beats_prefix() {
let exact = score_match("firefox", "firefox");
let prefix = score_match("firefox", "fire");
let abbrev = score_match("gnu firefox", "gf");
@@ -428,7 +428,7 @@ mod tests {
#[tokio::test]
async fn apps_abbreviation_match() {
let p = AppsPlugin::new(
let p = AppsPlugin::new_for_test(
MockSource::with(vec![("Visual Studio Code", "code")]),
ephemeral_frecency(),
);
@@ -440,7 +440,7 @@ mod tests {
#[tokio::test]
async fn apps_keyword_match() {
let p = AppsPlugin::new(
let p = AppsPlugin::new_for_test(
MockSource::with_keywords(vec![("Code", "code", vec!["editor", "ide"])]),
ephemeral_frecency(),
);
@@ -451,7 +451,7 @@ mod tests {
#[tokio::test]
async fn apps_fuzzy_typo_match() {
let p = AppsPlugin::new(
let p = AppsPlugin::new_for_test(
MockSource::with(vec![("Firefox", "firefox")]),
ephemeral_frecency(),
);
@@ -472,7 +472,7 @@ mod tests {
#[tokio::test]
async fn apps_category_appears_in_description() {
let p = AppsPlugin::new(
let p = AppsPlugin::new_for_test(
MockSource::with_categories(vec![("Code", "code", "Text Editor")]),
ephemeral_frecency(),
);
@@ -483,10 +483,10 @@ mod tests {
#[tokio::test]
async fn apps_empty_query_returns_top_frecent() {
let frecency = ephemeral_frecency();
frecency.record("app-Code");
frecency.record("app-Code");
frecency.record("app-Firefox");
let p = AppsPlugin::new(
frecency.record("app-Code:code");
frecency.record("app-Code:code");
frecency.record("app-Firefox:firefox");
let p = AppsPlugin::new_for_test(
MockSource::with(vec![("Firefox", "firefox"), ("Code", "code")]),
frecency,
);
@@ -507,8 +507,8 @@ mod tests {
save_to_path(&cache_file, &entries);
// Load from temp path — should contain Firefox
let loaded = load_from_path(&cache_file, &frecency).unwrap();
assert!(loaded.contains_key("app-Firefox"));
let loaded = load_from_path(&cache_file).unwrap();
assert!(loaded.contains_key("app-Firefox:firefox"));
std::fs::remove_file(&cache_file).ok();
}

1
crates/plugins/plugin-calc/src/lib.rs
View File

@@ -90,7 +90,6 @@ impl Plugin for CalcPlugin {
icon: None,
score: Score::new(90),
action: LaunchAction::CopyToClipboard(value_str),
on_select: None,
}]
}
_ => vec![],

1
crates/plugins/plugin-cmd/src/lib.rs
View File

@@ -36,7 +36,6 @@ impl Plugin for CmdPlugin {
icon: None,
score: Score::new(95),
action: LaunchAction::SpawnInTerminal(cmd.to_string()),
on_select: None,
}]
}
}

6
crates/plugins/plugin-files/src/lib.rs
View File

@@ -71,21 +71,19 @@ impl Plugin for FilesPlugin {
.unwrap_or(false)
})
.take(20)
.enumerate()
.map(|(i, entry)| {
.map(|entry| {
let full_path = entry.path();
let name = entry.file_name().to_string_lossy().to_string();
let is_dir = full_path.is_dir();
let title = if is_dir { format!("{name}/") } else { name };
let path_str = full_path.to_string_lossy().to_string();
SearchResult {
id: ResultId::new(format!("file-{i}")),
id: ResultId::new(&path_str),
title: ResultTitle::new(title),
description: Some(path_str.clone()),
icon: None,
score: Score::new(50),
action: LaunchAction::OpenPath(path_str),
on_select: None,
}
})
.collect()

12
crates/plugins/plugin-url/src/main.rs
View File

@@ -10,7 +10,7 @@ struct Query {
#[derive(Serialize)]
struct Action {
r#type: &'static str,
cmd: String,
path: String,
}
#[derive(Serialize)]
@@ -45,8 +45,8 @@ fn search(query: &str) -> Vec<Result> {
description: url.clone(),
score: 95,
action: Action {
r#type: "SpawnProcess",
cmd: format!("xdg-open {url}"),
r#type: "OpenPath",
path: url.clone(),
},
}]
}
@@ -112,7 +112,7 @@ mod tests {
fn search_returns_result() {
let results = search("https://example.com");
assert_eq!(results.len(), 1);
assert_eq!(results[0].action.cmd, "xdg-open https://example.com");
assert_eq!(results[0].action.path, "https://example.com");
}
#[test]
@@ -124,7 +124,7 @@ mod tests {
fn result_serializes() {
let results = search("https://example.com");
let json = serde_json::to_string(&results).unwrap();
assert!(json.contains("SpawnProcess"));
assert!(json.contains("xdg-open"));
assert!(json.contains("OpenPath"));
assert!(json.contains("https://example.com"));
}
}