From 3f28bc0f778b31d5c958f128cfba802039c330ae Mon Sep 17 00:00:00 2001
From: anesthetice <118751106+anesthetice@users.noreply.github.com>
Date: Thu, 18 Jul 2024 21:38:37 +0200
Subject: [PATCH 1/7] I mean it works? Further testing and benchmarking
 required

---
 Cargo.lock                             |  1 +
 Cargo.toml                             |  1 +
 crates/rnote-ui/Cargo.toml             |  1 +
 crates/rnote-ui/src/canvas/imexport.rs | 73 ++++++++++++++++++++------
 4 files changed, 59 insertions(+), 17 deletions(-)

diff --git a/Cargo.lock b/Cargo.lock
index c6e82457b2..e8f7033777 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -3252,6 +3252,7 @@ dependencies = [
  "async-fs",
  "base64",
  "cairo-rs",
+ "crc32fast",
  "fs_extra",
  "futures",
  "gettext-rs",
diff --git a/Cargo.toml b/Cargo.toml
index 11a562edcc..a2fad2eb89 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -29,6 +29,7 @@ base64 = "0.22.1"
 cairo-rs = { version = "0.19.4", features = ["v1_18", "png", "svg", "pdf"] }
 chrono = "0.4.38"
 clap = { version = "4.5", features = ["derive"] }
+crc32fast = "1.4"
 dialoguer = "0.11.0"
 flate2 = "1.0"
 fs_extra = "1.3"
diff --git a/crates/rnote-ui/Cargo.toml b/crates/rnote-ui/Cargo.toml
index 04d3e8e8ff..2f4a84789c 100644
--- a/crates/rnote-ui/Cargo.toml
+++ b/crates/rnote-ui/Cargo.toml
@@ -19,6 +19,7 @@ anyhow = { workspace = true }
 async-fs = { workspace = true }
 base64 = { workspace = true }
 cairo-rs = { workspace = true }
+crc32fast = { workspace = true }
 fs_extra = { workspace = true }
 futures = { workspace = true }
 gettext-rs = { workspace = true }
diff --git a/crates/rnote-ui/src/canvas/imexport.rs b/crates/rnote-ui/src/canvas/imexport.rs
index aa26ad0a8e..35ad942d50 100644
--- a/crates/rnote-ui/src/canvas/imexport.rs
+++ b/crates/rnote-ui/src/canvas/imexport.rs
@@ -2,6 +2,7 @@
 use super::RnCanvas;
 use anyhow::Context;
 use futures::channel::oneshot;
+use futures::AsyncReadExt;
 use futures::AsyncWriteExt;
 use gtk4::{gio, prelude::*};
 use rnote_compose::ext::Vector2Ext;
@@ -214,53 +215,91 @@ impl RnCanvas {
         let basename = file
             .basename()
             .ok_or_else(|| anyhow::anyhow!("Could not retrieve basename for file: `{file:?}`."))?;
+        let mut tmp_file_path = file_path.clone();
+        tmp_file_path.set_extension("tmp");
+
         let rnote_bytes_receiver = self
             .engine_ref()
             .save_as_rnote_bytes(basename.to_string_lossy().to_string());
-        let mut skip_set_output_file = false;
-        if let Some(current_file_path) = self.output_file().and_then(|f| f.path()) {
-            if crate::utils::paths_abs_eq(current_file_path, &file_path).unwrap_or(false) {
-                skip_set_output_file = true;
-            }
-        }
 
         self.dismiss_output_file_modified_toast();
+        tracing::info!("two-step save method called");
 
-        let file_write_operation = async move {
+        let file_write_operation = async {
             let bytes = rnote_bytes_receiver.await??;
+            tracing::info!("bytes received");
             self.set_output_file_expect_write(true);
             let mut write_file = async_fs::OpenOptions::new()
                 .create(true)
                 .truncate(true)
                 .write(true)
-                .open(&file_path)
+                .open(&tmp_file_path)
                 .await
                 .context(format!(
                     "Failed to create/open/truncate file for path '{}'",
                     file_path.display()
                 ))?;
-            if !skip_set_output_file {
-                // this installs the file watcher.
-                self.set_output_file(Some(file.to_owned()));
-            }
             write_file.write_all(&bytes).await.context(format!(
                 "Failed to write bytes to file with path '{}'",
                 file_path.display()
             ))?;
             write_file.sync_all().await.context(format!(
                 "Failed to sync file after writing with path '{}'",
-                file_path.display()
+                &tmp_file_path.display()
             ))?;
-            Ok(())
+
+            Ok::<(usize, u32), anyhow::Error>((bytes.len(), crc32fast::hash(&bytes)))
         };
 
-        if let Err(e) = file_write_operation.await {
+        let (size, checksum) = file_write_operation.await.map_err(|e| {
             self.set_save_in_progress(false);
             // If the file operations failed in any way, we make sure to clear the expect_write flag
             // because we can't know for sure if the output-file watcher will be able to.
             self.set_output_file_expect_write(false);
-            return Err(e);
-        }
+            e
+        })?;
+
+        tracing::info!(
+            "finished writing to file, checksum calculated as : {}",
+            checksum
+        );
+
+        let file_check_operation = async {
+            let mut read_file = async_fs::OpenOptions::new()
+                .read(true)
+                .open(&tmp_file_path)
+                .await
+                .context(format!(
+                    "Failed to open/read file for path '{}'",
+                    &tmp_file_path.display()
+                ))?;
+            let mut data: Vec<u8> = Vec::with_capacity(size);
+            read_file.read_to_end(&mut data).await?;
+
+            tracing::info!("finished reading temporary file");
+
+            let new_checksum = crc32fast::hash(&data);
+            tracing::info!("new checksum calculated as : {}", new_checksum);
+            if checksum != new_checksum {
+                return Err(anyhow::anyhow!(
+                    "Checksum of temporary file does not match internal"
+                ));
+            }
+
+            Ok::<(), anyhow::Error>(())
+        };
+        file_check_operation.await?;
+
+        let file_swap_operation = async {
+            if file_path.exists() {
+                async_fs::remove_file(&file_path).await?;
+            }
+            async_fs::rename(&tmp_file_path, &file_path).await?;
+            Ok::<(), anyhow::Error>(())
+        };
+        file_swap_operation.await?;
+
+        self.set_output_file(Some(gio::File::for_path(&file_path)));
 
         self.set_unsaved_changes(false);
         self.set_save_in_progress(false);

From 65a814dbeb96d696807ffc0129a133a2171116c1 Mon Sep 17 00:00:00 2001
From: anesthetice <118751106+anesthetice@users.noreply.github.com>
Date: Thu, 18 Jul 2024 23:11:38 +0200
Subject: [PATCH 2/7] benchmark code

---
 crates/rnote-ui/src/canvas/imexport.rs | 46 +++++++++++++++-----------
 1 file changed, 27 insertions(+), 19 deletions(-)

diff --git a/crates/rnote-ui/src/canvas/imexport.rs b/crates/rnote-ui/src/canvas/imexport.rs
index 35ad942d50..2e2ed0926f 100644
--- a/crates/rnote-ui/src/canvas/imexport.rs
+++ b/crates/rnote-ui/src/canvas/imexport.rs
@@ -212,22 +212,22 @@ impl RnCanvas {
         let file_path = file
             .path()
             .ok_or_else(|| anyhow::anyhow!("Could not get a path for file: `{file:?}`."))?;
-        let basename = file
-            .basename()
-            .ok_or_else(|| anyhow::anyhow!("Could not retrieve basename for file: `{file:?}`."))?;
         let mut tmp_file_path = file_path.clone();
         tmp_file_path.set_extension("tmp");
 
+        let basename = file
+            .basename()
+            .ok_or_else(|| anyhow::anyhow!("Could not retrieve basename for file: `{file:?}`."))?;
         let rnote_bytes_receiver = self
             .engine_ref()
             .save_as_rnote_bytes(basename.to_string_lossy().to_string());
 
         self.dismiss_output_file_modified_toast();
-        tracing::info!("two-step save method called");
 
+        use std::time::Instant;
+        let _global_start = Instant::now();
         let file_write_operation = async {
             let bytes = rnote_bytes_receiver.await??;
-            tracing::info!("bytes received");
             self.set_output_file_expect_write(true);
             let mut write_file = async_fs::OpenOptions::new()
                 .create(true)
@@ -248,10 +248,14 @@ impl RnCanvas {
                 &tmp_file_path.display()
             ))?;
 
-            Ok::<(usize, u32), anyhow::Error>((bytes.len(), crc32fast::hash(&bytes)))
+            let _start = Instant::now();
+            let hash = crc32fast::hash(&bytes);
+            tracing::info!("internal checksum : {:.5}", _start.elapsed().as_secs_f64());
+
+            Ok::<(usize, u32), anyhow::Error>((bytes.len(), hash))
         };
 
-        let (size, checksum) = file_write_operation.await.map_err(|e| {
+        let (size, internal_checksum) = file_write_operation.await.map_err(|e| {
             self.set_save_in_progress(false);
             // If the file operations failed in any way, we make sure to clear the expect_write flag
             // because we can't know for sure if the output-file watcher will be able to.
@@ -259,37 +263,35 @@ impl RnCanvas {
             e
         })?;
 
-        tracing::info!(
-            "finished writing to file, checksum calculated as : {}",
-            checksum
-        );
-
+        let _start = Instant::now();
         let file_check_operation = async {
             let mut read_file = async_fs::OpenOptions::new()
                 .read(true)
                 .open(&tmp_file_path)
                 .await
                 .context(format!(
-                    "Failed to open/read file for path '{}'",
+                    "Failed to open/read temporary file for path '{}'",
                     &tmp_file_path.display()
                 ))?;
             let mut data: Vec<u8> = Vec::with_capacity(size);
             read_file.read_to_end(&mut data).await?;
 
-            tracing::info!("finished reading temporary file");
-
-            let new_checksum = crc32fast::hash(&data);
-            tracing::info!("new checksum calculated as : {}", new_checksum);
-            if checksum != new_checksum {
+            let external_checksum = crc32fast::hash(&data);
+            if internal_checksum != external_checksum {
                 return Err(anyhow::anyhow!(
-                    "Checksum of temporary file does not match internal"
+                    "The checksum of the temporary file does not match the internal checksum"
                 ));
             }
 
             Ok::<(), anyhow::Error>(())
         };
         file_check_operation.await?;
+        tracing::info!(
+            "file_check_operation : {:.5}",
+            _start.elapsed().as_secs_f64()
+        );
 
+        let _start = Instant::now();
         let file_swap_operation = async {
             if file_path.exists() {
                 async_fs::remove_file(&file_path).await?;
@@ -298,12 +300,18 @@ impl RnCanvas {
             Ok::<(), anyhow::Error>(())
         };
         file_swap_operation.await?;
+        tracing::info!(
+            "file_swap_operation : {:.5}",
+            _start.elapsed().as_secs_f64()
+        );
 
         self.set_output_file(Some(gio::File::for_path(&file_path)));
 
         self.set_unsaved_changes(false);
         self.set_save_in_progress(false);
 
+        tracing::info!("total : {:.5}", _global_start.elapsed().as_secs_f64());
+
         Ok(true)
     }
 

From 36a4018c20ad5e7af1af89130879fa46e46459a7 Mon Sep 17 00:00:00 2001
From: anesthetice <118751106+anesthetice@users.noreply.github.com>
Date: Fri, 19 Jul 2024 09:43:52 +0200
Subject: [PATCH 3/7] code cleanup, added context to potential errors

---
 crates/rnote-ui/src/canvas/imexport.rs | 33 ++++++++------------------
 1 file changed, 10 insertions(+), 23 deletions(-)

diff --git a/crates/rnote-ui/src/canvas/imexport.rs b/crates/rnote-ui/src/canvas/imexport.rs
index 2e2ed0926f..4dc068c158 100644
--- a/crates/rnote-ui/src/canvas/imexport.rs
+++ b/crates/rnote-ui/src/canvas/imexport.rs
@@ -224,8 +224,6 @@ impl RnCanvas {
 
         self.dismiss_output_file_modified_toast();
 
-        use std::time::Instant;
-        let _global_start = Instant::now();
         let file_write_operation = async {
             let bytes = rnote_bytes_receiver.await??;
             self.set_output_file_expect_write(true);
@@ -248,11 +246,7 @@ impl RnCanvas {
                 &tmp_file_path.display()
             ))?;
 
-            let _start = Instant::now();
-            let hash = crc32fast::hash(&bytes);
-            tracing::info!("internal checksum : {:.5}", _start.elapsed().as_secs_f64());
-
-            Ok::<(usize, u32), anyhow::Error>((bytes.len(), hash))
+            Ok::<(usize, u32), anyhow::Error>((bytes.len(), crc32fast::hash(&bytes)))
         };
 
         let (size, internal_checksum) = file_write_operation.await.map_err(|e| {
@@ -263,7 +257,6 @@ impl RnCanvas {
             e
         })?;
 
-        let _start = Instant::now();
         let file_check_operation = async {
             let mut read_file = async_fs::OpenOptions::new()
                 .read(true)
@@ -279,39 +272,33 @@ impl RnCanvas {
             let external_checksum = crc32fast::hash(&data);
             if internal_checksum != external_checksum {
                 return Err(anyhow::anyhow!(
-                    "The checksum of the temporary file does not match the internal checksum"
+                    "Mismatch between the internal and external checksum, temporary file most likely corrupted"
                 ));
             }
 
             Ok::<(), anyhow::Error>(())
         };
         file_check_operation.await?;
-        tracing::info!(
-            "file_check_operation : {:.5}",
-            _start.elapsed().as_secs_f64()
-        );
 
-        let _start = Instant::now();
         let file_swap_operation = async {
             if file_path.exists() {
-                async_fs::remove_file(&file_path).await?;
+                async_fs::remove_file(&file_path).await.context(format!(
+                    "Failed to remove old save file with path '{}'",
+                    &file_path.display()
+                ))?;
             }
-            async_fs::rename(&tmp_file_path, &file_path).await?;
+            async_fs::rename(&tmp_file_path, &file_path)
+                .await
+                .context("Failed to rename the temporary save file into the main one")?;
+
             Ok::<(), anyhow::Error>(())
         };
         file_swap_operation.await?;
-        tracing::info!(
-            "file_swap_operation : {:.5}",
-            _start.elapsed().as_secs_f64()
-        );
 
         self.set_output_file(Some(gio::File::for_path(&file_path)));
-
         self.set_unsaved_changes(false);
         self.set_save_in_progress(false);
 
-        tracing::info!("total : {:.5}", _global_start.elapsed().as_secs_f64());
-
         Ok(true)
     }
 

From 652743cb4418d5499d099da60f19b55022d55db9 Mon Sep 17 00:00:00 2001
From: anesthetice <118751106+anesthetice@users.noreply.github.com>
Date: Fri, 19 Jul 2024 10:31:45 +0200
Subject: [PATCH 4/7] zeroize impl + benchmark, minor incorrect 'context' fixes

---
 crates/rnote-ui/src/canvas/imexport.rs | 32 ++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/crates/rnote-ui/src/canvas/imexport.rs b/crates/rnote-ui/src/canvas/imexport.rs
index 4dc068c158..c3fce78858 100644
--- a/crates/rnote-ui/src/canvas/imexport.rs
+++ b/crates/rnote-ui/src/canvas/imexport.rs
@@ -235,11 +235,11 @@ impl RnCanvas {
                 .await
                 .context(format!(
                     "Failed to create/open/truncate file for path '{}'",
-                    file_path.display()
+                    tmp_file_path.display()
                 ))?;
             write_file.write_all(&bytes).await.context(format!(
                 "Failed to write bytes to file with path '{}'",
-                file_path.display()
+                tmp_file_path.display()
             ))?;
             write_file.sync_all().await.context(format!(
                 "Failed to sync file after writing with path '{}'",
@@ -280,8 +280,35 @@ impl RnCanvas {
         };
         file_check_operation.await?;
 
+        use std::time::Instant;
+        let _start = Instant::now();
         let file_swap_operation = async {
             if file_path.exists() {
+                // zeroize the previous version of the save file
+                let mut zeroize_file = async_fs::OpenOptions::new()
+                    .write(true)
+                    .truncate(false) // no real need to truncate as we will manually overwrite everything
+                    .create(false)
+                    .open(&file_path)
+                    .await
+                    .context(format!(
+                        "Failed to open file for path '{}'",
+                        &file_path.display()
+                    ))?;
+                let file_size = zeroize_file.metadata().await?.len() as usize;
+                zeroize_file
+                    .write_all(&vec![0; file_size])
+                    .await
+                    .context(format!(
+                        "Failed to write bytes to file with path '{}'",
+                        file_path.display()
+                    ))?;
+                zeroize_file.sync_all().await.context(format!(
+                    "Failed to sync file after writing with path '{}'",
+                    &tmp_file_path.display()
+                ))?;
+
+                // finally remove the previous save file after it was zeroized
                 async_fs::remove_file(&file_path).await.context(format!(
                     "Failed to remove old save file with path '{}'",
                     &file_path.display()
@@ -294,6 +321,7 @@ impl RnCanvas {
             Ok::<(), anyhow::Error>(())
         };
         file_swap_operation.await?;
+        tracing::info!("file swap op : {:.7}", _start.elapsed().as_secs_f64());
 
         self.set_output_file(Some(gio::File::for_path(&file_path)));
         self.set_unsaved_changes(false);

From b4b5cfb8d30a774c6bb11c3d07c363e09c0c3717 Mon Sep 17 00:00:00 2001
From: anesthetice <118751106+anesthetice@users.noreply.github.com>
Date: Fri, 19 Jul 2024 10:57:39 +0200
Subject: [PATCH 5/7] removed benchmarking code

---
 crates/rnote-ui/src/canvas/imexport.rs | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/crates/rnote-ui/src/canvas/imexport.rs b/crates/rnote-ui/src/canvas/imexport.rs
index c3fce78858..203e8911e1 100644
--- a/crates/rnote-ui/src/canvas/imexport.rs
+++ b/crates/rnote-ui/src/canvas/imexport.rs
@@ -280,8 +280,6 @@ impl RnCanvas {
         };
         file_check_operation.await?;
 
-        use std::time::Instant;
-        let _start = Instant::now();
         let file_swap_operation = async {
             if file_path.exists() {
                 // zeroize the previous version of the save file
@@ -310,7 +308,7 @@ impl RnCanvas {
 
                 // finally remove the previous save file after it was zeroized
                 async_fs::remove_file(&file_path).await.context(format!(
-                    "Failed to remove old save file with path '{}'",
+                    "Failed to remove previous save file with path '{}'",
                     &file_path.display()
                 ))?;
             }
@@ -321,7 +319,6 @@ impl RnCanvas {
             Ok::<(), anyhow::Error>(())
         };
         file_swap_operation.await?;
-        tracing::info!("file swap op : {:.7}", _start.elapsed().as_secs_f64());
 
         self.set_output_file(Some(gio::File::for_path(&file_path)));
         self.set_unsaved_changes(false);

From f174144df68deecbf3cf5491329a0dcc10969b4b Mon Sep 17 00:00:00 2001
From: anesthetice <118751106+anesthetice@users.noreply.github.com>
Date: Sat, 27 Jul 2024 13:49:29 +0200
Subject: [PATCH 6/7] removed zeroize code

---
 crates/rnote-ui/src/canvas/imexport.rs | 25 -------------------------
 1 file changed, 25 deletions(-)

diff --git a/crates/rnote-ui/src/canvas/imexport.rs b/crates/rnote-ui/src/canvas/imexport.rs
index 203e8911e1..c40001cdb6 100644
--- a/crates/rnote-ui/src/canvas/imexport.rs
+++ b/crates/rnote-ui/src/canvas/imexport.rs
@@ -282,31 +282,6 @@ impl RnCanvas {
 
         let file_swap_operation = async {
             if file_path.exists() {
-                // zeroize the previous version of the save file
-                let mut zeroize_file = async_fs::OpenOptions::new()
-                    .write(true)
-                    .truncate(false) // no real need to truncate as we will manually overwrite everything
-                    .create(false)
-                    .open(&file_path)
-                    .await
-                    .context(format!(
-                        "Failed to open file for path '{}'",
-                        &file_path.display()
-                    ))?;
-                let file_size = zeroize_file.metadata().await?.len() as usize;
-                zeroize_file
-                    .write_all(&vec![0; file_size])
-                    .await
-                    .context(format!(
-                        "Failed to write bytes to file with path '{}'",
-                        file_path.display()
-                    ))?;
-                zeroize_file.sync_all().await.context(format!(
-                    "Failed to sync file after writing with path '{}'",
-                    &tmp_file_path.display()
-                ))?;
-
-                // finally remove the previous save file after it was zeroized
                 async_fs::remove_file(&file_path).await.context(format!(
                     "Failed to remove previous save file with path '{}'",
                     &file_path.display()

From ef0519a7f503b3d6dc1ffb626b16670d1ff98ea1 Mon Sep 17 00:00:00 2001
From: anesthetice <118751106+anesthetice@users.noreply.github.com>
Date: Fri, 9 Aug 2024 00:04:42 +0200
Subject: [PATCH 7/7] newline failed fmt check

---
 crates/rnote-ui/src/canvas/imexport.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crates/rnote-ui/src/canvas/imexport.rs b/crates/rnote-ui/src/canvas/imexport.rs
index ffa97d656f..79aee4c638 100644
--- a/crates/rnote-ui/src/canvas/imexport.rs
+++ b/crates/rnote-ui/src/canvas/imexport.rs
@@ -300,7 +300,7 @@ impl RnCanvas {
 
         self.set_output_file(Some(gio::File::for_path(&file_path)));
         debug!("Saving file has finished successfully");
-      
+
         self.set_unsaved_changes(false);
         self.set_save_in_progress(false);