Repository: flxzt/rnote (language: rust)

Issue:
Add confirmation prompt to deletion action
This is my WIP attempt to resolve #1136, but it isn't working at the moment. Clearly, I don't understand Rust and the whole borrowing logic enough for me to resolve this at the moment. I tried many different things but can't get the logic "Rusty" enough. At the moment, the issue is that the `&appwindow` reference in trash.rs doesn't live long enough. I tried creating a clone, among other things, but that didn't help either.

Other than that, I tried to copy the style of the other dialogues as much as possible to keep the whole architectural idea the same.

I'll keep working on this sometime, but in the meantime, if someone knows how to properly do this, feel free to point me in the right direction or open a PR.
prompt for confirmation when removing folders
**Describe the bug**  
I deleted the 'Documents' directory from the Default Workspace, corresponding to '~/Documents' linux directory.

The directory was permanently deleted, and not even a user dialog warning about this was issued.

The deleted directory wasn't moved to the Trash bin, it was permanently deleted.

I tried recovering it with the Linux tool 'testdisk' without success.

**To Reproduce**  
Steps to reproduce the behavior:
1. Go to the Workspace
2. Delete a directory

**Expected behavior**  

The deleted directory should have been moved to the Trash bin, or at least warn me that it was going to be permanently deleted.

**Console Output**  

I can't provide the output because I turned off the computer to try to recover the deleted data with 'testdisk' Linux utility. I'm afraid that turning it on again will overwrite the unallocated disk sectors that were originally occupied by the deleted data.

**Desktop**  
 - OS: Kubuntu 22.04
 - App Version: Rnote v0.10.2
 - Installation Source: Flatpak
 - Desktop Environment: KDE Plasma 5.24 
 - Display Server: Wayland
 - Input Source: Mouse and Keyboard, External Graphics Tablet




Discussion / hints:
I'd say to look at what's done here : 
https://github.com/flxzt/rnote/blob/42f16dadb3d370aad72926e1017e26124dc1ff6e/crates/rnote-ui/src/dialogs/mod.rs#L108

Wrap what's done inside the "trash" option inside of the `glib::spawn_future_local(clone! ...` part, to have weak references instead of strong ones (if you don't you have lifetime issues) 

> Do we actually need to prompt when trashing in all cases?
> 
> It's a bit redundant because `trash` should already be recoverable (in contrast to _deletion_).
> 
> Maybe it's best to only display the dialog for folders that are not empty to avoid mistakes like the one from the original issue?

I was thinking about this as well. However, the error in the issue was mainly the user deleting something they didn't want to delete and not knowing that it just gets moved to the trash. I could see the exact same issue being reported with someone accidentally deleting a really important file, so I don't think only adding it for folders is the user friendliest solution.

Edit: I was also thinking maybe the wording should be something along the lines of “Are you sure you want to move this file to the trash?” to make it clearer that it's not actually permanently deleted.
> I was thinking about this as well. However, the error in the issue was mainly the user deleting something they didn't want to delete and not knowing that it just gets moved to the trash. I could see the exact same issue being reported with someone accidentally deleting a really important file, so I don't think only adding it for folders is the user friendliest solution.

Alright, that makes sense. Let's keep it for all files and folder.

>Edit: I was also thinking maybe the wording should be something along the lines of “Are you sure you want to move this file to the trash?” to make it clearer that it's not actually permanently deleted.

Yes that reads better to me!

I just noticed a different issue with the dialog, it seems to be working only once for an individual file row. When it is cancelled and then tried to be opened a second time it just does not appear
> > Edit: I was also thinking maybe the wording should be something along the lines of “Are you sure you want to move this file to the trash?” to make it clearer that it's not actually permanently deleted.
> 
> Yes that reads better to me!

Alright, fixed it ^^

> I just noticed a different issue with the dialog, it seems to be working only once for an individual file row. When it is cancelled and then tried to be opened a second time it just does not appear

That's weird. You're right, I did some testing, but I have no idea what could be the cause. It should get correctly destroyed when it gets cancelled IMO.
So after using 'r-linux' data recovery tool, I could track the deleted files and find them in '~/.local/share/Trash/files/'. Just moving them to 'home/' solved the problem.

Anyway, I insist that RNote should have a pop-up dialog warning the user about this.
Yes, agreed. At least when trashing folders that are not empty. It's too easy to accidentally do that currently
I'll have a go at this if that's fine with everyone :) Can't promise a time frame though, I'll have to learn to deal with GTK first.

Fix patch:
diff --git a/AUTHORS b/AUTHORS
index afeb6efa87..b10e34db63 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -4,4 +4,5 @@ Seio Inoue
 Moritz Mechelk
 PhilProg
 Silvan Schmidt
-Doublonmousse
\ No newline at end of file
+Doublonmousse
+RayJW
diff --git a/crates/rnote-ui/data/ui/dialogs/dialogs.ui b/crates/rnote-ui/data/ui/dialogs/dialogs.ui
index 35345f7d86..a375d6e11e 100644
--- a/crates/rnote-ui/data/ui/dialogs/dialogs.ui
+++ b/crates/rnote-ui/data/ui/dialogs/dialogs.ui
@@ -203,4 +203,15 @@ Changes which are not saved will be permanently lost.</property>
       <object class="RnIconPicker" id="edit_selected_workspace_icon_picker"></object>
     </child>
   </object>
+
+  <object class="AdwAlertDialog" id="dialog_trash_file">
+    <property name="heading" translatable="yes">Trash File</property>
+    <property name="body" translatable="yes">Are you sure you want to move this file to the trash?</property>
+    <property name="default-response">cancel</property>
+    <property name="close-response">cancel</property>
+    <responses>
+      <response id="cancel" translatable="yes">Cancel</response>
+      <response id="trash" appearance="destructive" translatable="yes">Trash</response>
+    </responses>
+  </object>
 </interface>
diff --git a/crates/rnote-ui/src/dialogs/mod.rs b/crates/rnote-ui/src/dialogs/mod.rs
index 86bcd3f84d..013ddab299 100644
--- a/crates/rnote-ui/src/dialogs/mod.rs
+++ b/crates/rnote-ui/src/dialogs/mod.rs
@@ -586,6 +586,50 @@ pub(crate) async fn dialog_edit_selected_workspace(appwindow: &RnAppWindow) {
     dialog.present(appwindow.root().as_ref());
 }
 
+pub(crate) async fn dialog_trash_file(appwindow: &RnAppWindow, current_file: &gio::File) {
+    let builder = Builder::from_resource(
+        (String::from(config::APP_IDPATH) + "ui/dialogs/dialogs.ui").as_str(),
+    );
+    let dialog: adw::AlertDialog = builder.object("dialog_trash_file").unwrap();
+
+    match dialog.choose_future(appwindow).await.as_str() {
+        "trash" => {
+            glib::spawn_future_local(clone!(
+                #[weak]
+                appwindow,
+                #[strong]
+                current_file,
+                async move {
+                    current_file.trash_async(
+                        glib::source::Priority::DEFAULT,
+                        None::<&gio::Cancellable>,
+                        clone!(
+                            #[weak]
+                            appwindow,
+                            #[strong]
+                            current_file,
+                            move |res| {
+                                if let Err(e) = res {
+                                    appwindow
+                                        .overlays()
+                                        .dispatch_toast_error(&gettext("Trashing file failed"));
+                                    error!(
+                                        "Trash filerow file `{current_file:?}` failed , Err: {e:?}"
+                                    );
+                                    return;
+                                }
+                            }
+                        ),
+                    );
+                }
+            ));
+        }
+        _ => {
+            // Cancel
+        }
+    }
+}
+
 const WORKSPACELISTENTRY_ICONS_LIST: &[&str] = &[
     "workspacelistentryicon-bandaid-symbolic",
     "workspacelistentryicon-bank-symbolic",
diff --git a/crates/rnote-ui/src/workspacebrowser/filerow/actions/trash.rs b/crates/rnote-ui/src/workspacebrowser/filerow/actions/trash.rs
index aa8d9df682..c5b67b29be 100644
--- a/crates/rnote-ui/src/workspacebrowser/filerow/actions/trash.rs
+++ b/crates/rnote-ui/src/workspacebrowser/filerow/actions/trash.rs
@@ -1,9 +1,7 @@
 // Imports
-use crate::workspacebrowser::RnFileRow;
 use crate::RnAppWindow;
-use gettextrs::gettext;
-use gtk4::{gio, glib, glib::clone, prelude::FileExt};
-use tracing::debug;
+use crate::{dialogs, workspacebrowser::RnFileRow};
+use gtk4::{gio, glib, glib::clone};
 
 /// Create a new `trash` action.
 pub(crate) fn trash(filerow: &RnFileRow, appwindow: &RnAppWindow) -> gio::SimpleAction {
@@ -13,30 +11,19 @@ pub(crate) fn trash(filerow: &RnFileRow, appwindow: &RnAppWindow) -> gio::Simple
         filerow,
         #[weak]
         appwindow,
-        move |_action_trash_file, _| {
+        move |_, _| {
             let Some(current_file) = filerow.current_file() else {
                 return;
             };
-            current_file.trash_async(
-                glib::source::Priority::DEFAULT,
-                None::<&gio::Cancellable>,
-                clone!(
-                    #[weak]
-                    filerow,
-                    #[strong]
-                    current_file,
-                    move |res| {
-                        if let Err(e) = res {
-                            appwindow
-                                .overlays()
-                                .dispatch_toast_error(&gettext("Trashing file failed"));
-                            debug!("Trash filerow file `{current_file:?}` failed , Err: {e:?}");
-                            return;
-                        }
-                        filerow.set_current_file(None);
-                    }
-                ),
-            );
+            glib::spawn_future_local(clone!(
+                #[weak]
+                appwindow,
+                #[strong]
+                current_file,
+                async move {
+                    dialogs::dialog_trash_file(&appwindow, &current_file).await;
+                }
+            ));
         }
     ));
     action


Test patch: