Created attachment 130909 [details] [review]
0001-Nautilus-extension-and-simple-GUI-formatter-tool.patch

Patch against git head, apply on top of the gnome-settings-daemon patch (for simplicity, clashes in configure.ac).

Things to consider:
 - the "standalone" nautilus-gdu mode is called 'gnome-disk-utility-format' -- maybe rename to something more clear?
 - my old TODO list is included in the nautilus directory, for tracking purposes; might be removed once I fix remaining issues and file bugs against g-d-u
 - we don't have icons yet, Mike Langlie is working on them

Possible binary name: gnome-disk-formatter, matches well with the 'Disk Formatter' menuitem.

+  if (! g_option_context_parse (context, &argc, &argv, &error)) {
+    g_printerr ("Could not parse arguments: %s\n", error->message);
+    g_error_free (error);
+    return 1;
+  }

[...]

+  /*  Initialize gtk  */
+  gtk_init (&argc, &argv);

This is not really the correct way to handle commandline parsing in gtk apps. 
You should either use gtk_get_option_group() and add the GTK+ options to your own context, or use gtk_init_with_args() to inject your own entries in the gtk argument parsing.

Created attachment 130981 [details] [review]
0001-Nautilus-extension-and-simple-GUI-formatter-tool.patch

(In reply to comment #2)
> Possible binary name: gnome-disk-formatter, matches well with the 'Disk
> Formatter' menuitem.

That sounds better, I was thinking about `gnome-disk-format` but used your suggestion at the end


(In reply to comment #3)
> This is not really the correct way to handle commandline parsing in gtk apps. 
> You should either use gtk_get_option_group() and add the GTK+ options to your
> own context, or use gtk_init_with_args() to inject your own entries in the gtk
> argument parsing.

Changed to gtk_init_with_args().

+  g_set_prgname ("gnome-disk-utility-format");

Should update this to match the new binary name.

Also, there is a bunch of g_print() calls that should probably go away.

But I think this mostly looks good now, and we should get it committed and worry about details later. David ?

(In reply to comment #5)
> +  g_set_prgname ("gnome-disk-utility-format");
> 
> Should update this to match the new binary name.
Oops, forgot to change it.

> Also, there is a bunch of g_print() calls that should probably go away.
Most of the messages are g_debug() prints already but Nautilus somewhat eats them so I left few g_print() until I find out what's wrong.

I think it may be worthwhile to take a few steps back and try to figure what we want the user experience to be like.

There are two use cases here

 1. User wants to format an existing partition / whole-disk device
    (the latter is for floppies)

 2. User has an uninitialized disk

For 1. the Nautilus extension should provide a "Format..." option in the context menu. For 2. we want to make our notification daemon notify the user and say something like "You have inserted a disk that appears to be uninitialized" and then launch the format program.

So in this bug we should focus on

 - Providing the Nautilus extension for 1.
 - Providing the format program used in both 1. and 2.

First of all, the Nautilus extension should be very minimal, it should basically just provide a "Format..." menu item for GVolume objects... and then spawn the format program. The format program should be a separate program installed into $libexecdir. Said program should not have any desktop file or anything.

Initially let's not worry about use case 2. So basically, all the format program needs to take is a device file and just call FilesystemCreate() on DeviceKit-disks and then report progress/errors as appropriate. Also needs to do something special if the device is unlocked/mounted/busy.

It looks like the code in the patch does most of it but I somewhat disagree with the user interface / user experience. I think something like this is much nicer for a simple tool like this

 +-----------------------------------------------------------------+
 |                    Format Volume "Stuff"                     [x]|
 +-----------------------------------------------------------------+
 |  <big><b>Format volume "Stuff"?</big></b>                       |
 |                                                                 |
 |  You are about to format the volume "Stuff" (dev/sda1). All     |
 |  existing data will be deleted.                                 |
 |                                                                 |
 |     Type: [Compatible with all computers (FAT)|V]               |
 |     Name: [davidz's data 20090223               ]               |
 |                                                                 |
 |                                                                 |
 | [Open Disk Utility]                           [Cancel] [Format] |
 +-----------------------------------------------------------------+

Notes:

 - The name entry is prefilled with "<username>'s data <date>"

 - The type combo box has the following members
   - "Compatible with all systems (FAT)" -> FAT
   - "Compatible with Linux computers (ext3)" -> ext3
   - "Encrypted, compatible with Linux computers (LUKS)" -> luks + ext3

 - We always pass the take_ownership_* options if applicable
   -  cf. http://hal.freedesktop.org/docs/DeviceKit-disks/Device.html#Device.FilesystemCreate

 - The tooltip for "Open Disk Utility" reads "Format volume using
   the Palimpsest Disk Utility" and if clicked the format dialog
   is dismissed and Palimpsest is started with an option to go to
   the device (need bug 575948 fixed)

 - Focus is initially at the [Cancel] button

When the user clicks format we automatically unmount the filesystem / lock the device if needed. Use gdu_util_dialog_show_filesystem_busy() if necessary. Don't ask for erase options. So ideally clicking "Format" will bring the user to the progress screen

 +-----------------------------------------------------------------+
 |                       Format Volume "Stuff"                  [x]|
 +-----------------------------------------------------------------+
 |                                                                 |
 |                                                                 |
 |                                                                 |
 |                                                                 |
 |   Formatting volume: [+++++++                  ]                |
 |                                                                 |
 |                                                                 |
 |                                                                 |
 |                                                                 |
 |                                                                 |
 +-----------------------------------------------------------------+

When formatting succeeds without error, then no need to show a confirmation dialog. Maybe we want the Nautilus extension to actually mount the volume / open a window once it is done.

Anyway, I think a simple UI like this is much better. Thoughts?

One point I was trying to make that might be worth stressing: the format tool should not have to care about partition tables or optical discs or anything weird like that. It only needs to care about calling FilesystemCreate() on the passed device file.

At least it won't have to care about partitioning until we attempt to solve use case 2.

This should also make the code a lot simpler.

I agree that the simplified UI you describe for the "Format a partition" use case is what the nautilus extension should be moving towards. I proposed some of the same things in bug 575851 and bug 575852. But I think we should develop those improvements in git, not in bugzilla.

Thanks David, I must admit your suggestions looks consistent to me with the rest of the Gnome desktop. As you said, most of the functionality is there and changing UI should be relatively simple.

>  2. User has an uninitialized disk
> For 2. we want to make our notification daemon notify the user
> and say something like "You have inserted a disk that appears to be
> uninitialized" and then launch the format program.
Good idea, however I would not strip format functionality of uninitialized disks from Nautilus, it should not harm.

> First of all, the Nautilus extension should be very minimal, it should
> basically just provide a "Format..." menu item for GVolume objects... and then
> spawn the format program. The format program should be a separate program
> installed into $libexecdir. 
> Maybe we want the Nautilus extension to actually mount the volume /
> open a window once it is done.
If we make the Nautilus extension spawn a format program and then want to automatically mount newly created filesystem, it would be better to have all the code in the extension.

At the moment, the extension and the standalone mode share most of the code, the difference is only the way the dialog is opened. But this is more a question of having the same code in two binaries.


The standalone mode was originally intended for debugging and as a bonus... Having the formatter tool in the same menu beside Text editor is questionable, so I guess we really don't need a desktop file.

Anyway, if you don't have major objections, I think it would be better to commit now and slowly transform this thing in git.

(In reply to comment #10)
> Thanks David, I must admit your suggestions looks consistent to me with the
> rest of the Gnome desktop. As you said, most of the functionality is there and
> changing UI should be relatively simple.

Cool.

> >  2. User has an uninitialized disk
> > For 2. we want to make our notification daemon notify the user
> > and say something like "You have inserted a disk that appears to be
> > uninitialized" and then launch the format program.
> Good idea, however I would not strip format functionality of uninitialized
> disks from Nautilus, it should not harm.

I don't think we want to show uninitialized disks in the file manager at all; if we do it's probably a bug. Also defining what "uninitialized disk/media/partition" *really* means is somewhat open to interpretation. It can either be 

 - A disk/media with no partition table and no known file system signature on
   the disk; or

 - A disk/media with a known partition table but no known file system signature
   on any of the partitions; or

 - possibly other oddball cases

We need to be careful when making this call. We also need to be careful in not throwing away information when presenting this to the user. Which implies having a notification icon or modal dialog with this information show up.

So I think we want to punt on the "uninitialized disk/media" use case for the time being; e.g. let's focus on making the "Format..." use case work.

> > First of all, the Nautilus extension should be very minimal, it should
> > basically just provide a "Format..." menu item for GVolume objects... and then
> > spawn the format program. The format program should be a separate program
> > installed into $libexecdir. 
> > Maybe we want the Nautilus extension to actually mount the volume /
> > open a window once it is done.
> If we make the Nautilus extension spawn a format program and then want to
> automatically mount newly created filesystem, it would be better to have all
> the code in the extension.

Not really. It's much cleaner to have the code in a standalone binary for reasons of robustness and ease of debugging. The Nautilus extension, as the first cut, really just should be something like

 volume = nautilus_file.get_volume();
 device_file = g_volume_get_identifier (volume, UNIX_DEVICE_FILE);
 if (device_file != NULL)
   {
     device = gdu_pool_get_by_device_file (pool, device_file);
     if (device != NULL) 
       {
         /* show "Format..." item */
       }
   }

Later on we can add other stuff to the Nautilus extension, e.g. reworking the "Basic" property page of the properties window and so forth.

> At the moment, the extension and the standalone mode share most of the code,
> the difference is only the way the dialog is opened. But this is more a
> question of having the same code in two binaries.
> 
> The standalone mode was originally intended for debugging and as a bonus...
> Having the formatter tool in the same menu beside Text editor is questionable,
> so I guess we really don't need a desktop file.

Just because the code is in a separate program/process does not imply it needs a menu item / desktop file.

> Anyway, if you don't have major objections, I think it would be better to
> commit now and slowly transform this thing in git.

I do; I'd like to do this in Bugzilla before committing it to mainline.

    David

Also, the Nautilus extension and the format tool should probably live in separate sub-directories; suggest

 src/gdu-nautilus-extension
 src/gdu-format-tool

> I do; I'd like to do this in Bugzilla before committing it to mainline.

Well, this is git, so presumably Tomas can just do the necessary changes in his local repo.

Created attachment 133426 [details] [review]
0001-Nautilus-extension-and-simple-GUI-formatter-tool.patch

Reworked patch, UI design adapted to the suggestions outlined earlier. Code has been split, making the format tool and nautilus extension independent. The nautilus extension has been made optional, g-d-u will compile without nautilus libs.

There are some new icons included in the patch (thanks to Mike Langlie!), used by the nautilus menu extension and possibly useful for bug 580126. I also kept support for uninitialized devices, for the bug 580126 purposes.

Patches in bug 573826 are still needed for proper nautilus extension functionality.

From my perspective, the only major thing missing is to complete support for LUKS volumes:
 - when the format tool is spawned with a LUKS device argument, find a parent, so that user can destroy the LUKS and format partition as normal ext3.
 - unmount checks (and the busy dialog) should be done for the enclosed /dev/dm-* device
This can be implemented later, it's not a blocker in current state.

FYI, I'm keeping my work in separate git repo: http://fedorapeople.org/gitweb?p=tbzatek/public_git/gnome-disk-utility.git;a=shortlog;h=refs/heads/nautilus-gdu

Oh yeah, and the code needs to be passed through indent before merging.

Great, looking much better now.

Cosmetic things I noticed on the first try:

- The window gets mapped much larger, and then shrinks to the right size. 

- The 'small print' ("You are about to format...") gets broken suboptimally. This is a GTK+ issue...

- It would be great if we could be intelligent and not show "All existing data will be deleted" if the medium is blank (like a blank cd, in my case). Likewise the scary confirmation dialog is not necessary for blank media.

- The dialog looks a bit wierd while the confirmation and password dialogs are up, because the content is already replaced by the naked progress bar. Maybe wait with doing that until the operation actually starts ?

- I think  the "Open Disk Utility" button would be better if there was a hint somewhere in the dialog that the disk utility may offer functionality that this dialog is missing. Not sure how to formulate that, and where to put it...


All these things can certainly be handled after merging the code.

Looking a bit into the patch:

+#ifdef HAVE_CONFIG_H
+ #include <config.h> /* for GETTEXT_PACKAGE */
+#endif

Not sure about gvfs coding style, but I personally hate HAVE_CONFIG_H and always unconditionally include <config.h> first.


//  g_strfreev (media_compat);   /* so, is this const then?  */

...C++ comments, too


witch (error->code) {
+  case GDU_ERROR_FAILED:
+          error_msg = _("The operation failed.");
+          break;

[ ... ]

+  default:
+          error_msg = _("Unknown error");
+          break;
+  }
Why do you need to manually map the code to an error message ?
Isn't error->message just that ? 


      if (strcasecmp (part_type, data->recommended_part_type) != 0)

Seems odd to compare types with strcasecmp - is there any uncertainty about the correct case, here ?

+char *
+g_icon_get_string (GIcon *icon)

Invading the glib namespace like this is on your own risk...:-)

This looks a lot better; here are the high-level comments

 1. I don't think gdu-format-tool should be in the business of
    modifying partition data such as the type. Stuff like this
    doesn't really matter (except for booting etc.), if people want
    to change it they can use Palimpsest. It only adds time to the
    format operation and it's a point where we can fail (see below).

 2. I think gdu-format-tool should bail out with an error dialog
    if the device is already mounted or otherwise busy. No need to
    check this upfront, just attempt the operation and DeviceKit-disks
    will return an error if it's busy.

    (it turns out it's *hard* to determine if a device is "busy" (with
     things like stacked blocked devices and all) so better keep that
     logic in DeviceKit-disks.)

    The Nautilus extension should try to unmount the device in question
    before invoking the format tool. This makes things a lot easier
    as calling g_mount_unmount() on a cleartext LUKS device will
    also tear down the LUKS mapping. And the nautilus extension should
    just complain with an error dialog if unmounting fails. 

    Just easier all around I think.

 3. If selecting a cleartext volume for LUKS, the nautilus extension
    launches the format tool with the device file for the cleartext
    device file e.g. /dev/dm-0. This is wrong, we should pass the
    encrypted device e.g. /dev/sdb1 or whatever to the format tool
    since this is typically what you want.

    Otherwise you can end up with this if you decide to use LUKS for
    formatting /dev/dm-0...
   
     - USB Thumbdrive (/dev/sdb)
      - Encrypted Data (/dev/sdb1, LUKS)
       - Encrypted Data (/dev/dm-0, LUKS)
        - My File System (/dev/dm-1, ext3 fs)

    It also fails because we are trying to modify partition data and
    that won't work because /dev/dm-0 is not a partition.

    Now, the question is how do you get /dev/sdb1 instead of /dev/dm-0?
    Turns out, that if the LUKS device is already "unmounted" then
    g_volume_get_identifier() will give you /dev/sdb1 instead of /dev/dm-0
    so this issue should go away when 2. is fixed.

 4. We really need to fix the problem with Nautilus popping up a dialog
    asking to unlock the volume when you format something with LUKS. I
    recently added the :device-detection-time property to DeviceKit-disks
    and will make GVfs make use of that to set the should_automount flag
    accordingly. That should hopefully fix that.

UI comments

 5. No need to show "[Icon] The volume is currently mounted" cf. 2. above.

 6. I like the Type combo box but people will probably want ext2 there
    as well for USB flash devices to avoid journaling (which can wear
    out flash). So suggest to add

     Compatible with Linux systems, No journal (ext2)

    Also please s/computers/systems/ for ext3 and LUKS to be consistent.
    I just pushed changes to DeviceKit-disks and gnome-disk-utility to
    support ext2.

 7. The "Open Disk Utility" button should use the Palimpsest icon

 8. No need for an additional confirmation dialog when pressing "Format"
    (we already tell the user in the existing dialog and the default action
    is "Close".

 9. When formatting is complete, the dialog still stays around. I think
    it should just exit...

10. When select ext3 and not modifying the default label ("davidz's data
    20090429") I get this error message

    org.freedesktop.DeviceKit.Disks.Error.Failed: Error creating file system: helper exited with exit code 1: given file system label exceeds 16 characters

    You should use gdu_known_filesystem_get_max_label_len() to adjust the
    maximum lenght of the GtkEntry. Also suggest to use a shorter default
    name that is guaranteed to fit in 16 bytes (suggest "Data YYYYMMDD").

11. Instead of "All existing data will be deleted" suggest to use
    "All existing data will be irrevocably erased. Make sure important
     data is backed up. This action cannot be undone". This might also
    help with GtkLabel deficiencies.

Next up are some comments on the code itself.

Some comments on the actual patch itself

 1. It's a bit superfluous to call the dir gdu-nautilus-extension, suggest
    to just call it nautilus-extension

 2. The source files says
    /* -*- Mode: C; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 8 -*- */
    but the code in them don't follow it. Now, I'm not going to force you
    to use emacs ;-), don't worry, but please reformat the code so it
    follows the rest of the code base.

 3. No trailing whitespace.. check, it all seems good!
    (if you use emacs, put "(setq-default show-trailing-whitespace t)") 

 4. Generally Gdu instead of GDU (cf. NautilusGDU)

 5. Some parameters are called in nautilus-gdu.c are called cvs,
    probably leftover from basing your extension on the CVS example
    extension.

 6. class is a C++ keyword, use klass instead (GLib style)

 7. never use return in the middle of a function, use goto instead

 8. use G_GNUC_CONST for _get_type() functions

 9. generally use gchar, gint instead of char, int. This is also
    helpful in reminding people that g_free() rather than free()
    should be used (so e.g. use char* only for strings to be freed
    with e.g. free() instead of g_free()).

10. as Matthias says, please avoid invading name spaces (g_icon_get_string()).

11. specifically if g_icon_get_string() fails, instead use gtk_window_set_icon()
    with a pixbuf obtained from gdu_util_get_pixbuf_for_presentable() using
    a reasonable icon size e.g. GTK_ICON_SIZE_DIALOG.

12. static FormatComboBoxItem filesystem_combo_items[] = {
    {"vfat", FALSE, "Compatible with all systems (FAT)"},
    {"ext3", FALSE, "Compatible with Linux computers (ext3)"},
    {"ext3", TRUE, "Encrypted, compatible with Linux computers (LUKS)"}

    use N_() and gettext() here. Also suggest to move to C file

13. format-window-operation.c : there's a lot of (dead) code here; I think
    the only thing you need is to do gdu_device_op_filesystem_create() and
    (for now) also obtain the authorization before doing that. When we
    port stuff to polkit 1.0, the app will not need to know about polkit
    at all.

14. It would be nice to mount the device after formatting it, kinda
    depends on getting item 4. from comment 17 fixed.

15. Need to add files to po/POTFILES.in

(In reply to comment #15)
> - It would be great if we could be intelligent and not show "All existing data
> will be deleted" if the medium is blank (like a blank cd, in my case). Likewise
> the scary confirmation dialog is not necessary for blank media.

Actually we should not pretend that we can handle formatting / blanking optical discs at all. They're not really intended for filesystems and is, in general, a support nightmare that we won't (or can't) deal with. Also, optical media is so last millennium ;-)

If people want something like this, it should in the Brasero or nautilus-cd-burner extensions.

> +#ifdef HAVE_CONFIG_H
> + #include <config.h> /* for GETTEXT_PACKAGE */
> +#endif

This is fixed in Tomas' repo


> +char *
> +g_icon_get_string (GIcon *icon)

This too


> So suggest to add
>
>     Compatible with Linux systems, No journal (ext2)

This too


>   Also please s/computers/systems/ for ext3 and LUKS to be consistent

This too


> 7. The "Open Disk Utility" button should use the Palimpsest icon

This too


> 8. No need for an additional confirmation dialog when pressing "Format"
>    (we already tell the user in the existing dialog and the default action
>    is "Close".

This too


> 10. When select ext3 and not modifying the default label ("davidz's data
>    20090429") I get this error message

This too.

Created attachment 133686 [details] [review]
fix initial window size

Created attachment 133697 [details] [review]
trivial fix

Created attachment 133698 [details] [review]
trivial fix

Created attachment 133699 [details] [review]
unmount before launching

Created attachment 133700 [details] [review]
simplifications

Created attachment 133701 [details] [review]
string improvement

Created attachment 133703 [details] [review]
trivial naming fix

Created attachment 133704 [details] [review]
trivial naming fix

Created attachment 133705 [details] [review]
coding style fixes

Created attachment 133706 [details] [review]
coding style fixes

Created attachment 133707 [details] [review]
indentation

after these patches, nautilus-gdu should be almost good to go, just figuring luks devices left to do.

Can I ask that we continue this excercise on a branch ?

(In reply to comment #32)
> after these patches, nautilus-gdu should be almost good to go, just figuring
> luks devices left to do.
> 
> Can I ask that we continue this excercise on a branch ?

Can you post a diff against master (git diff origin/master or something) or some other way so the patches can be review as a whole (we'll commit them as a series of course)? Thanks.

Created attachment 133711 [details] [review]
diff against origin/master

unfinished things:

- I ripped the unmounting and luks locking actions out of the formatting tool after adding the unmounting to the nautilus extension, but the partition handling code is still in 

- Need to figure out how to obtain right device for luks volumes

- Make dialog go away after formatting and mount the device

My patch includes fixes for (at least) 2, 3, 4, 5, 6, 8, 9, 15 of your code comments

I've committed a patch with some further formatting fixes and with gdu- stripped off the directory names. Oh, I also did the POTFILES.in additions.

The luks stuff still needs work, I got nautilus to crash playing with it.

I've fixed the remaining issues and also refactored the code into GtkDialog subclasses (we might want to move these to libgdu-gtk some day).