After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 770738 - Show list of available formats in the system
Show list of available formats in the system
Status: RESOLVED FIXED
Product: gnome-disk-utility
Classification: Core
Component: format dialog
3.20.x
Other Linux
: Normal enhancement
: ---
Assigned To: Kai Lüke
gnome-disk-utility-maint
Depends on:
Blocks:
 
 
Reported: 2016-09-02 08:15 UTC by Bastián Díaz
Modified: 2017-09-18 12:21 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Check available mkfs tooling (7.88 KB, patch)
2017-05-10 21:32 UTC, Kai Lüke
none Details | Review
Split encryption from filesystem selection (15.18 KB, patch)
2017-05-10 21:32 UTC, Kai Lüke
none Details | Review
List most supported other filesystems (9.97 KB, patch)
2017-05-10 21:33 UTC, Kai Lüke
none Details | Review
mockup (2.58 MB, image/png)
2017-05-11 08:19 UTC, Bastián Díaz
  Details
Format Redesign (41.50 KB, image/png)
2017-05-17 11:06 UTC, Kai Lüke
  Details
Format Redesign Select (44.97 KB, image/png)
2017-05-17 11:06 UTC, Kai Lüke
  Details
Format Redesign Select More (48.70 KB, image/png)
2017-05-17 11:07 UTC, Kai Lüke
  Details
Format Redesign Select Other (44.87 KB, image/png)
2017-05-17 11:07 UTC, Kai Lüke
  Details
Redesign Format Dialog (36.23 KB, patch)
2017-05-23 12:22 UTC, Kai Lüke
none Details | Review
Use header bar dialogs (18.64 KB, patch)
2017-05-23 12:22 UTC, Kai Lüke
none Details | Review
List filesystems in PopOver (14.84 KB, patch)
2017-05-23 12:23 UTC, Kai Lüke
none Details | Review
Expand filesystem list from UDisk (11.84 KB, patch)
2017-05-23 12:24 UTC, Kai Lüke
none Details | Review
Screenshot format1 (21.48 KB, image/png)
2017-05-23 12:28 UTC, Kai Lüke
  Details
Screenshot format2 (35.35 KB, image/png)
2017-05-23 12:28 UTC, Kai Lüke
  Details
Screenshot format3 (42.49 KB, image/png)
2017-05-23 12:29 UTC, Kai Lüke
  Details
Screenshot format4 (42.73 KB, image/png)
2017-05-23 12:29 UTC, Kai Lüke
  Details
Screenshot format5 (36.95 KB, image/png)
2017-05-23 12:30 UTC, Kai Lüke
  Details
Screenshot formatpwshow (31.19 KB, image/png)
2017-05-23 12:30 UTC, Kai Lüke
  Details
Screenshot formatcreatepart (39.57 KB, image/png)
2017-05-23 12:31 UTC, Kai Lüke
  Details
Format Volume (blogpost) (173.39 KB, image/png)
2017-06-11 22:51 UTC, Kai Lüke
  Details
Redesign Format Dialog (182.92 KB, patch)
2017-06-26 17:31 UTC, Kai Lüke
needs-work Details | Review
Move volume and partition utility functions (6.37 KB, patch)
2017-07-08 16:18 UTC, Kai Lüke
none Details | Review
Change initial partition size units to be greater (1.64 KB, patch)
2017-07-08 16:18 UTC, Kai Lüke
none Details | Review
Update Partition and Format Dialog for Redesign (43.56 KB, patch)
2017-07-08 16:20 UTC, Kai Lüke
none Details | Review
Delete old files and rename partition and format dialog files (66.22 KB, patch)
2017-07-08 16:21 UTC, Kai Lüke
none Details | Review
Redesign Format Dialog (84.05 KB, patch)
2017-07-08 16:32 UTC, Kai Lüke
none Details | Review
Move volume and partition utility functions (7.56 KB, patch)
2017-07-09 20:09 UTC, Kai Lüke
needs-work Details | Review
Change initial partition size units to be greater (1.64 KB, patch)
2017-07-09 20:11 UTC, Kai Lüke
needs-work Details | Review
Update Partition and Format Dialog for Redesign (43.56 KB, patch)
2017-07-09 20:12 UTC, Kai Lüke
needs-work Details | Review
Delete old files, rename partition/format dialog files (66.22 KB, patch)
2017-07-09 20:13 UTC, Kai Lüke
needs-work Details | Review
Detect a recent enough UDisks version (1.20 KB, patch)
2017-07-09 20:13 UTC, Kai Lüke
needs-work Details | Review
Redesign Format Dialog (82.11 KB, patch)
2017-07-09 20:16 UTC, Kai Lüke
none Details | Review
Detect UDisks version 2.7.2 (from #676555) (1.04 KB, patch)
2017-07-24 15:03 UTC, Kai Lüke
none Details | Review
Move partition size helpers (#676555) (5.47 KB, patch)
2017-07-24 15:03 UTC, Kai Lüke
committed Details | Review
Move volume and partition utility functions (9.30 KB, patch)
2017-07-24 15:04 UTC, Kai Lüke
committed Details | Review
Redesign Format Dialog (207.60 KB, patch)
2017-07-24 15:08 UTC, Kai Lüke
needs-work Details | Review
Redesign Format Dialog (203.92 KB, patch)
2017-08-14 12:04 UTC, Kai Lüke
committed Details | Review

Description Bastián Díaz 2016-09-02 08:15:54 UTC
This is because my experience formatting an SSD in a format not available in the main format volume dialog (type "f2fs" in custom format and cross my fingers). Gnome disks do not have a way to know the formats currently supported by the system, which causes the user needs to use other forms outside the graphical  user interface provided.

To improve this behavior sugges

• Having two dialogues format: a) One simple for Nautilus (FAT, exFAT, NTFS, ext4) and one for gnome-disks in addition to the above, shows a list advanced with additional file formats available in the system.
• Add a similar "file system support" option found in Gparted

Cheers
Comment 1 Kai Lüke 2017-05-10 21:31:08 UTC
I think this is related to some of the other bugs and I try to find an approach for it, without having two dialogs depending from where Disks is invoked.

Here are the screenshots, open for discussion:
https://tubcloud.tu-berlin.de/index.php/apps/gallery/s/OEjKBJ30lg4EJ3o

(Ideally during action or in a file system support matrix close to the help there should be the option to spawn an installation of missing tools through PackageKit.)
Comment 2 Kai Lüke 2017-05-10 21:32:01 UTC
Created attachment 351584 [details] [review]
Check available mkfs tooling

Rightnow UDisks will fail if needed mkfs binaries are
not present when creating a filesystem, leading to
cryptic error messages in Disks.

As workaround until a query mechanism in UDisks is
implemented now Disks checks and hints for needed
binares in the format dialog. The list for supported
filesystems which can be supplied via D-Bus in UDisks
version 2.6 was copied in order to support current
distributions.
Comment 3 Kai Lüke 2017-05-10 21:32:35 UTC
Created attachment 351585 [details] [review]
Split encryption from filesystem selection

The principle of both Ext4 and LUKS+Ext4 being just
filesystem choices becomes strange if you would would
double entries for xfs or btrfs as well in order to
allow them to be in a LUKS partition.

The idea of splitting LUKS encryption from the filesystem
choice is not to hurt usability by higher complexity
but instead safe the additional format operation with
the desired filesystem if it is not Ext4.
Comment 4 Kai Lüke 2017-05-10 21:33:15 UTC
Created attachment 351586 [details] [review]
List most supported other filesystems

Custom filesystems were entered by typing but
matched anyway against a fixed list. So on one
hand it is more work to type but on the other it
is also not visible what the allowed values are.

Almost the full list of supported filesystems is now
offered to be selected, just ext2/3, minix, nilfs2 and
reiserfs have been left out for now since they might
be of no interest anymore for common tasks.
Comment 5 Ondrej Holy 2017-05-11 07:14:25 UTC
Thanks for the screenshots!

However, I don't think we want to dynamically add new elements in the dialogs this way, because it changes the dialog size. Maybe we can do something similar what is used in Settings -> Region & Language -> Language...

Maybe you can also find some inspiration in the following guidelines:
https://developer.gnome.org/hig/stable/

Allan, can you please take a look?
Comment 6 Ondrej Holy 2017-05-11 07:27:13 UTC
Also, I think we should not ideally list the unsupported fs types at all...
Comment 7 Bastián Díaz 2017-05-11 08:01:01 UTC
(In reply to Kai Lüke from comment #1)
> I think this is related to some of the other bugs and I try to find an
> approach for it, without having two dialogs depending from where Disks is
> invoked.
> 

Nice. When I proposed, I was thinking about how other systems such as Windows or MacOS work, where the file manager shows a very basic dialog (like the new and simplified dialog for compressing files in Nautilus) and for more options, you have to open a specialized software. 

> Here are the screenshots, open for discussion:
> https://tubcloud.tu-berlin.de/index.php/apps/gallery/s/OEjKBJ30lg4EJ3o
> 

Nice
 
> (Ideally during action or in a file system support matrix close to the help
> there should be the option to spawn an installation of missing tools through
> PackageKit.)

Would this include some supported encryption systems? Currently the Luks system is well supported, but it would be nice for the user to know that other systems are available that can be useful (such as disloker for read bitlocker devices)
Comment 8 Bastián Díaz 2017-05-11 08:19:25 UTC
Created attachment 351603 [details]
mockup

I made a small drawing of how I think the popup should be to format volumes.

It feels strange to be able to enable or disable encryption when not all file systems support this or even if in the future will add other encryption systems (veracrypt?).
One option would be to enable encryption from the headerbar, so the list displayed in the popover would be limited to the supported file systems.
Comment 9 Kai Lüke 2017-05-11 10:48:00 UTC
Thanks for the drawing!
Another option would be to select LUKS as 'first filesystem' and offer the option to specify the contained filesystem.

I am unhappy with the bumpy behavior of the dialog, too - e.g. when the passphrase options are shown. Would CSS transitions help or allocating the needed window space from the beginning?
Comment 10 Kai Lüke 2017-05-11 12:13:51 UTC
Ah, on the encryption support: there is not technical hurdle to use FAT on LUKS and maybe the windows port would also be fine with it – but yes, normally this won't be clicked for FAT and NTFS.
Comment 11 Kai Lüke 2017-05-11 12:46:05 UTC
Concerning the listing of filesystems lacking tools: With the Popup they could be moved into the More→ and be insensitive?
Comment 12 Bastián Díaz 2017-05-11 18:58:11 UTC
(In reply to Kai Lüke from comment #9)
> Thanks for the drawing!
> Another option would be to select LUKS as 'first filesystem' and offer the
> option to specify the contained filesystem.
> 

The problem is that either ext4, FAT or another file system, in the first instance LUKS is only compatible with Linux, as is Bitlocker in Windows and FileVault in macOS.


> I am unhappy with the bumpy behavior of the dialog, too - e.g. when the
> passphrase options are shown.

On other systems such as Windows or macOS the encryption option is independent of the formatting. It might be a good idea to consider it for gnome-disks.

> Would CSS transitions help or allocating the
> needed window space from the beginning?

I think that is something that can best solve a designer who knows well the GNOME HIG.


I don't know if it is possible to suggest in the list the recommended file systems for one type of device or its capacity and the rest of file systems to display them when opening "more".

Example:
SD or Pendrive = 4GB: FAT32 or ext3
SD or Pendrive greater than 4GB and less than 32GB: FAT32, ext3, exFAT, etc.
SD or Pendrive greater than 32GB: exFAT, F2FS, etc.
External HD: NTFS, ext4, xfs, etc.
External SD: NTFS, ext4, F2FS, exFAT, etc.

Recommendations of the SD association by type: https://www.sdcard.org/developers/overview/capacity/ or
Comment 13 Kai Lüke 2017-05-11 20:33:30 UTC
Currently this is done by preselection:
default: ext4
but for removable drives: FAT if flash storage or < 20GB, otherwise NTFS (if available)
Comment 14 Ondrej Holy 2017-05-16 09:40:21 UTC
Review of attachment 351584 [details] [review]:

Just some notes in the meantime...

::: src/disks/gducreatefilesystemwidget.c
@@ +266,3 @@
                                     gdu_utils_get_max_label_length (fstype));
   name = gtk_entry_get_text (GTK_ENTRY (widget->name_entry));
 

I think that unsupported filesystems should not be listed and consequently the following checks are not needed...

::: src/libgdu/gduutils.c
@@ +761,3 @@
 /* ---------------------------------------------------------------------------------------------------- */
 
+/* @TODO: udisks2 >= 2.6: org.freedesktop.UDisks2.Manager.SupportedFilesystems */

I think that UDisks requirement should be bumped (or detect available UDisks version) and the mentioned UDisks API should be used instead of hardcoding the list...

@@ +778,3 @@
+
+gchar *
+gdu_utils_get_tool_for_fs (const gchar *fstype)

It would be nice to have the following functionality available directly in UDisks...
Comment 15 Kai Lüke 2017-05-17 11:06:13 UTC
Created attachment 352020 [details]
Format Redesign

I worked the mockup out with navigating through the PopOver.

Yes, it would be good to hint that LUKS+FAT does not work on Windows out of the box (display on the right side of the encryption switch).
Comment 16 Kai Lüke 2017-05-17 11:06:39 UTC
Created attachment 352021 [details]
Format Redesign Select
Comment 17 Kai Lüke 2017-05-17 11:07:01 UTC
Created attachment 352022 [details]
Format Redesign Select More
Comment 18 Kai Lüke 2017-05-17 11:07:21 UTC
Created attachment 352023 [details]
Format Redesign Select Other
Comment 19 Bastián Díaz 2017-05-17 20:29:59 UTC
(In reply to Kai Lüke from comment #18)
> Created attachment 352023 [details]
> Format Redesign Select Other

I don't think it's necessary to add a button in the popover to install additional software. From my experience using GNOME I see two options IHMO...

A) Only show the file systems installed in the distro.
B) Show all file systems supported by gnome-disks (the least used ones can be hidden). When a non-installed file system is selected, a third-party software installation dialog will appear in the same way as when an audio codec is missing in Totem for example.

My suggestion for a list of file systems supported by gnome-disks I think it should be a different report, since it points to an option to be directly seen in gnome-disks (hamburguer icon) similar to Gparted, see: http://gparted.org/screens/gparted-file-system-support.png

great work
Cheers
Comment 20 Kai Lüke 2017-05-18 10:36:12 UTC
Ok, good to compare it with codecs, so B) would be an implicit way which also avoids having this support matrix in the interface (install buttons go there).

So it seems that hiding the uninstalled is the prefered way. Then we could just have a 'More' page and use the style of language selection with "…" to show all supported.
Comment 21 Kai Lüke 2017-05-23 12:22:15 UTC
Created attachment 352409 [details] [review]
Redesign Format Dialog

I implemented the new layout with some changes, still the patches need some refinement if we agree on the proposed UI.

My main change is to discard having different pages in the PopOver because I think since it is showing a list with entries of the same type. So I went for a list as known in e.g. language selection (control center → region or gedit programming language highlighting).

Also the requirement is UDisks 2.6 to query for the list. Tooling is not checked anymore here, this needs a different bug report to discuss.
Comment 22 Kai Lüke 2017-05-23 12:22:58 UTC
Created attachment 352410 [details] [review]
Use header bar dialogs
Comment 23 Kai Lüke 2017-05-23 12:23:40 UTC
Created attachment 352411 [details] [review]
List filesystems in PopOver
Comment 24 Kai Lüke 2017-05-23 12:24:07 UTC
Created attachment 352412 [details] [review]
Expand filesystem list from UDisk
Comment 25 Kai Lüke 2017-05-23 12:28:28 UTC
Created attachment 352413 [details]
Screenshot  format1

Here are some screenshots, but for better judgement you can also build it (UDisks 2.6 is in Debian experimental if you don't run e.g. Rawhide):
Patches are applied here:
git clone git@github.com:pothos/gnome-disk-utility.git
cd gnome-disk-utility
git checkout redesign-format-dialog
./autogen.sh && make && src/disks/gnome-disks
Comment 26 Kai Lüke 2017-05-23 12:28:44 UTC
Created attachment 352414 [details]
Screenshot  format2
Comment 27 Kai Lüke 2017-05-23 12:29:04 UTC
Created attachment 352415 [details]
Screenshot  format3
Comment 28 Kai Lüke 2017-05-23 12:29:32 UTC
Created attachment 352416 [details]
Screenshot  format4
Comment 29 Kai Lüke 2017-05-23 12:30:08 UTC
Created attachment 352418 [details]
Screenshot  format5
Comment 30 Kai Lüke 2017-05-23 12:30:51 UTC
Created attachment 352419 [details]
Screenshot  formatpwshow
Comment 31 Kai Lüke 2017-05-23 12:31:11 UTC
Created attachment 352420 [details]
Screenshot  formatcreatepart
Comment 32 Kai Lüke 2017-05-23 12:54:32 UTC
So the list can be expanded twice:
Initially the currently prefered choices are shown,
then the list of secondary options (coming from other bug reports) can be revealed,
finally, if wanted, all remaining supported filesystems can be shown.
Comment 33 Allan Day 2017-05-23 15:48:26 UTC
I was asked to comment on the screenshots earlier today. Usually I'd take more time to fully explore the problem space, but I'm going away on holiday tonight and wanted to leave you with something to go on.

My main reaction to this is that the list of formats is too long, and the explanations aren't helpful for each one. We shouldn't aim to show every possible format. Instead, we should only show those that are genuinely useful, and provide helpful labels that enable someone without specialist knowledge to pick the correct one.

As for the presentation of the list, here we run into some general issues with the design of this dialog. You might choose to ignore these issues for the time being, but they might be worth bearing in mind:

 * In generally the UI relies heavily on comboboxes/popovers, which makes it a lot of work to dig into. Generally comboboxes should be avoided if possible in this type of dialog, for this reason.

 * Windows shouldn't resize depending on the options you pick.

 * It would be better if the confirmation step could be included in the same window, rather having the slightly contradictory blue "format..." button that doesn't actually format.

 * Given the amount that's going on, it might well be worth splitting out the encryption part into a separate section.

Here's a very quick, rough mockup, to give you an idea of what I mean:

https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/disks/format-volume.png
Comment 34 Bastián Díaz 2017-05-26 18:15:28 UTC
(In reply to Allan Day from comment #33)

Nice, I only have some suggestions as users that I hope you see after your holiday (considering that it was a mockup created quickly)

• Considering that this dialogue will also be shown in Nautilus, the descriptions could be different. For Ext4 it should be clear that it is compatible with any Linux (it is even the best option to use encryption), NTFS has read only support in macOS and although it is true that FAT is compatible with older systems, it is not patent that useful and sometimes necessary to use it today for certain devices.
Example:
  • FAT   (For use with older systems and flash storage up to 32 GB)
  • exFAT (For use with all modern systems and devices for flash storage)
  • Ext4  (For use with any Linux system but not with other systems)
  • NTFS  (For use with Windows and read only in macOS)

• I guess the encryption option will not be selectable for incompatible file systems. It should also display a warning if a user chooses a file system whose description will not be met using encryption (eg, the user selects FAT and activates encryption (Luks), however it would only be compatible with Linux systems)

Cheers
Comment 35 Allan Day 2017-06-02 17:23:49 UTC
The mockup I linked to above has evolved considerably over the past couple of days, so I'd recommend having another look if you're interested.
Comment 36 Allan Day 2017-06-02 17:26:54 UTC
(In reply to Bastián Díaz from comment #34)
...
> Example:
>   • FAT   (For use with older systems and flash storage up to 32 GB)
>   • exFAT (For use with all modern systems and devices for flash storage)
>   • Ext4  (For use with any Linux system but not with other systems)
>   • NTFS  (For use with Windows and read only in macOS)
...

Is exFAT something we can realistically support? According to Wikipedia, it's proprietary and patent encumbered.

In general I prefer to give the usage first and the name of the filesystem second - it's friendlier to those who aren't familiar with filesystems.
Comment 37 Bastián Díaz 2017-06-02 18:26:28 UTC
(In reply to Allan Day from comment #36)
> 
> Is exFAT something we can realistically support? According to Wikipedia,
> it's proprietary and patent encumbered.
> 

That's true, but it's not something new coming from Microsoft. However, here we are not talking about software distribution, but simply giving support in UI if it's installed in the system (fuse-exfat and exfat-utils) and in the case of fedora it is usually in the main repositories .
Microsoft has not made any moves with any non-commercial implementation and I doubt that MS don't know fuse-exfat project. It would be impractical to do so, considering its intentions to make exFat a standard and interoperate with products that use and recommend it, such as those coming from the SD association.

Maybe a lawyer can say something with more knowledge.

> In general I prefer to give the usage first and the name of the filesystem
> second - it's friendlier to those who aren't familiar with filesystems.

Of course, I just gave an example of labels, not how I had to look.
Comment 38 Kai Lüke 2017-06-04 21:30:44 UTC
Hello,

thanks, I'm happy with the new proposal, it will just involve more changes to move from dialogs to GtkAssistant.
Some questions ahead: Currently Disks explains the cases when secure erasing should be used in the confirmation dialog - do you think that kind of text should be displayed somewhere or just go away? If it's wanted the whole 'Erase' option could go to the second page.

I also linked a bug requesting support for encryption with other filesystems. Maybe this checkbox should also fade in for the choice of 'Other'?
In contrast to not including filesystem choices this is not so serious since a simple workaround is to reformat the contained filesystem.
https://bugzilla.gnome.org/show_bug.cgi?id=783390

For the topic of showing available filesystems I think it's good to stick with the design proposal for now without strange exFAT and later include e.g. an app preference to have more options in the 'Other' list because this discussion will always come up ;)

Regards,
Kai
Comment 39 Charles 2017-06-04 22:37:17 UTC
Just my two cents.
Reading the discussion I realize that probably my ticket is not very appropriate. If the design perspective is different, you can close my report, since I can always use the terminal for those tasks.
However, seeing this mockup (https://github.com/gnome-design-team/gnome-mockups/blob/master/disks/format-volume.png) and being a fan of offering the best options in Linux for Linux users, only allowing use of ext4 FS for encryption is not the best and is an old discussion. Only an advanced user would go beyond and disable the journalig and other things for their optimization in flash devices.
Like the binomial NTFS and FAT file systems, in Linux should be enhanced ext4 and F2FS. About exFAT I can only say that even Veracrypt shows it in its options when it is available on the system on any platform.

Will the disk format dialog also change? I mean the one on the hamburger icon in disk. Thank you.

@Kai I see that you are a GSoC student, my best wishes for you.
Comment 40 Ondrej Holy 2017-06-05 08:46:07 UTC
Allan, thanks, the proposal looks really great!

Just I am not sure that the other selection button is the best idea because I think it is not obvious that you have to click on it and choose the filesystem before you can click "Next". 

I agree I would ignore exFAT since it is not part of kernel, is proprietary and usually is not installed by default.

Maybe it would be better to have a separate page of the assistant with encryption settings as you proposed initially than just limiting this to ext4. But we should warn that it doesn't have to work with other operation systems by default etc.
Comment 41 Allan Day 2017-06-07 10:56:47 UTC
I've update the mockup, primarily to tweak which filesystems are offered.

 * exFAT removed, since it's proprietary and not straightforward to install on many Linux distros
 * f2fs removed, since it's not clear how it would be useful. The only possible application I can see would be to use it for internal SSDs, but it seems to be too experimental to recommend at this stage.
 * No filesystem removed, since it doesn't really fit into the context of a "format" operation and is only useful if erasing
 * Added the ability to encrypt XFS filesystems
Comment 42 Bastián Díaz 2017-06-07 17:33:55 UTC
(In reply to Allan Day from comment #41)
> I've update the mockup, primarily to tweak which filesystems are offered.
> 
>  * exFAT removed, since it's proprietary and not straightforward to install
> on many Linux distros

Bug 770733 must be closed. I understand that patents are a strong reason, but it's not difficult to install in many distros. In fact I have not found a distro that doesn't have the packages, even in totally free distros like GuixSD.

>  * f2fs removed, since it's not clear how it would be useful. The only
> possible application I can see would be to use it for internal SSDs, but it
> seems to be too experimental to recommend at this stage.

F2FS stopped being experimental a long time ago and is part of the Kernel. It's used internally on many Android devices by default as an alternative to ext4 and is a very good file system for other flash devices (not only SSD). 

>  * No filesystem removed, since it doesn't really fit into the context of a
> "format" operation and is only useful if erasing
>  * Added the ability to encrypt XFS filesystems

Following the same logic, XFS would not make any sense to a common user, unless it is a Red Hat business client (16 TB?).
In principle the spirit of the report was to add more options than those currently available to use disks as a replacement for gparted. The redesign is a very good thing, but in the final form it only adds a new file system (XFS). 
The option to choose a custom file system had a terrible usability, but at least it allowed to choose something more graphically IMHO.

Regards.
Comment 43 Allan Day 2017-06-08 11:43:49 UTC
(In reply to Bastián Díaz from comment #42)
> (In reply to Allan Day from comment #41)
> > I've update the mockup, primarily to tweak which filesystems are offered.
> > 
> >  * exFAT removed, since it's proprietary and not straightforward to install
> > on many Linux distros
> 
> Bug 770733 must be closed. I understand that patents are a strong reason,
> but it's not difficult to install in many distros. In fact I have not found
> a distro that doesn't have the packages, even in totally free distros like
> GuixSD.

My understanding is that it requires RPM fusion on Fedora. That's definitely in 3rd party territory. There's also an issue regarding what exFAT is good for - since it's not generally installed on Linux distros, it's not a great fit for removable devices that you might want to use with other users' machines.

> >  * f2fs removed, since it's not clear how it would be useful. The only
> > possible application I can see would be to use it for internal SSDs, but it
> > seems to be too experimental to recommend at this stage.
> 
> F2FS stopped being experimental a long time ago and is part of the Kernel.
> It's used internally on many Android devices by default as an alternative to
> ext4 and is a very good file system for other flash devices (not only SSD). 

I've been talking to the udisks developers. They couldn't give a good reason why people would use it, and didn't know of anyone using it. I've also searched the web for information about using f2fs with popular distros. I couldn't find any recent posts or documentation regarding why or how it should be used.
 
> >  * No filesystem removed, since it doesn't really fit into the context of a
> > "format" operation and is only useful if erasing
> >  * Added the ability to encrypt XFS filesystems
> 
> Following the same logic, XFS would not make any sense to a common user,
> unless it is a Red Hat business client (16 TB?).

If 16 TB disks aren't something we need to support, I completely agree that XFS shouldn't be in there. The reason I did include it is that there's a major distro that uses it by default, and there's documentation for it on the web.

> In principle the spirit of the report was to add more options than those
> currently available to use disks as a replacement for gparted. The redesign
> is a very good thing, but in the final form it only adds a new file system
> (XFS). 

When I said that I'd look at the design of this, I agreed that it would be a good idea to review the filesystem options. However, we shouldn't lose sight of the purpose of this dialog. The goal is not to expose all technical possibilities. It is not even to expose all the options that any user might want. Instead, it is to enable users to do common tasks, without requiring specialist knowledge, and to only present options that are well-supported and have clear uses.
Comment 44 Bastián Díaz 2017-06-08 17:54:48 UTC
(In reply to Allan Day from comment #43)
> (In reply to Bastián Díaz from comment #42)
> > (In reply to Allan Day from comment #41)
> 
> My understanding is that it requires RPM fusion on Fedora. That's definitely
> in 3rd party territory. 

Of course, each distribution is classified differently, but the point is that it is not something difficult to install in many distros. Consider that back then it was the same thing that users had to do to get full support for NTFS.

> There's also an issue regarding what exFAT is good
> for - since it's not generally installed on Linux distros, it's not a great
> fit for removable devices that you might want to use with other users'
> machines.
> 

If you consider the use of removable devices only with GNU/Linux users, something in practice is unlikely and that would be a good example to use a linux native format. Nowadays most of the pendrives and SD cards of certain capacity come preformatted with exFAT and is the new standard. A user can use this format and share contents of a pendrive with Windows and MacOS users alike without asking them to install additional software, etc.
I have already understood that the file system will not be added and that the subject matter of patents is reasonable, but I don't agree with the other arguments you have given.

> 
> I've been talking to the udisks developers. They couldn't give a good reason
> why people would use it, and didn't know of anyone using it. I've also
> searched the web for information about using f2fs with popular distros. I
> couldn't find any recent posts or documentation regarding why or how it
> should be used.
>  

I agree, only a group of users use this file system and is currently only accessible for advanced users, but it is not something totally unknown. Websites like phoronix have shown their potential as a unique (known) file system created specifically for flash devices.
It is usually more popular to maintain the factory fs or use FAT as the only available alternative, but it is a common mistake to recommend using ext4 or another journaled file system.
F2FS would satisfy a specific but unpopular use case: a) a user who wants to use a native Linux file system, b) that is suitable for flash devices and c) can be encrypted

> 
> If 16 TB disks aren't something we need to support, I completely agree that
> XFS shouldn't be in there. 
>

Then I realized that this was the limit of ext4, something that a common user would never use.

>The reason I did include it is that there's a
> major distro that uses it by default, and there's documentation for it on
> the web.
> 

Yeah, RHEL... However there are other major distros that include Btrfs or OpenZFS too.

> 
> When I said that I'd look at the design of this, I agreed that it would be a
> good idea to review the filesystem options. However, we shouldn't lose sight
> of the purpose of this dialog. The goal is not to expose all technical
> possibilities. It is not even to expose all the options that any user might
> want. Instead, it is to enable users to do common tasks, without requiring
> specialist knowledge, and to only present options that are well-supported
> and have clear uses.

I would agree if it were only used in Nautilus, but a more knowledgeable user who would use gnome-disks would find it insufficient.
Comment 45 Charles 2017-06-09 02:52:10 UTC
(In reply to Ondrej Holy from comment #40)
> 
> Maybe it would be better to have a separate page of the assistant with
> encryption settings as you proposed initially than just limiting this to
> ext4. But we should warn that it doesn't have to work with other operation
> systems by default etc.
 
I agree with this, filling with checkboxes doesnt look like a good thing. 

...

On the other hand I dont think that the vision should be so biased (fedora/redhat?), For example several distros dont preinstall xfsprogs by default, so f2fs and btrfs are useful in Linux world to be considered. 
To talk about something inappropriate would be to create LVM partitions.
Comment 46 Allan Day 2017-06-09 11:45:48 UTC
(In reply to Charles from comment #45)
> (In reply to Ondrej Holy from comment #40)
> > 
> > Maybe it would be better to have a separate page of the assistant with
> > encryption settings as you proposed initially than just limiting this to
> > ext4. But we should warn that it doesn't have to work with other operation
> > systems by default etc.
>  
> I agree with this, filling with checkboxes doesnt look like a good thing. ...

Having the encryption option on the first page allows us to skip the encryption page if it's not being used. That's quite a lot of overhead for users, especially when encryption probably won't be the common operation.

> On the other hand I dont think that the vision should be so biased
> (fedora/redhat?)

I've been searching for documentation regarding the other big distros too, fwiw. The goal is to find a set of options that work out of the box with the main big distros.
Comment 47 Allan Day 2017-06-09 12:06:45 UTC
(In reply to Bastián Díaz from comment #44)

I'm replying selectively here. I think we're going round in circles on some of the points.

...
> > There's also an issue regarding what exFAT is good
> > for - since it's not generally installed on Linux distros, it's not a great
> > fit for removable devices that you might want to use with other users'
> > machines.
> 
> If you consider the use of removable devices only with GNU/Linux users,
> something in practice is unlikely and that would be a good example to use a
> linux native format. Nowadays most of the pendrives and SD cards of certain
> capacity come preformatted with exFAT and is the new standard.

Everything I've seen suggests that FAT is still the defacto format for removable storage and the devices that use it. If you want to present alternative evidence please do so.

> A user can
> use this format and share contents of a pendrive with Windows and MacOS
> users alike without asking them to install additional software, etc.

But my point is that they can't generally then share it with someone on another Linux distro, without that person having to install something special (and not everyone will be able to do that).

> I have already understood that the file system will not be added and that
> the subject matter of patents is reasonable, but I don't agree with the
> other arguments you have given.
> 
> > 
> > I've been talking to the udisks developers. They couldn't give a good reason
> > why people would use it, and didn't know of anyone using it. I've also
> > searched the web for information about using f2fs with popular distros. I
> > couldn't find any recent posts or documentation regarding why or how it
> > should be used.
> 
> I agree, only a group of users use this file system and is currently only
> accessible for advanced users, but it is not something totally unknown.
> Websites like phoronix have shown their potential as a unique (known) file
> system created specifically for flash devices.

Sorry, I don't think we should include a filesystem option because phoronix has said that it has potential. ;)

...
> F2FS would satisfy a specific but unpopular use case: a) a user who wants to
> use a native Linux file system, b) that is suitable for flash devices and c)
> can be encrypted
> 
...
> >The reason I did include it is that there's a
> > major distro that uses it by default, and there's documentation for it on
> > the web.
> 
> Yeah, RHEL... However there are other major distros that include Btrfs or
> OpenZFS too.

It's not just a matter of a filesystem being available or even having a purpose. These things need to be tested, supported, documented, reasonably widely used, have some assurance of integration and quality.
Comment 48 Bastián Díaz 2017-06-09 16:46:21 UTC
(In reply to Allan Day from comment #47)

>
> I'm replying selectively here. I think we're going round in circles on some
> of the points.
> 

Of course, it is somewhat difficult to get something forward when the concessions are unclear.

> 
> Everything I've seen suggests that FAT is still the defacto format for
> removable storage and the devices that use it. If you want to present
> alternative evidence please do so.
> 

Everything?
As I mentioned, exFAT is part of the SDXC specification and flash memory makers recommend it for storage over 16 GB and sell preformatted device with that file system. OS, xbox and another "new" third party devices, etc.
The problem is not to demonstrate if it's a de facto standard or is really useful, because it is, but here the situation occurs when one standard overlaps another and if one works it seems that there is no need to support the new.

The issue of patents or licensing is a reasonable limitation, but it makes no sense to argue that the necessary packages were not available in most distros or difficult to install.

> 
> But my point is that they can't generally then share it with someone on
> another Linux distro, without that person having to install something
> special (and not everyone will be able to do that).
> 

Assuming the user is silly. The user will not be forced to use X or Y, he will simply have the options to choose according to the situation. The FAT size limitation per file is well known.

> 
> Sorry, I don't think we should include a filesystem option because phoronix
> has said that it has potential. ;)
> 

Sorry for thinking you had perspective. At no point did I say that it should be added because Phoronix says... but I used it as an example because it is a serious web that tests interesting for our platform. Irony is not good for any debate.

Did you have any thoughts on the use case that I mentioned? That would make the discussion better.

> 
> It's not just a matter of a filesystem being available or even having a
> purpose. 

All FS has a purpose and is not about listing everything FS that supports the Linux Kernel.

> These things need to be tested, supported, documented, reasonably
> widely used, have some assurance of integration and quality.

That is ideal and I agree, but it doesn't give much in the ecosystem and under that perspective would only negatively limit the options.
Comment 49 Charles 2017-06-09 17:09:00 UTC
(In reply to Allan Day from comment #46)
> (In reply to Charles from comment #45)
> > (In reply to Ondrej Holy from comment #40)
> > > 
> > > Maybe it would be better to have a separate page of the assistant with
> > > encryption settings as you proposed initially than just limiting this to
> > > ext4. But we should warn that it doesn't have to work with other operation
> > > systems by default etc.
> >  
> > I agree with this, filling with checkboxes doesnt look like a good thing. ...
> 
> Having the encryption option on the first page allows us to skip the
> encryption page if it's not being used. That's quite a lot of overhead for
> users, especially when encryption probably won't be the common operation.
 
That's why I preferred other window to use encryption. I do not see problem using Luks with fat for example.

> > On the other hand I dont think that the vision should be so biased
> > (fedora/redhat?)
> 
> I've been searching for documentation regarding the other big distros too,
> fwiw. The goal is to find a set of options that work out of the box with the
> main big distros.

Each distro has its own interests and way of working, so I find it difficult. It is best to choose a list of presets and display them when they are available on the system.
Comment 50 Charles 2017-06-09 17:56:37 UTC
(In reply to Bastián Díaz from comment #48)
> 
> Everything?
> As I mentioned, exFAT is part of the SDXC specification and flash memory
> makers recommend it for storage over 16 GB and sell preformatted device with
> that file system. OS, xbox and another "new" third party devices, etc.
> The problem is not to demonstrate if it's a de facto standard or is really
> useful, because it is, but here the situation occurs when one standard
> overlaps another and if one works it seems that there is no need to support
> the new.
> 

I agree with this. One must be blind to obviate that exfat is the new standard, but devs will have to spend more years to be considered, as usually happens in Linux. I myself experienced the process of finding a "how to" in the web to use my new pendrive in my favorite distro. I remember a related recently post in planet gnome.

https://lleksah.wordpress.com/2017/05/24/formatting-a-new-extfat-usb-on-fedora/

For my f2fs and exfat are into the same category, the difference is that one is native to Linux and the other allows me to interoperate with other systems. Both let me carry my DVD iso or the long videos on my digital camera where fat doesnt work.

@Allan Asking another person to install additional software to use a device is very common. I live surrounded by Windows users.
Comment 51 Kai Lüke 2017-06-11 22:51:55 UTC
Created attachment 353584 [details]
Format Volume (blogpost)

Hello,

thanks for the passion in finding a good solution for this issue. I made a blog post from my perspective on how the discussion is going. The suggestion in the end (and here attached) is similar to what is found in installers since we are already in the wizard dialog domain now. What do you think of it?

http://pothos.blogsport.eu/2017/06/12/an-observation-in-ui-design/

Kind regards,
Kai
Comment 52 Kai Lüke 2017-06-14 09:59:59 UTC
In relation to Nautilus' compression dialog (it shows zip, xz and 7zip but no custom option) the 'Custom' option could also just go away by default. A preference for Disks would toggle it to be available: "Show format with custom file system".
Comment 53 Bastián Díaz 2017-06-14 18:55:54 UTC
(In reply to Kai Lüke from comment #52)
> In relation to Nautilus' compression dialog (it shows zip, xz and 7zip but
> no custom option) the 'Custom' option could also just go away by default.

I agree with you. The custom option forces the user to have a knowledge of the topic.

> A preference for Disks would toggle it to be available: "Show format with
> custom file system".

If you follow your design would be the best option. On the other hand, I don't consider Allan's design to be bad, but sinning in diminishing the options to that point makes it little useful from my perspective.
Comment 54 Kai Lüke 2017-06-26 17:31:05 UTC
Created attachment 354526 [details] [review]
Redesign Format Dialog

Hello,

I've implemented it with minor changes due to using GtkAssistant (e.g. Cancel button) and it also applies to adding partitions.
The custom options is available from within Disks but not from Nautilus, assuming that this will be the most relevant use case.

The button in the header bar is grey because I could not get it done with CSS.

This is not a small change because the things depend on each other through the pages transitions. Once this is in I hope it will be quicker to iterate on the design.

Greetings,
Kai
Comment 55 Ondrej Holy 2017-06-29 13:14:41 UTC
Review of attachment 354526 [details] [review]:

Thanks for the patch. It really contains a lot of changes. It would be really really nice to split in more patches (e.g. separate patch for the functions moved to gduutils, update format volume first and then move in another patch and append create partition stuff), because you are reusing a lot of code from the removed files, but it is hard to track what is new and what is just moved etc.

Wouldn't be better to have separate widgets such as GduCreatePartitionWidget, GduCreateFilesystemWidget, GduPasswordWidget and just interconnect them in GduCreateFormat? I think it is a step back when you are removing some of those, it will be harder to maintain...

Extended partition creation doesn't work...

If I going to create a second partition it creates the partition but formats the already existing once! I have not got any time to find why... be careful, it can cause data loss!

Use "Format" ("Create") button instead of "Apply", it should also have a red color, "Cancel" should be only on the first page.

Please try to match the borders, margins, padding as are in the mockup (e.g. bigger spaces between name, erase, type part)!

I've seen the following critical, please try to fix:
(gnome-disks:8940): Gtk-CRITICAL **: gtk_assistant_set_page_has_padding: assertion 'child != NULL' failed

This is pre-existing bug, but would be nice to fix it also (in a separate patch)... together with UI updates for the password page as per mockup.
(gnome-disks:8940): Gtk-WARNING **: Negative content width -2 (allocation 0, extents 1x1) while allocating gadget (node block, owner GtkLevelBar)

::: src/disks/gduapplication.c
@@ +266,3 @@
       g_application_hold (_app);
+      gdu_create_format_show (app->client, NULL, object_to_select,
+                              FALSE, FALSE, 0, 0, (GCallback)g_application_release, _app);

Add space after (GCallback).

::: src/disks/gducreateformat.c
@@ +25,3 @@
+#define CUSTOM_PAGE_NR 2
+#define PASSWORD_PAGE_NR 3
+#define CONFIRM_PAGE_NR 4

enum?

@@ +27,3 @@
+#define CONFIRM_PAGE_NR 4
+
+/* TODO: use libbytesize which comes as dependency for libblockdev and UDisks 2.7 */

That would be really nice...

@@ +30,3 @@
+/* Keep in sync with Glade file */
+static const guint64 unit_sizes[NUM_UNITS] = {
+  (1ULL),                /*  0: bytes */

Isn't guint64 just UL? Consider using G_GUINT64_CONSTANT...

@@ +41,3 @@
+  ((1ULL)<<40),          /*  9: TiB */
+  ((1ULL)<<50),          /* 10: PiB */
+};

Maybe would be also nice to create enum with MB, TB... and then you can use unit_sizes[TB] in some calculations instead of hardcoded 100ULL... this code is also good adept for gduutils..

@@ +158,3 @@
+    }
+
+  if (gtk_adjustment_get_value (data->size_adjustment) > 0)

&& !show_dos_error ?

@@ +184,3 @@
+
+static void
+set_unit_num (CreateFormatData *data, gint unit_num)

I don't care about the unit-related functions since they are copied...

@@ +377,3 @@
+  gboolean has_next;
+
+  if (data->add_partition) /* no confirmation page needed,

Actually, I think it would be nice to show confirmation page (aka summary) also in this case, but without the warning obviously...

@@ +378,3 @@
+
+  if (data->add_partition) /* no confirmation page needed,
+                            * set format/custom format pages as last depending if password/custom FS page is following */

Would be better to add such a long comment on an empty line before the if-command...

@@ +379,3 @@
+  if (data->add_partition) /* no confirmation page needed,
+                            * set format/custom format pages as last depending if password/custom FS page is following */
+    {

Would be nice to handle the following in some separate function depending on already set values...

@@ +497,3 @@
+    g_variant_builder_add (&options_builder, "{sv}", "label", g_variant_new_string (gtk_entry_get_text (data->fs_name_entry)));
+
+  if (!(g_strcmp0 (fs_type, "vfat") == 0 || g_strcmp0 (fs_type, "ntfs") == 0|| g_strcmp0 (fs_type, "exfat") == 0))

Missing space before ||.

Wouldn't be nicer to use != && instead of negation?

@@ +617,3 @@
+  gchar *s1;
+  gchar *s2;
+  gint offset = data->add_partition ? 0 : 1;  /* the partition page might not be present */

It would be better to provide e.g. get_page_number (CUSTOM_PAGE) function to deal with the offsets "internally"...

@@ +619,3 @@
+  gint offset = data->add_partition ? 0 : 1;  /* the partition page might not be present */
+
+  /* gather data on current device usage for the confirmation page */

Would be nice to set the following only once somewhere and not on each prepare signal...

@@ +622,3 @@
+  info = udisks_client_get_object_info (data->client, data->object);
+  unused_space = gdu_utils_get_unused_for_block (data->client, data->block);
+  s1 =  g_strdup_printf ("%s — %s", udisks_object_info_get_description (info),

I think you should use _("") and add comment for translators...

@@ +638,3 @@
+      gtk_widget_show (GTK_WIDGET (data->used_amount_label));
+      s1 = udisks_client_get_size_for_display (data->client, size - unused_space, FALSE, FALSE);
+      s2 = g_strdup_printf ("%s (%.1f%%)", s1, 100.0 * (size - unused_space) / size);

I think you should use _("") and add comment for translators...

@@ +674,3 @@
+                        guint64       add_partition_maxsize,
+                        GCallback     finished_cb,
+                        gpointer      cb_data)

gdu_create_format_show is half-synchronous and half-asynchrounous. I see that it used to be there before, but I am not sure it is a right way to go since you are rewriting the code from scratch. Maybe would be better to split the dialog class and the logic...

@@ +704,3 @@
+  data->cur_unit_num = -1;
+
+  data->part_dos_extended_checkbutton  = GTK_CHECK_BUTTON (gtk_builder_get_object (data->builder, "dos-extended-checkbutton"));

Double space before ==...

@@ +712,3 @@
+
+  data->infobar_vbox = GTK_WIDGET (gtk_builder_get_object (data->builder, "infobar-vbox"));;
+  data->dos_error_infobar = gdu_utils_create_info_bar (GTK_MESSAGE_ERROR,

Wouldn't be better to create the info bar only if really needed?

@@ +760,3 @@
+  g_signal_connect (data->other_encrypt_checkbutton, "toggled", G_CALLBACK (on_fs_type_changed), data);
+
+  /* custom format page content */

if (show_custom) { ?

@@ +761,3 @@
+
+  /* custom format page content */
+  scrolled = gtk_scrolled_window_new (NULL, NULL);

I think that the scrolled window is not needed here and is rather confusing in the UI.

@@ +775,3 @@
+  fs_radio = row_adder (data, "exfat", "exFAT Flash Storage Windows Filesystem", fs_radio);
+  fs_radio = row_adder (data, "udf", "UDF CD/DVD/BD Filesystem", fs_radio);
+  fs_radio = row_adder (data, "empty", "No Filesystem", fs_radio);

You should mark the description as translatable - _("").

The labels are not easily readable in my humble opinion. Wouldn't be better to use similar formatting as is used for the main fs, or dashes, or so...
e.g.  Volume larger than 16TB, Linux only (XFS), Another Linux Filesystem (XFS), XFS - Linux Filesystem...

Shouldn't we also check that those fs are available?

@@ +811,3 @@
+  else /* no partition added, format page is now the first */
+    {
+      gtk_assistant_remove_page (data->assistant, PART_PAGE_NR);

Wouldn't be better to just skip the page and avoid using the offsets?

@@ +814,3 @@
+    }
+
+  /* workaround to hide the GtkAssistant sidebar but still have page titles */

So not sure whether it won't be better to use GtkDialog with GtkStack instead of such hacks...

@@ +832,3 @@
+    {
+      /* default FAT for flash and disks/media smaller than 20G (assumed to be flash cards) */
+      if (gdu_utils_is_flash (data->drive) || udisks_drive_get_size (data->drive) < (guint64)(20ULL * 1000ULL*1000ULL*1000ULL))

|| !gdu_utils_is_ntfs_available () ?

As far as I know gunit64 is UL, so it would be probably better to use just UL without casting, or G_GUINT64_CONSTANT... is it needed at all?

::: src/disks/ui/create-format.ui
@@ +38,3 @@
+            <property name="orientation">vertical</property>
+            <child>
+              <placeholder/>

Would be nice to remove the placeholders... also on other places...

@@ +100,3 @@
+                    <property name="tooltip_markup" translatable="yes">The size of the partition to create, in megabytes</property>
+                    <property name="tooltip_text" translatable="yes">The size of the partition to create, in megabytes</property>
+                    <property name="invisible_char">●</property>

I don't understand why glade adds unnecessary stuff, for example, invisible_char... I would remove it... but it is probably ok...

@@ +259,3 @@
+            <property name="can_focus">False</property>
+            <property name="halign">start</property>
+            <property name="label" translatable="yes">&lt;small&gt;For example: "Angela's Files" or "Backup".&lt;/small&gt;</property>

I think it is better to set font-size, or some class instead of adding markups in the translatable strings, applies on other places also...

::: src/libgdu/gduutils.c
@@ +604,3 @@
+        {
+          ret = TRUE;
+          goto out;

Please don't use goto if not needed, break is perfectly ok here.

@@ +614,3 @@
+guint
+gdu_utils_count_primary_dos_partitions (UDisksClient *client,
+                                        UDisksPartitionTable *table)

Please align the identificators.

@@ +617,3 @@
+{
+  GList *partitions, *l;
+  guint ret = 0;

\n

@@ +624,3 @@
+      if (!udisks_partition_get_is_contained (partition))
+        ret += 1;
+    }

\n

@@ +626,3 @@
+    }
+  g_list_foreach (partitions, (GFunc) g_object_unref, NULL);
+  g_list_free (partitions);

\n

@@ +632,3 @@
+gboolean
+gdu_utils_have_dos_extended (UDisksClient *client,
+                             UDisksPartitionTable *table)

dtto

@@ +654,3 @@
+gdu_utils_is_inside_dos_extended (UDisksClient *client,
+                                  UDisksPartitionTable *table,
+                                  guint64 offset)

dtto

::: src/libgdu/gduutils.h
@@ +51,3 @@
+
+guint gdu_utils_count_primary_dos_partitions (UDisksClient *client,
+                                              UDisksPartitionTable *table);

Please align the identificators to be consistent as much as possible.

@@ +54,3 @@
+
+gboolean gdu_utils_have_dos_extended (UDisksClient *client,
+                                      UDisksPartitionTable *table);

dtto

@@ +58,3 @@
+gboolean gdu_utils_is_inside_dos_extended (UDisksClient *client,
+                                           UDisksPartitionTable *table,
+                                           guint64 offset);

dtto
Comment 56 Kai Lüke 2017-06-30 09:55:57 UTC
Thanks for the feedback, I'll have a look into GtkStack to simplify the page logic and use the properties of new widget classes as clear separation for the pages. I would still like to have all stuff in a single glade file, without the placeholders it would not be able to edit it anymore, but I'll try to filter it more.

Creating a logical partition with UDisks < 2.7 will look strange because the udev notifications are wrong and so the partitions misdisplayed. If you detach and attach the loop device or trigger a udev event for the device I think it will be ok again. Anyway the way Disks calculates the size is not ok, because it promises too much and should take the 1 MB steps in extended partitions into account (it will demand a too big partition otherwise). If it really took your first partition then this is also related to the gap which has to come and there was a recent change for selecting the new partition in UDisks which could be solving this. Also the best would be to set the maximal partition size '0' if it's not changed anyway.
Comment 57 Pali 2017-07-05 00:53:41 UTC
> +  fs_radio = row_adder (data, "udf", "UDF CD/DVD/BD Filesystem", fs_radio);

Maybe you should add better description. UDF is not only for optical disks, but can be used also on hard disks or usb flash disks. UDF is really Universal Disk Format. Note that support for random access read/write for hard disks was in UDF standard before support for rewritable optical disks...
Comment 58 Kai Lüke 2017-07-08 16:18:16 UTC
Created attachment 355171 [details] [review]
Move volume and partition utility functions
Comment 59 Kai Lüke 2017-07-08 16:18:59 UTC
Created attachment 355172 [details] [review]
Change initial partition size units to be greater
Comment 60 Kai Lüke 2017-07-08 16:20:39 UTC
Created attachment 355173 [details] [review]
Update Partition and Format Dialog for Redesign
Comment 61 Kai Lüke 2017-07-08 16:21:54 UTC
Created attachment 355174 [details] [review]
Delete old files and rename partition and format dialog files
Comment 62 Kai Lüke 2017-07-08 16:32:54 UTC
Created attachment 355175 [details] [review]
Redesign Format Dialog

Ok, I've done my best to split it in several patches. The longer this bug here stays open the more complicated it gets to handle the needed changes. I really hope that this here is the final state and all other stuff which is not too essential for this bug goes into new bug reports (and in smaller patches ;) ).

Now I've separated the page content logic in their own files with defined functions to get the information of a page.
Because of the interdependence I think the solution to handle all the paging state of the dialog in only one function is better to maintain.

Best regards,
Kai
Comment 63 Pali 2017-07-08 20:15:54 UTC
>+    return _("UDF – Universal Disk Format, like FAT and used on optical media");

Not "like", but "unlike" FAT :-) Unlike FAT, UDF supports POSIX permissions, 2GB+ large files, symlinks, ...

What about this?

"UDF – Universal Disk Format (Windows, Linux, Mac OS X), suitable for removable disks and optical media"
Comment 64 Kai Lüke 2017-07-09 20:09:03 UTC
Created attachment 355223 [details] [review]
Move volume and partition utility functions
Comment 65 Kai Lüke 2017-07-09 20:11:40 UTC
Created attachment 355224 [details] [review]
Change initial partition size units to be greater
Comment 66 Kai Lüke 2017-07-09 20:12:20 UTC
Created attachment 355225 [details] [review]
Update Partition and Format Dialog for Redesign
Comment 67 Kai Lüke 2017-07-09 20:13:02 UTC
Created attachment 355226 [details] [review]
Delete old files, rename partition/format dialog files
Comment 68 Kai Lüke 2017-07-09 20:13:36 UTC
Created attachment 355227 [details] [review]
Detect a recent enough UDisks version
Comment 69 Kai Lüke 2017-07-09 20:16:36 UTC
Created attachment 355228 [details] [review]
Redesign Format Dialog

Thanks for the hint, I've rephrased it a bit (and changed some spacing elsewhere).
Comment 70 Pali 2017-07-09 20:40:57 UTC
That is better now! "like FAT" was not a good description.
Comment 71 Ondrej Holy 2017-07-10 15:09:53 UTC
Review of attachment 355223 [details] [review]:

Looks good, however, each patch should be buildable separately, so it is necessary to rename the functions on all places, where they are used. Separate patches don't make much sense if you can't build them, sorry that I didn't say this explicitely before. This is applicable on other patches also...

It is good to add also some description for the commits why this is done (e.g. that it is needed for the following redesign patches). Applies on other patches also...
Comment 72 Ondrej Holy 2017-07-10 15:09:57 UTC
Review of attachment 355224 [details] [review]:

Looks good apart from the following:

::: src/disks/gducreatepartitiondialog.c
@@ +200,2 @@
 static void
+create_partition_populate (CreatePartitionPage data)

We want to be buildable, so CreatePartitionData *data.

@@ +205,2 @@
   /* figure out default unit */
+  if (data->max_size > unit_sizes[4] * 10ULL)

Would be really nice to introduce enums with unit names (e.g. unit_size[TB] and unit_num = TB), but this is irrelevant to this changes and will be hopefully fixed by libbytesize in the future...

@@ +207,2 @@
     {
+      /*         size > 10TB -> TB */

Maybe better to remove the leading spaces...

@@ +212,2 @@
     {
+      /* 1TB > size > 10GB -> GB */

10TB >...?

@@ +219,3 @@
       unit_num = 2;
     }
   else if (data->max_size > unit_sizes[1] * 100ULL)

...* 10ULL?
Comment 73 Ondrej Holy 2017-07-10 15:10:00 UTC
Review of attachment 355225 [details] [review]:

Attachments 255225, 355226 and 355228 needs to be merged probably to make them buildable...

::: src/disks/gducreatepartitiondialog.c
@@ +13,3 @@
 #include <math.h>
 
+#include "gducreatepartitionpage.h"

Should be part of the patch, same as other files needed for build...

@@ +18,3 @@
 
+/* TODO: Use libbytesize once it builds with jhbuild.
+ *       It comes as dependency for libblockdev and UDisks 2.7

Maybe better to file bug report about it instead of such comments...

@@ +36,3 @@
 };
 
+struct CreatePartitionPageData

As far as I can tell _CreatePartitionPage or CreatePartitionPagePrivate is used in such case...

@@ +43,2 @@
   guint64 max_size;
+  guint64 offset;

This change is not needed...

@@ +103,3 @@
+  g_object_set_data_full (G_OBJECT (data->widget),
+                          "complete", GUINT_TO_POINTER (can_proceed), NULL);
+  g_signal_emit_by_name (data->widget, "notify::name", NULL);

You should rather create this as a real GObject and create own signals and do not abuse notify::name... See has-info signal in gducreatefilesystemwidget.c for example. Same for other widgets...

@@ +115,2 @@
 static void
+create_partition_property_changed (GObject *object, GParamSpec *pspec, CreatePartitionPage user_data)

Is this really wanted? We should honor the style which is used in the rest of file/project, I don't have time to investigate it, just asking. This applies also for several following changes...

::: src/disks/gduformatvolumedialog.c
@@ +48,3 @@
+  CreateOtherPage other_page;       /* custom filesystem page */
+  CreatePasswordPage password_page;    /* set password page */
+  CreateConfirmPage confirm_page;     /* confirm format page */

Align those comments, or don't use multiple spaces...

@@ +110,3 @@
+  gtk_window_set_title (GTK_WINDOW (data->dialog), g_value_get_string (&title));
+  data->prev = NULL;
+  data->next = "confirm";

It would be nice to have macros for the page names...

@@ +113,3 @@
+
+  if (data->add_partition)
+        data->next = NULL;

Alignment...

@@ +126,3 @@
+        {
+          data->prev = "partition";
+        }

In one case you use {} for one-liners, one other you do not, please be consistent...

@@ +145,3 @@
+      if (gdu_create_filesystem_page_is_encrypted (data->filesystem_page))
+        data->prev = "format";
+      else if (gdu_create_filesystem_page_is_other(data->filesystem_page) &&

Missing space before "(".

@@ +153,3 @@
+      data->next = NULL;
+
+      if (gdu_create_filesystem_page_is_encrypted(data->filesystem_page) ||

dtto

@@ +154,3 @@
+
+      if (gdu_create_filesystem_page_is_encrypted(data->filesystem_page) ||
+          (gdu_create_filesystem_page_is_other(data->filesystem_page) &&

dtto

@@ +198,3 @@
 }
 
+

Unwanted white space...

@@ +324,2 @@
     {
+      if (data->prev != NULL)

When this may happen? We should avoid such situation if it can really happen...

@@ +335,1 @@
+  if (data->next != NULL)

dtto

@@ +410,3 @@
+    data->partition_page = gdu_create_partition_page_new (data->builder, data->client, data->table,
+                                                          add_partition_maxsize, add_partition_offset,
+                                                          G_CALLBACK (update_dialog), data);

I would rather see something like:
data->partition_page = gdu_create_partition_page_new (data->builder, data->client, data->table, add_partition_maxsize, add_partition_offset);
g_signal_connect (data->partition_page, "changed", G_CALLBACK (update_dialog), data);

Applies on the other widgets also...
Comment 74 Ondrej Holy 2017-07-10 15:10:01 UTC
Review of attachment 355226 [details] [review]:

The removal of the unwanted parts should be probably part of the following patch. The rename can be done separately, but it means that you initially include old files and rename them later, but also with rename of includes and relevant Makefile changes to be always buildable... but maybe it is needlessly complicated and would be better to merge them.
Comment 75 Ondrej Holy 2017-07-10 15:10:04 UTC
Review of attachment 355227 [details] [review]:

Good to make it separately, but ifdefs should be also part of this patch... so we can easily use git revert in future to remove this once it is obsolete.

::: configure.ac
@@ +52,3 @@
 GLIB2_REQUIRED=2.31.0
 UDISKS2_REQUIRED=2.1.1
+UDISKS2_EXTRA=2.7.2

I don't think we need this EXTRA constant...

@@ +81,3 @@
+dnl ****************************
+
+PKG_CHECK_MODULES([UDISKS2EXTRA], [udisks2 >= $UDISKS2_EXTRA],

I would hardcode the version here...

@@ +82,3 @@
+
+PKG_CHECK_MODULES([UDISKS2EXTRA], [udisks2 >= $UDISKS2_EXTRA],
+                  [AC_DEFINE([HAVE_UDISKS2_EXTRA], 1, [Define to 1 if recent UDisks2])],

and use HAVE_UDISKS2_2_7_2, or HAVE_UDISKS2_CAN_FORMAT, or so...
Comment 76 Ondrej Holy 2017-07-10 15:10:07 UTC
Review of attachment 355228 [details] [review]:

I've looked at it just quickly, but I wanted to send it today (I will try to look more deeply tomorrow)...

Looks overall (both code and UI) much cleaner now, thanks.

More padding is needed at the bottom of the dialog though.

Also it doesn't work well with the font-scale, but we can deal with it later.

::: src/disks/gduapplication.c
@@ +266,3 @@
       g_application_hold (_app);
+      gdu_create_format_show (app->client, NULL, object_to_select,
+                              FALSE, FALSE, 0, 0, (GCallback)g_application_release, _app);

Missing space after (GCallback). It is already existing "bug", but would be nice to fix here...

::: src/disks/gducreateconfirmpage.c
@@ +41,3 @@
+  /* Translators: In most cases this should not need translation unless the
+   *              separation character '—' is not appropriate. The strings come
+   *              from UDisks.

It is good to describe the context and what is first and what is second param, or add an example.

::: src/disks/gducreateconfirmpage.h
@@ +16,3 @@
+
+struct CreateConfirmPageData;
+typedef struct CreateConfirmPageData *CreateConfirmPage;

As far as I can tell it is not really common to lose the * in such case, I would rather see "typedef struct CreateConfirmPageData CreateConfirmPage;" at least. Would be best to create this as a proper object though, see:
https://developer.gnome.org/gobject/stable/howto-gobject.html

Applies on other pages also...

::: src/disks/gducreatefilesystempage.c
@@ +25,3 @@
+};
+
+const gchar*

Missing space before star

::: src/disks/gducreateotherpage.c
@@ +44,3 @@
+
+static const gchar *
+get_fs_description (const gchar *fs_type)

I think that the strings still need more love, but we can improve later...

::: src/disks/gdupasswordstrengthwidget.c
@@ +26,3 @@
   GtkBox parent;
 
+  GtkWidget *level_bar; /* TODO: fix GtkLevelBar warning in GTK or replace with GtkEntry progress fraction */

Again, maybe better to file a bug report about... the strength bar needs a redesign anyway (I wish to make generic gtkstrengthbar in order to unify it across the whole GNOME).
Comment 77 Chris Murphy 2017-07-11 06:21:55 UTC
Quick format (comment 8) and Erase (comment 33) switch, this really should be omitted for anything that's not definitely rotating media. As long as all stale fs signatures are removed first, just let it be. If supported by the device, it will be trimmed as that's the default mkfs option for ext4, XFS, Btrfs now. For FAT and NTFS it's reasonable to try blkdiscard first.

As for which Linux fs to support, toss a coin. ext4 has the plus of generally being familiar. XFS and Btrfs are incrementally more flash friendly respectively, both have large device size support, and Btrfs has explicit SSD optimizations, autodetected.

I think the VFS integrated encryption is too new, really only ext4 and F2FS support this for now. So I'd stick to combinations that are sanely supported.

As for the best cross platform format it comes down to FAT, NTFS, and UDF. FAT has widespread support, including TV firmware upgrades, and as media sticks. exFAT has licensing fees and isn't broadly supported on Linux, I would skip it. UDF is better and more cross platform than any of the options. Even NTFS, which macOS only has read-only support for.

I would not mess with any of Apple's stuff: HFS+, nor CoreStorage, nor APFS until Apple publishes details on this format or an open source driver (they never did for CoreStorage).
Comment 78 Kai Lüke 2017-07-11 07:06:02 UTC
Thanks for the feedback!
A simple wipe of known signatures is performed in UDisks through libblockdev and if that should handle SSDs in a better way then a report must go there. The full erase is (directly) done by UDisks, so a report about integrating blkdiscard needs to go there. Secure erase is not exposed in GNOME Disks and I'm not sure about the state in UDisks. Development goes on here:
https://github.com/storaged-project/

For the FS-level encryption I would also just wait a bit, also for non-LUKS cross platform stuff in UDisks.

Pushing UDF is something that was missed in the last decade (and now there is exFat) - Can mobile phones handle cards with UDF? Most cameras can't? It would be very nice if some of you can continue discussion whether we want it to be the default on removable drives (and have FAT/exFAT/NTFS as options) on this bug report here:
Bug 735525 - Replace NTFS preset with UDF
https://bugzilla.gnome.org/show_bug.cgi?id=735525
Comment 79 Pali 2017-07-11 07:29:38 UTC
(In reply to Kai Lüke from comment #78)
> Pushing UDF is something that was missed in the last decade (and now there
> is exFat) - Can mobile phones handle cards with UDF? Most cameras can't? It
> would be very nice if some of you can continue discussion whether we want it
> to be the default on removable drives (and have FAT/exFAT/NTFS as options)
> on this bug report here:
> Bug 735525 - Replace NTFS preset with UDF
> https://bugzilla.gnome.org/show_bug.cgi?id=735525

Depends on usage. exFAT is not a win, it falls into same category as NTFS.Undocumented, vendor locked, patent fee filesystem which is natively and correctly supported only on Windows + some licensed OEM vendors (in cameras, some phones). Not natively supported on Linux or Apple's systems. And due to those issue I was told that Linux (kernel) does not get exFAT support. What is bad is that people start using it even if they do not have to.

On the other hand UDF is natively supported on Linux, Windows and Mac OS X without need to install any 3rd applications. Which is nice. The worst part is that phones or camera do not support it.

Therefore for removable disks which are aimed to use in computers, UDF is the best option. And for SD cards, aimed for cameras/phones FAT32 is probably the best cross-platform solution.

I would not suggest to use NTFS or exFAT unless it is the only option.
Comment 80 Chris Murphy 2017-07-11 12:42:59 UTC
(In reply to Kai Lüke from comment #78)

> The full erase is (directly) done by UDisks, so a report about integrating
> blkdiscard needs to go there.

I think the full erase is an unnecessary and confusing feature to expose in the Disks UI. There's no benefit on any drive, but it's slightly bad for flash because it's unnecessary wear. I don't think the storaged folks have a reliable way of knowing rotating vs non-rotating devices, nothing distinguishes USB flash drives from hard drives, both show a 1 for /sys/block/<dev>/queue/rotational.

> Pushing UDF is something that was missed in the last decade (and now there
> is exFat) - Can mobile phones handle cards with UDF? Most cameras can't?

I've had numerous discussions with sd card manufacturers over the years, and a very large percentage of data loss inducing corruption they claimed was not the sd card's fault, but spurious behavior due to firmware bugs. And one of the triggers is a card formatted from a source other than that particular camera. So at least in professional circles, it's not trustworthy to format the card elsewhere and stick it into the camera, the camera's format option should be used to erase the card.

As for phones, sure it's either FAT or exFAT as the reliable cross platform choice. I don't know what level of support for UDF iOS and Android have.
Comment 81 Pali 2017-07-11 13:27:21 UTC
(In reply to Chris Murphy from comment #80)
> (In reply to Kai Lüke from comment #78)
> 
> > The full erase is (directly) done by UDisks, so a report about integrating
> > blkdiscard needs to go there.
> 
> I think the full erase is an unnecessary and confusing feature to expose in
> the Disks UI. There's no benefit on any drive, but it's slightly bad for
> flash because it's unnecessary wear.

Nope. If disk drive supports trim is it a big benefit to erase & mark all blocks in firmware as unused. Firmware then do not have to relocate and all blocks which are not used by newly formatted disk yet and can improve wear-leveling.

> > Pushing UDF is something that was missed in the last decade (and now there
> > is exFat) - Can mobile phones handle cards with UDF? Most cameras can't?
> 
> I've had numerous discussions with sd card manufacturers over the years, and
> a very large percentage of data loss inducing corruption they claimed was
> not the sd card's fault, but spurious behavior due to firmware bugs.

If I were SD card manufacturer I would say same thing despite fact if my cards caused data loss or not. For me in this case would be really bad to say that my HW is defected. So it is expected that they say it is not SD card fault.

Same question should be asked people on the other side -- manufacturers of cameras and those who maintain that firmware inside.

> And one
> of the triggers is a card formatted from a source other than that particular
> camera. So at least in professional circles, it's not trustworthy to format
> the card elsewhere and stick it into the camera, the camera's format option
> should be used to erase the card.

I do not know details about exFAT or NTFS. But for FAT32 this looks like a unbelievable thing. FAT32 has one exact structure and after formatting card it is clear how it should looks like. What could differ between FAT32 format applications are C-H-S and other values in boot sector (which make sense only for C-H-S hdd disks, not for SD cards) and what is stored in free/unused sectors (zeroed when doing full disk format or garbage from previous filesystem). For exFAT/NTFS it could be different...

> As for phones, sure it's either FAT or exFAT as the reliable cross platform
> choice. I don't know what level of support for UDF iOS and Android have.

Yes, FAT32 (VFAT with LFN) is widely supported. exFAT less. And UDF probably is not available on production phones.
Comment 82 Chris Murphy 2017-07-11 14:06:42 UTC
(In reply to Pali from comment #81)

> Nope. If disk drive supports trim is it a big benefit ...

I'm aware of that. But what does the "erase/quick format" switch do? In current Disks, it literally writes zeros, which is just not useful. It takes a long time, and serves no purpose, and increases wear.

The mkfs commands for ext4, XFS, and Btrfs already do trim on supporting devices. There's no good reason to inhibit that with a UI element.

There is maybe a valid optimization for storaged to try and issue blkdiscard before formatting with any other file system, since those file systems' mkfs does not issue trim. But yeah that's a storaged question.


> If I were SD card manufacturer I would say same thing despite fact if my
> cards caused data loss or not. For me in this case would be really bad to
> say that my HW is defected. So it is expected that they say it is not SD
> card fault.

But then it's also been proven to be the case in field use. Corruptions are very rare when using in-camera formatting, and avoiding in-camera file deletion. Users who don't do those things have significantly higher instances of data corruption. And I've seen completely reproducible data corruption by formatting a card in one camera, and moving it into another camera without reformatting again. The camera firmware in this regard has been pretty much shit, that's the diplomatic version.

> Same question should be asked people on the other side -- manufacturers of
> cameras and those who maintain that firmware inside.

They recommend formatting the card in the camera. If you ask if it should work or will work if you format it elsewhere, they do not answer the question, they repeat the recommendation. Format the card in camera.

> What could differ between FAT32 format
> applications are C-H-S and other values in boot sector (which make sense
> only for C-H-S hdd disks, not for SD cards) and what is stored in
> free/unused sectors (zeroed when doing full disk format or garbage from
> previous filesystem).

Only recently do cameras supporting SD Cards and exFAT issue blkdiscard at format time. Historically this was never done with early SD and CF cards, so there was a lot of stale data left on the card after formatting. Nevertheless that shouldn't matter either, but there have been a lot of bugs over the years with camera firmware as it relates to writing to flash media. And flash media for cameras/video has become very optimized for basically just doing writes, not overwrites or file deletions.

Further the camera's block device and file system error handling is minimal, the FAT driver contained in firmware is tiny.


> > As for phones, sure it's either FAT or exFAT as the reliable cross platform
> > choice. I don't know what level of support for UDF iOS and Android have.
> 
> Yes, FAT32 (VFAT with LFN) is widely supported. exFAT less. And UDF probably
> is not available on production phones.

And so that brings up another gotcha with cameras is they never consistently supported long file names, the vast majority were 8.3 only, and of course it's different with exFAT.

Anyway as I mention in 735525, if you want a single default, not knowing anything else, it would be FAT32 to support the widest array of devices and platforms. Once you hit the 2TB drive size, it can't be FAT32 or UDF.
Comment 83 Pali 2017-07-11 15:05:44 UTC
(In reply to Chris Murphy from comment #82)
> Anyway as I mention in 735525, if you want a single default, not knowing
> anything else, it would be FAT32 to support the widest array of devices and
> platforms.

Agree.
Comment 84 Pali 2017-07-12 07:41:01 UTC
(In reply to Chris Murphy from comment #82)
> I'm aware of that. But what does the "erase/quick format" switch do? In
> current Disks, it literally writes zeros, which is just not useful. It takes
> a long time, and serves no purpose, and increases wear.

Agree, write zeros without trimming them is in most cases useless. Probably it could make sense in some special circumstances, but it would be out-of-scope of normal usage...

> The mkfs commands for ext4, XFS, and Btrfs already do trim on supporting
> devices. There's no good reason to inhibit that with a UI element.
> 
> There is maybe a valid optimization for storaged to try and issue blkdiscard
> before formatting with any other file system, since those file systems' mkfs
> does not issue trim. But yeah that's a storaged question.

Lot of times I format disks without trimming them (calling mkfs.ext4 with option which disable it). Reason is simple: if I for any reason format data which I should not have, I would have some chance to restore something... After writing zeros to whole disk or trimming it, there is basically no chance anymore.

Probably if GUI already has a switch for "quick format" and "full erase & format", later one can be used for trim + normal format for non-rotational+trim-capable-disks. And choosing sane default (e.g. when trim is not supported, then default would be "quick" otherwise when trim is supported and is fast, then second "full erase & format" can be default). And when calling mke2fs (for ext2/3/4) in "quick format" then call it with option to not use díscard.

But this is just a suggestion which I think can be useful.

> > Same question should be asked people on the other side -- manufacturers of
> > cameras and those who maintain that firmware inside.
> 
> They recommend formatting the card in the camera. If you ask if it should
> work or will work if you format it elsewhere, they do not answer the
> question, they repeat the recommendation. Format the card in camera.

This would mean that we do not have to care so much about SD cards which are primary used in cameras. As requirements is to format them in device.
Comment 85 Kai Lüke 2017-07-24 15:03:08 UTC
Created attachment 356308 [details] [review]
Detect UDisks version 2.7.2 (from #676555)
Comment 86 Kai Lüke 2017-07-24 15:03:52 UTC
Created attachment 356309 [details] [review]
Move partition size helpers (#676555)
Comment 87 Kai Lüke 2017-07-24 15:04:24 UTC
Created attachment 356310 [details] [review]
Move volume and partition utility functions
Comment 88 Kai Lüke 2017-07-24 15:08:24 UTC
Created attachment 356311 [details] [review]
Redesign Format Dialog

Updated to master branch (meson), use propper GObjects and signals.
Comment 89 Ondrej Holy 2017-07-26 15:28:46 UTC
Review of attachment 356308 [details] [review]:

No need to duplicate those patches here, it is better to just mention it and set "Depends on:" failed...
Comment 90 Ondrej Holy 2017-07-26 15:28:50 UTC
Review of attachment 356310 [details] [review]:

Looks ok, thanks!
Comment 91 Ondrej Holy 2017-07-27 12:42:36 UTC
Review of attachment 356311 [details] [review]:

Thanks for the update. The code is more readable now, but I was not probably concrete enough when I was talking about GObjects in the previous review. I think we should create the pages as GtkWidget (use e.g. GtkBox as parent class), not just plain GObject. And use their classes directly in create-format.ui instead. So you can potentially use them somewhere else if needed. Similarly, as it is done in e.g. gducreatefilesystemwidget.c which you are removing now. What do you think?

I don't personally like vertical alignment of the header files, but we should do it as per guidelines. The major rule is to have three columns, but I think that almost any of your newly added header files is correctly aligned. Please try to fix them, ask on irc if needed.
https://developer.gnome.org/programming-guidelines/stable/c-coding-style.html.en#header-files

::: src/disks/gducreateformatdialog.c
@@ +359,3 @@
+  if (data->add_partition)
+    {
+    size = gdu_create_partition_page_get_size (data->partition_page);

wrongly aligned

@@ +438,3 @@
+  data->confirm_page = gdu_create_confirm_page_new (data->builder, data->client, data->object, data->block);
+
+  data->back = GTK_BUTTON (gtk_dialog_add_button (data->dialog,_("_Cancel"), GTK_RESPONSE_CANCEL));

Missing space before _("_Cancel"), same on the following lines...

::: src/disks/gducreateotherpage.c
@@ +117,3 @@
+}
+
+#ifdef HAVE_UDISKS2_EXTRA

HAVE_UDISKS2_7_2?

::: src/disks/ui/create-format.ui
@@ +170,3 @@
+                            <property name="use_underline">True</property>
+                            <style>
+                              <class name="explanation-label"/>

I suppose this should not be used there... it should be a regular label, shouldn't be?

@@ +222,3 @@
+                <child>
+                  <object class="GtkCheckButton" id="dos-extended-checkbutton">
+                    <property name="label" translatable="yes">Extended Partition to contain Logical Partitions</property>

The button looks a bit misaligned in the dialog...

@@ +729,3 @@
+                    <property name="halign">end</property>
+                    <property name="label" translatable="yes">Device</property>
+                    <property name="mnemonic_widget">internal-radiobutton</property>

This is not true... same for the following.

@@ +744,3 @@
+                    <property name="can_focus">False</property>
+                    <property name="halign">start</property>
+                    <property name="label" translatable="yes">8 GB Drive - General USB Flash Drive</property>

This should not be prefiled... same for the following.
Comment 92 Mitchell Horne 2017-08-08 02:32:10 UTC
+1 for making the format pages as composite widgets rather than GObjects. Additionally it might be worthwhile to turn the format dialog itself into a widget as the GduCreateFilesystemWidget was before. I noticed a strange behavior while testing that dragging the format dialog will move the entire application instead of just the dialog.
Comment 93 Kai Lüke 2017-08-14 12:04:36 UTC
Created attachment 357548 [details] [review]
Redesign Format Dialog

Finally I hope to have it ready with a widget class for each page.
Comment 94 Ondrej Holy 2017-08-25 11:33:40 UTC
Review of attachment 357548 [details] [review]:

Awesome, I think we are done here, great work! Just some details...

It seems it is not possible to format the existing extended partition, we should not allow formatting of it, or fix the workflow. This is probably already existing bug, so feel free to file another bug report about.

::: src/disks/gducreateconfirmpage.h
@@ +16,3 @@
+
+#define GDU_TYPE_CREATE_CONFIRM_PAGE gdu_create_confirm_page_get_type ()
+G_DECLARE_FINAL_TYPE (GduCreateConfirmPage, gdu_create_confirm_page, GDU, CREATE_CONFIRM_PAGE, GtkGrid)

nitpick: Wouldn't be GduConfirmPage instead of GduCreateConfirmPage enough? Applies on others...

@@ +22,3 @@
+                                                                 UDisksBlock  *block);
+
+void                  gdu_create_confirm_page_fill_confirmation (GduCreateConfirmPage *page);

Do we need this function public available? Isn't enough to call this from gdu_create_confirm_page_new?

::: src/disks/gducreateformatdialog.c
@@ +54,3 @@
+  GduCreateOtherPage *other_page;            /* custom filesystem page */
+  GduCreatePasswordPage *password_page;      /* set password page */
+  GduCreateConfirmPage *confirm_page;        /* confirm format page */

nitpick: Not sure we need those since we can obtain them from stack using the macros...

::: src/disks/gducreatepartitionpage.c
@@ +158,3 @@
+            show_dos_error = TRUE;
+          else if (num_primary == 3)
+            show_dos_warning = TRUE;

Warning is shown that it is last primary partition, which can be created, but extended partition seems to be created automatically, so the warning has to be changed or the automagic should be avoided.

Btw the warning/error should not probably have the tiny gray border and the error should have red/orange background, but this is probably already existing bug, so please file bug report about.

::: src/disks/gducreatepasswordpage.h
@@ +19,3 @@
+G_DECLARE_FINAL_TYPE (GduCreatePasswordPage, gdu_create_password_page, GDU, CREATE_PASSWORD_PAGE, GtkGrid)
+
+GduCreatePasswordPage *gdu_create_password_page_new          (void);

Please file a bug report that the password dialog needs more love to match the control-center password dialog.

::: src/disks/ui/create-filesystem-page.ui
@@ +208,3 @@
+    <child>
+      <placeholder/>
+    </child>

nitpick: I see that glade generates those placeholders, but do we have to commit them? Is glade able to open the files without them? Applies on other palces also...
Comment 95 Kai Lüke 2017-09-04 01:47:53 UTC
Good, thanks.
I thought about removing the 'Create' part in the naming but there could be other dialogs with confirmation pages. My intention with gdu_create_confirm_page_fill_confirmation was to gather the information as late as possible.
What do you mean with automatic creation of extended partitions? I don't see it and think this logic is still correct.

Format option of an extended partition:
https://bugzilla.gnome.org/show_bug.cgi?id=787231

Password dialog appearence:
https://bugzilla.gnome.org/show_bug.cgi?id=787232

I will correct the warning box margin when committing it after the freeze (the dialog-vbox has border width 2 which should be 0). Sometimes placeholders can be removed, sometimes not, I'll check then.
Comment 96 Ondrej Holy 2017-09-04 09:28:24 UTC
Ok, thank!

(In reply to Kai Lüke from comment #95)
> What do you mean with automatic creation of extended partitions? I don't see
> it and think this logic is still correct.

1/ format disk (default settings)
2/ add first partition and let enough space for others (default settings)
3/ add second, third partition and forth partition same way... 

The result is that forth partition is inside extended partition magically, but I wanted to have 4 primary partitions...
Comment 97 Kai Lüke 2017-09-18 12:20:41 UTC
Comment on attachment 357548 [details] [review]
Redesign Format Dialog

Placeholders are directly added in Glade when opened and saved, removing them now would mean to remove them at every edit manually again, so I leave them in.

(Added <property name="border_width">0</property> for internal-child in create-format.ui)