Hello,

thanks, that a good idea. It seems that only Ext4 allows it during mount and all others not.

Created attachment 356681 [details] [review]
Add unmount option to edit filesystem dialog

I kept it simple and just added an option to the checkbox to unmount the filesystem before changing the label. Would any other features be necessary/desired for this? (E.g. Remounting the filesystem, hiding the option for ext4, etc.)

I tested every case except where unmounting fails, but that is handled. Please let me know what you think.

Review of attachment 356681 [details] [review]:

Nice to do something about that!
If we check the filesystem type for Ext2/3/4 then we can just ignore the mount state and continue. For all others we could call the ensure_unused function before changing the label. That would eliminate the need of an additional UI element which still lets the operation fail if not selected correctly. Unmounting something automatically to perform an action would not be unusual in GNOME Disks. Would you like to try this?

Best regards,
Kai

::: src/disks/gdufilesystemdialog.c
@@ +153,3 @@
+                                                g_variant_new ("a{sv}", NULL), /* options */
+                                                NULL, /* cancellable */
+                                                &error))

Maybe it's better to use the gdu_window_ensure_unused function instead of unmounting only this filesystem interface because for btrfs there can be more than one mountpoint. Anyway gdu_window_ensure_unused is also not aware of the other btrfs mountpoints yet so the result of them is currently the same ;)

Sure, I can use that instead. I would be in favor of automatically trying to unmount, but should we provide the user with a notice that this might happen or does it happen enough in other parts of the application that this should be expected?

I have no preference on this, the write benchmark will also do it without notice, but yes, you could keep the mount test and the test that this is not an ExtY filesystem in order to show a sentence that it will be unmounted.

Created attachment 357133 [details] [review]
Same as previous with review comments incorporated

Here is the updated patch. It will now umount automatically for mounted, non-ext filesystems and display a message to the user.

There are two areas for which I have questions/would like feedback:

Is there an existing function or cleaner way of determining if a filesystem is of a certain type? This was the best I could come up with after looking through gduutils and the udisks documentation, but I'd prefer not to have to use hardcoded values like I did.

Please look closely at the memory de-allocation. I don't think I missed anything but I'd like a second set of eyes. Also, is the existing ChangeFilesystemLabelData struct de-allocated properly? To me it looks wrong but that could be a gap in my understanding.

Thanks.

Review of attachment 357133 [details] [review]:

Thanks, well done! Just the marked code style changes would be nice.

And one more thing: This would introduce usage of colored icons - for now only the black/white versions are used inside of entries. So if opened there is one icon on the right and one on the left in color… Maybe it's better to position the text at the top of the dialog and add some space around it but no icon? Other opinions?

::: src/disks/gdufilesystemdialog.c
@@ +96,3 @@
+ensure_unused_cb (UDisksObject *object,
+                  GAsyncResult *res,
+                  gpointer user_data)

Maybe "user_data" could be aligned with the "res" as in the rest of the file.

@@ +122,3 @@
+  g_object_unref (data->filesystem);
+  g_free (data->new_label);
+  g_free (data);

Instead of doing it piece for piece here there could be a edit_filesystem_free () function which does it. To implement a _free function for each structure is what's commonly done in the project.

@@ +143,3 @@
   gchar *fstype;
+  const gchar *const *mount_points;
+  gboolean needsUnmount;

Can you please rename it to needs_unmount to distinguish it from camel case for types?

Created attachment 357316 [details] [review]
Adjust existing memory management for dialog

Alright fixed up those issues. I agree it does look strange with the two opposing icons so I did as you suggested and moved the message up top and removed the extra icon.

I also added an extra commit to update the memory management for the existing struct in the file to use a cleanup function.

Created attachment 357317 [details] [review]
Add unmount function to edit filesystem dialog

Created attachment 357318 [details]
Dialog with unmount warning displayed

Here's a screenshot for easy reference.

Review of attachment 357316 [details] [review]:

Hello,
thanks for doing this in addition.

::: src/disks/gdufilesystemdialog.c
@@ +32,3 @@
+{
+  if (data->dialog != NULL)
+    g_object_unref (data->dialog);

Short note for next time: An additional linebreak is common to visually separate the if-block from the rest.

@@ +92,3 @@
       g_error_free (error);
     }
+  g_object_unref (filesystem);

This unref here is wrong because the data is not transfered for a callback call. I will just remove it while committing ;)

Apologies I see why the filesystem unref is confusing here. In the other commit though I increased the ref count of filesystem before passing it to the set_label function as that is an asynchronous call. Unless doing so was a mistake it would need this additional unref at the end of the callback.

Review of attachment 357317 [details] [review]:

Thanks, overall it's good and will be included directly as bug fix.

::: src/disks/gdufilesystemdialog.c
@@ +50,3 @@
+    g_object_unref (data->window);
+  if (data->filesystem != NULL)
+    g_object_unref (data->filesystem);

For next time: Instead of != NULL and unref it can be done in a single line with g_clear_object (&var) which also sets the pointer to NULL.

@@ +133,3 @@
+  else
+    {
+      udisks_filesystem_call_set_label (g_object_ref (data->filesystem),

The ref is not done for issueing a D-Bus call, I'll remove it while committing.

@@ +223,3 @@
+  else
+    {
+      udisks_filesystem_call_set_label (g_object_ref (filesystem),

Same here, ref is not needed.

::: src/disks/ui/edit-filesystem-dialog.ui
@@ +1,2 @@
 <?xml version="1.0" encoding="UTF-8"?>
+<!-- Generated with glade 3.20.0 -->

I'll filter the many changes due to using a newer version to save the file. After the 3.26 release we can reopen all files and save them to prevent this in the future.