Address review comments

Byron · Jan 9, 2024 · 63639c3 · 63639c3
1 parent fd89ea5
commit 63639c3
Show file tree

Hide file tree

Showing 3 changed files with 48 additions and 42 deletions.
diff --git a/src/freedesktop.rs b/src/freedesktop.rs
@@ -17,7 +17,7 @@ use std::{
 
 use log::{debug, warn};
 
-use crate::{Error, TrashContext, TrashItem, TrashItemMetadata};
+use crate::{Error, TrashContext, TrashItem, TrashItemMetadata, TrashItemSize};
 
 type FsError = (PathBuf, std::io::Error);
 
@@ -223,15 +223,15 @@ pub fn metadata(item: &TrashItem) -> Result<TrashItemMetadata, Error> {
     // which is the filename in the `id` field.
     let info_file = &item.id;
 
-    // A bunch of unwraps here. This is fine because if any of these fail that means
-    // that either there's a bug in this code or the target system didn't follow
-    // the specification.
     let file = restorable_file_in_trash_from_info_file(info_file);
     assert!(virtually_exists(&file).map_err(|e| fs_error(&file, e))?);
     let metadata = fs::symlink_metadata(&file).map_err(|e| fs_error(&file, e))?;
     let is_dir = metadata.is_dir();
-    let size =
-        if is_dir { fs::read_dir(&file).map_err(|e| fs_error(&file, e))?.count() as u64 } else { metadata.len() };
+    let size = if is_dir {
+        TrashItemSize::Entries(fs::read_dir(&file).map_err(|e| fs_error(&file, e))?.count())
+    } else {
+        TrashItemSize::Bytes(metadata.len())
+    };
     Ok(TrashItemMetadata { is_dir, size })
 }
 
@@ -259,7 +259,6 @@ where
         // that either there's a bug in this code or the target system didn't follow
         // the specification.
         let file = restorable_file_in_trash_from_info_file(info_file);
-        assert!(virtually_exists(&file).map_err(|e| fs_error(&file, e))?);
         if file.is_dir() {
             std::fs::remove_dir_all(&file).map_err(|e| fs_error(&file, e))?;
         // TODO Update directory size cache if there's one.
@@ -816,8 +815,8 @@ fn get_mount_points() -> Result<Vec<MountPoint>, Error> {
     target_os = "netbsd"
 )))]
 fn get_mount_points() -> Result<Vec<MountPoint>, Error> {
-    // On platforms that don't have support yet, simply return no mount points
-    Ok(Vec::new())
+    // On platforms that don't have support yet, return an error
+    Err(Error::Unknown { description: "Mount points cannot be determined on this operating system".into() })
 }
 
 #[cfg(test)]

diff --git a/src/lib.rs b/src/lib.rs
@@ -304,13 +304,22 @@ impl Hash for TrashItem {
     }
 }
 
+/// Size of a [`TrashItem`] in bytes or entries
+#[derive(Debug, Clone)]
+pub enum TrashItemSize {
+    /// Number of bytes in a file
+    Bytes(u64),
+    /// Number of entries in a directory, non-recursive
+    Entries(usize),
+}
+
 /// Metadata about a [`TrashItem`]
 #[derive(Debug, Clone)]
 pub struct TrashItemMetadata {
     /// True if the [`TrashItem`] is a directory, false if it is a file
     pub is_dir: bool,
-    /// Number of entries for directories, number of bytes for files
-    pub size: u64,
+    /// The size of the item, as a [`TrashItemSize`] enum
+    pub size: TrashItemSize,
 }
 
 #[cfg(any(

diff --git a/src/windows.rs b/src/windows.rs
@@ -1,4 +1,4 @@
-use crate::{Error, TrashContext, TrashItem, TrashItemMetadata};
+use crate::{Error, TrashContext, TrashItem, TrashItemMetadata, TrashItemSize};
 use std::{
     borrow::Borrow,
     ffi::{c_void, OsStr, OsString},
@@ -129,39 +129,37 @@ pub fn list() -> Result<Vec<TrashItem>, Error> {
 
 pub fn metadata(item: &TrashItem) -> Result<TrashItemMetadata, Error> {
     ensure_com_initialized();
-    unsafe {
-        let id_as_wide: Vec<u16> = item.id.encode_wide().chain(std::iter::once(0)).collect();
-        let parsing_name = PCWSTR(id_as_wide.as_ptr());
-        let item: IShellItem = SHCreateItemFromParsingName(parsing_name, None)?;
-        let is_dir = item.GetAttributes(SFGAO_FOLDER)? == SFGAO_FOLDER;
-        let size = if is_dir {
-            let pesi: IEnumShellItems = item.BindToHandler(None, &BHID_EnumItems)?;
-            let mut size = 0;
-            loop {
-                let mut fetched_count: u32 = 0;
-                let mut arr = [None];
-                pesi.Next(&mut arr, Some(&mut fetched_count as *mut u32))?;
-
-                if fetched_count == 0 {
-                    break;
-                }
+    let id_as_wide: Vec<u16> = item.id.encode_wide().chain(std::iter::once(0)).collect();
+    let parsing_name = PCWSTR(id_as_wide.as_ptr());
+    let item: IShellItem = unsafe { SHCreateItemFromParsingName(parsing_name, None)? };
+    let is_dir = unsafe { item.GetAttributes(SFGAO_FOLDER)? } == SFGAO_FOLDER;
+    let size = if is_dir {
+        let pesi: IEnumShellItems = unsafe { item.BindToHandler(None, &BHID_EnumItems)? };
+        let mut size = 0;
+        loop {
+            let mut fetched_count: u32 = 0;
+            let mut arr = [None];
+            unsafe { pesi.Next(&mut arr, Some(&mut fetched_count as *mut u32))? };
+
+            if fetched_count == 0 {
+                break;
+            }
 
-                match &arr[0] {
-                    Some(_item) => {
-                        size += 1;
-                    }
-                    None => {
-                        break;
-                    }
+            match &arr[0] {
+                Some(_item) => {
+                    size += 1;
+                }
+                None => {
+                    break;
                 }
             }
-            size
-        } else {
-            let item2: IShellItem2 = item.cast()?;
-            item2.GetUInt64(&PKEY_Size)?
-        };
-        Ok(TrashItemMetadata { is_dir, size })
-    }
+        }
+        TrashItemSize::Entries(size)
+    } else {
+        let item2: IShellItem2 = item.cast()?;
+        TrashItemSize::Bytes(unsafe { item2.GetUInt64(&PKEY_Size)? })
+    };
+    Ok(TrashItemMetadata { is_dir, size })
 }
 
 pub fn purge_all<I>(items: I) -> Result<(), Error>