fix: smaller fixes from the project review

- settings: one invalid value in ui.json no longer resets all settings
  (including the storage location), only that value
- sync: a worker whose pass outlives a stop/start ends afterwards
- updater: only install packages whose signed file name carries a newer
  version than the running one
- canvas: grow the stroke vertex buffer at most up to the device limit

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
This commit is contained in:
haraldandClaude Fable 5.2 committed 2026-11-14 15:49:11 +01:00
1 parent a22f081115
commit e95b03e316
5 files changed
+160 -11

No files matched your search

+29 -2
View File
@@ -1021,8 +1021,13 @@ impl CanvasState {
} }
if page.vertex_cache.len() > self.committed_vertex_capacity { if page.vertex_cache.len() > self.committed_vertex_capacity {
self.committed_vertex_capacity = let vertex_size = std::mem::size_of::<InkVertex>() as u64;
(page.vertex_cache.len() * 2).max(Self::INITIAL_COMMITTED_CAPACITY); let max_vertices = (self.device.limits().max_buffer_size / vertex_size) as usize;
self.committed_vertex_capacity = grown_vertex_capacity(
page.vertex_cache.len(),
Self::INITIAL_COMMITTED_CAPACITY,
max_vertices,
);
self.committed_vertex_buffer = self.device.create_buffer(&wgpu::BufferDescriptor { self.committed_vertex_buffer = self.device.create_buffer(&wgpu::BufferDescriptor {
label: Some("Committed Strokes Vertex Buffer (Reallocated)"), label: Some("Committed Strokes Vertex Buffer (Reallocated)"),
size: (self.committed_vertex_capacity * std::mem::size_of::<InkVertex>()) as u64, size: (self.committed_vertex_capacity * std::mem::size_of::<InkVertex>()) as u64,
@@ -1735,10 +1740,32 @@ fn text_resolution(screen_px_per_unit: f32) -> f32 {
2.0_f32.powf(steps / 4.0).clamp(0.125, 8.0) 2.0_f32.powf(steps / 4.0).clamp(0.125, 8.0)
} }
/// Capacity for a vertex buffer that has to hold `needed` vertices: twice as much, so that
/// it is not reallocated with every stroke, but no more than the device allows. Doubling
/// alone asked for a buffer beyond the limit as soon as a very full page used half of it,
/// which ends in a validation panic.
fn grown_vertex_capacity(needed: usize, initial: usize, max_vertices: usize) -> usize {
needed
.saturating_mul(2)
.max(initial)
.min(max_vertices)
.max(needed)
}
#[cfg(test)] #[cfg(test)]
mod tests { mod tests {
use super::*; use super::*;
#[test]
fn test_vertex_capacity_doubles_up_to_the_device_limit() {
assert_eq!(grown_vertex_capacity(10, 1000, 1_000_000), 1000);
assert_eq!(grown_vertex_capacity(5000, 1000, 1_000_000), 10_000);
// More than half the limit: the limit, not twice the need
assert_eq!(grown_vertex_capacity(600_000, 1000, 1_000_000), 1_000_000);
// Never less than what is needed
assert_eq!(grown_vertex_capacity(2_000_000, 1000, 1_000_000), 2_000_000);
}
#[test] #[test]
fn test_text_resolution_is_quantized_and_never_below_screen() { fn test_text_resolution_is_quantized_and_never_below_screen() {
for px in [0.2_f32, 0.27, 0.5, 1.3, 3.9] { for px in [0.2_f32, 0.27, 0.5, 1.3, 3.9] {
+42 -6
View File
@@ -953,6 +953,16 @@ struct Shared {
/// Finished sign-in, to be stored by the app /// Finished sign-in, to be stored by the app
signed_in: Option<Account>, signed_in: Option<Account>,
stop: bool, stop: bool,
/// Counts the started workers. A worker whose pass was still running when the next one
/// was started must end afterwards, although `stop` is `false` again by then.
generation: u64,
}
impl Shared {
/// Is the worker of `generation` to end?
fn retired(&self, generation: u64) -> bool {
self.stop || self.generation != generation
}
} }
type SharedState = Arc<(Mutex<Shared>, Condvar)>; type SharedState = Arc<(Mutex<Shared>, Condvar)>;
@@ -986,6 +996,7 @@ impl CloudSync {
changed: false, changed: false,
signed_in: None, signed_in: None,
stop: false, stop: false,
generation: 0,
}), }),
Condvar::new(), Condvar::new(),
)), )),
@@ -1009,15 +1020,19 @@ impl CloudSync {
fn start(&mut self, account: Account) { fn start(&mut self, account: Account) {
self.stop_worker(); self.stop_worker();
{ let generation = {
let mut shared = lock(&self.shared); let mut shared = lock(&self.shared);
shared.stop = false; shared.stop = false;
shared.generation += 1;
shared.status = Status::Idle { last: None }; shared.status = Status::Idle { last: None };
shared.next_pass = Instant::now(); shared.next_pass = Instant::now();
} shared.generation
};
let shared = self.shared.clone(); let shared = self.shared.clone();
let worker_account = account.clone(); let worker_account = account.clone();
self.worker = Some(std::thread::spawn(move || worker(shared, worker_account))); self.worker = Some(std::thread::spawn(move || {
worker(shared, worker_account, generation)
}));
self.account = Some(account); self.account = Some(account);
} }
@@ -1211,7 +1226,7 @@ fn sign_in(
} }
} }
fn worker(shared: SharedState, account: Account) { fn worker(shared: SharedState, account: Account, generation: u64) {
let Some(state_path) = SyncState::file_path() else { let Some(state_path) = SyncState::file_path() else {
set_status(&shared, Status::Failed(t!("Kein Datenordner").to_string())); set_status(&shared, Status::Failed(t!("Kein Datenordner").to_string()));
return; return;
@@ -1228,7 +1243,7 @@ fn worker(shared: SharedState, account: Account) {
let (dir, ctx) = { let (dir, ctx) = {
let mut guard = lock(&shared); let mut guard = lock(&shared);
loop { loop {
if guard.stop { if guard.retired(generation) {
return; return;
} }
let now = Instant::now(); let now = Instant::now();
@@ -1252,7 +1267,7 @@ fn worker(shared: SharedState, account: Account) {
eprintln!("[Nextcloud] Sync-Stand nicht gespeichert: {err}"); eprintln!("[Nextcloud] Sync-Stand nicht gespeichert: {err}");
} }
let mut guard = lock(&shared); let mut guard = lock(&shared);
if guard.stop { if guard.retired(generation) {
return; return;
} }
guard.status = match result { guard.status = match result {
@@ -1282,6 +1297,27 @@ mod tests {
use super::*; use super::*;
use std::collections::HashMap; use std::collections::HashMap;
#[test]
fn worker_of_an_earlier_start_is_retired() {
let mut shared = Shared {
status: Status::Off,
local_dir: PathBuf::new(),
ctx: Context::default(),
next_pass: Instant::now(),
changed: false,
signed_in: None,
stop: false,
generation: 1,
};
assert!(!shared.retired(1));
// Stopped and started again while the pass of worker 1 was still running
shared.generation = 2;
assert!(shared.retired(1));
assert!(!shared.retired(2));
shared.stop = true;
assert!(shared.retired(2));
}
/// Server folder in memory; folder ETags change with anything inside, like Nextcloud's. /// Server folder in memory; folder ETags change with anything inside, like Nextcloud's.
#[derive(Default)] #[derive(Default)]
struct FakeRemote { struct FakeRemote {
+4
View File
@@ -179,6 +179,10 @@ pub const EN: &[(&str, &str)] = &[
("Kariert", "Grid"), ("Kariert", "Grid"),
("KAROS, LINIEN UND PUNKTE", "GRID, LINES AND DOTS"), ("KAROS, LINIEN UND PUNKTE", "GRID, LINES AND DOTS"),
("kein Asset-Ordner", "no asset folder"), ("kein Asset-Ordner", "no asset folder"),
(
"'{asset_name}' ist keine neuere Version und wird nicht installiert.",
"'{asset_name}' is not a newer version and will not be installed.",
),
("Kein Eintrag für '{asset_name}' in SHA256SUMS.txt gefunden. Heruntergeladene Datei wurde gelöscht.", "No entry for '{asset_name}' found in SHA256SUMS.txt. The downloaded file was deleted."), ("Kein Eintrag für '{asset_name}' in SHA256SUMS.txt gefunden. Heruntergeladene Datei wurde gelöscht.", "No entry for '{asset_name}' found in SHA256SUMS.txt. The downloaded file was deleted."),
("Kein Release ausgewählt. Download abgelehnt.", "No release selected. Download refused."), ("Kein Release ausgewählt. Download abgelehnt.", "No release selected. Download refused."),
("Keine manifest.json vorhanden", "No manifest.json present"), ("Keine manifest.json vorhanden", "No manifest.json present"),
+44 -3
View File
@@ -302,15 +302,35 @@ impl UiSettings {
crate::paths::config_dir().map(|d| d.join("ui.json")) crate::paths::config_dir().map(|d| d.join("ui.json"))
} }
/// Lädt die Einstellungen; bei fehlender oder beschädigter Datei gelten Standardwerte. /// Keeps every setting of a partly invalid file that can still be read; only the
/// unreadable ones get their default. One bad value must not reset everything, least of
/// all the storage location.
fn salvage(content: &str) -> Self {
let Ok(serde_json::Value::Object(file)) = serde_json::from_str(content) else {
return Self::default();
};
let mut good = serde_json::Map::new();
for (key, value) in file {
good.insert(key.clone(), value);
let candidate = serde_json::Value::Object(good.clone());
if serde_json::from_value::<Self>(candidate).is_err() {
eprintln!("[Einstellungen] '{key}' ist ungültig, verwende Standardwert.");
good.remove(&key);
}
}
serde_json::from_value(serde_json::Value::Object(good)).unwrap_or_default()
}
/// Lädt die Einstellungen; bei fehlender Datei gelten Standardwerte, bei einer teilweise
/// ungültigen Datei nur für die unlesbaren Werte.
pub fn load_from(path: &Path) -> Self { pub fn load_from(path: &Path) -> Self {
let mut settings: Self = match std::fs::read_to_string(path) { let mut settings: Self = match std::fs::read_to_string(path) {
Ok(content) => serde_json::from_str(&content).unwrap_or_else(|e| { Ok(content) => serde_json::from_str(&content).unwrap_or_else(|e| {
eprintln!( eprintln!(
"[Einstellungen] '{}' ist ungültig ({e}), verwende Standardwerte.", "[Einstellungen] '{}' ist ungültig ({e}), unlesbare Werte werden ersetzt.",
path.display() path.display()
); );
Self::default() Self::salvage(&content)
}), }),
Err(_) => Self::default(), Err(_) => Self::default(),
}; };
@@ -437,6 +457,27 @@ mod tests {
assert_eq!(UiSettings::load_from(&path), UiSettings::default()); assert_eq!(UiSettings::load_from(&path), UiSettings::default());
} }
#[test]
fn test_one_invalid_value_does_not_reset_the_other_settings() {
let dir = tempfile::tempdir().unwrap();
let path = dir.path().join("ui.json");
std::fs::write(
&path,
br#"{"storage_dir": "/home/pia/Nextcloud/ink-notes", "eraser_radius_mm": "big", "math_assist": true}"#,
)
.unwrap();
let loaded = UiSettings::load_from(&path);
assert_eq!(
loaded.storage_dir,
Some(PathBuf::from("/home/pia/Nextcloud/ink-notes"))
);
assert!(loaded.math_assist);
assert_eq!(
loaded.eraser_radius_mm,
UiSettings::default().eraser_radius_mm
);
}
#[test] #[test]
fn test_seven_pens_stay_readable_for_older_versions() { fn test_seven_pens_stay_readable_for_older_versions() {
// Versions before 0.44 read `pen_presets` as exactly four pens (Issue #56) // Versions before 0.44 read `pen_presets` as exactly four pens (Issue #56)
+41
View File
@@ -272,6 +272,22 @@ pub fn is_newer_version(remote_tag: &str, current_tag: &str, include_prereleases
} }
} }
/// Version in a release file name (`ink-notes_<version>_<arch>.<ext>`).
pub fn asset_version(asset_name: &str) -> Option<&str> {
let rest = asset_name.strip_prefix("ink-notes_")?;
let (version, _) = rest.split_once('_')?;
parse_semver(version).map(|_| version)
}
/// Is the package named `asset_name` newer than the running version `current`?
///
/// The signature only covers file names and hashes, not the release they are offered in. A
/// server could therefore present an old, correctly signed package under a new version
/// number. The version in the signed file name closes that gap.
pub fn asset_is_upgrade(asset_name: &str, current: &str) -> bool {
asset_version(asset_name).is_some_and(|version| is_newer_version(version, current, true))
}
/// Berechnet den hex-kodierten SHA-256 Hash einer Datei. /// Berechnet den hex-kodierten SHA-256 Hash einer Datei.
pub fn compute_sha256(path: &Path) -> Result<String, String> { pub fn compute_sha256(path: &Path) -> Result<String, String> {
let mut file = std::fs::File::open(path).map_err(|e| { let mut file = std::fs::File::open(path).map_err(|e| {
@@ -712,6 +728,14 @@ impl UpdaterController {
} }
}; };
if !asset_is_upgrade(&safe_asset_name, env!("CARGO_PKG_VERSION")) {
self.status = UpdaterStatus::Error(t!(
"'{asset_name}' ist keine neuere Version und wird nicht installiert.",
asset_name = safe_asset_name
));
return;
}
self.status = UpdaterStatus::Downloading { self.status = UpdaterStatus::Downloading {
asset_name: safe_asset_name.clone(), asset_name: safe_asset_name.clone(),
percent: 0.0, percent: 0.0,
@@ -1036,6 +1060,23 @@ mod tests {
assert!(is_newer_version("1.0.0-rc1", "0.9.0", true)); assert!(is_newer_version("1.0.0-rc1", "0.9.0", true));
} }
#[test]
fn test_only_packages_named_as_newer_are_upgrades() {
assert_eq!(asset_version("ink-notes_0.46.0_amd64.deb"), Some("0.46.0"));
assert_eq!(
asset_version("ink-notes_0.46.0_x86_64.AppImage"),
Some("0.46.0")
);
assert_eq!(asset_version("SHA256SUMS.txt"), None);
assert_eq!(asset_version("ink-notes_latest_amd64.deb"), None);
assert!(asset_is_upgrade("ink-notes_0.46.0_arm64.apk", "0.45.0"));
// An old release offered again under a new tag
assert!(!asset_is_upgrade("ink-notes_0.30.0_amd64.deb", "0.45.0"));
assert!(!asset_is_upgrade("ink-notes_0.45.0_x86_64.exe", "0.45.0"));
assert!(!asset_is_upgrade("something_else.deb", "0.45.0"));
}
#[test] #[test]
fn test_sanitize_asset_name() { fn test_sanitize_asset_name() {
assert_eq!( assert_eq!(