Yeah, we should do that again (we used to before before the 3.4 rewrite). One annoyance is that there's no gtk_entry_set_max_length_bytes() method - filesystems only allow a certain number of bytes for the label and are oblivious to the encoding (in our case we just assume UTF-8) - but we can work around that.

Patch is pending.

Created attachment 349163 [details] [review]
Check maximal length of FAT label, add icon hint for wrong fields

Review of attachment 349163 [details] [review]:

Thanks for the patch, it works, but I think it should be done in a different way. I am convinced that we should provide same behavior for bytes as it is for characters. So, I see 3 possibilities here:
- provide patch adding gtk_entry_set_max_bytes for GTK+ if GTK+ developers want it (worth to discuss on #gtk+ before coding, e.g. with mclasen),
- create GduEntry based on GtkEntry with this functionality (this may be used anyway before GTK+ is released...),
- or handle the input text somehow directly in the gducreatefilesystemwidget.c code (maybe preedit-changed signal might be useful, worth to take a look on gtk_entry_set_max_length implementation)

Also this needs to be fixed in src/disks/gdufilesystemdialog.c, that's the reason why it would be nice to have _set_max_bytes functionality.

Please add bugzilla links into the commit messages. It is kinda useful when you are debugging something and needs to find why some changes were done. See:
https://wiki.gnome.org/Newcomers/CodeContributionWorkflow

::: src/disks/gducreatefilesystemwidget.c
@@ +196,3 @@
+      if (strlen (name) > 11)
+        {
+          gtk_entry_set_icon_from_icon_name (GTK_ENTRY (widget->name_entry), GTK_ENTRY_ICON_SECONDARY, "gtk-dialog-warning");

You should also set tooltip text to be obvious what is wrong...

Created attachment 349191 [details] [review]
Restrict maximal label length without gtk_entry_set_max_length

Thanks for the review, now the behavior should be consistent regardless of non-ascii input.

Review of attachment 349191 [details] [review]:

Thanks. Overall looks good, but have some comments (nitpick)...

Please create separate commits for each change if possible, so one for the length and one for the icon.

The commit messages may have "tag:" at the beginning, but it is usually file name, dir name, or another logical part. It seems that gnome-disks doesn't use this convenience a lot, so I would remove the "Dialog behavior:" prefix from the commit message at all...

::: src/disks/gducreatefilesystemwidget.c
@@ +218,3 @@
+          else
+            {
+              gtk_entry_set_icon_from_icon_name (GTK_ENTRY (widget->confirm_passphrase_entry), GTK_ENTRY_ICON_SECONDARY, "gtk-dialog-warning");

The line is too long, please wrap the arguments.

You should use "dialog-warning-symbolic", "gtk-dialog-warning" is deprecated.

Maybe it would be nice to add a tooltip to be obvious why the icon is shown (see gtk_entry_set_icon_tooltip_text).

@@ +310,3 @@
       g_object_notify (G_OBJECT (widget), "fstype");
     }
+  if (g_strcmp0 (widget->name, name) != 0 || max_utf8_length != max_length)

Why is this needed?

@@ +359,3 @@
+    }
+
+  if (max_length > 0 && full_length > max_length)

This works nicely for one char changes, but it is problem for example when you paste some text from the clipboard. The pasted text should be inserted, but trimmed.
(same for gdufilesystemdialog.c)

::: src/disks/gdufilesystemdialog.c
@@ +24,3 @@
   GtkWidget *dialog;
+  gchar     *orig_label;
+  guint      label_max_length;

Please do not align the variables here. The most important rule is imitating surrounding (project-wide) code ;-)
(see https://developer.gnome.org/programming-guidelines/stable/c-coding-style.html.en if you have some doubts about formating)

@@ +104,3 @@
   ChangeFilesystemLabelData data;
   const gchar *label_to_set;
+  gchar       *fstype;

Please do not align the variable here.

::: src/libgdu/gduutils.c
@@ +784,3 @@
+gdu_utils_get_max_label_length (const gchar *fstype)
+{
+  guint max_length = 0;

Nitpick: It would be nice to add here empty line as a separator between the declarations and the code.

@@ +789,3 @@
+      max_length = 15;
+    }
+  if (g_strcmp0 (fstype, "vfat") == 0)

Please use "else if" here in order to avoid further processing.

@@ +792,3 @@
+    {
+      max_length = 11;
+    }

Nitpick: It would be nice to add here empty line as a separator between the code and the return statement.

Created attachment 349265 [details] [review]
Restrict maximal label length if known

This here is shorter now and allows to paste long text because it trims after the change-signal. It's still imperfect because it behaves not identical to gtk_entry_set_max_length (like additional turning on overwrite at the keyboard).

Created attachment 349266 [details] [review]
Hint for passphrase confirmation field

I have split out the passphrase fix into this here.

There is already a tooltip text (which is quite ok) and I thought it did not allow another one, but even without still gtk_entry_set_icon_tooltip_text does not have a result. I think it's a bug in GTK.

(In reply to Kai Lüke from comment #8)
> Created attachment 349266 [details] [review] [review]
> Hint for passphrase confirmation field
> 
> I have split out the passphrase fix into this here.
> 
> There is already a tooltip text (which is quite ok) and I thought it did not
> allow another one, but even without still gtk_entry_set_icon_tooltip_text

I think it should show the icon tooltip even if there is entry tooltip...

> does not have a result. I think it's a bug in GTK.

Hmm, please try to find a relevant GTK+ bug report. Please file a new bug report if you can't find anything relevant...

Review of attachment 349266 [details] [review]:

Thanks, it's almost ready to push...

::: src/disks/gducreatefilesystemwidget.c
@@ +233,3 @@
+                                                 "dialog-warning-symbolic");
+              gtk_entry_set_icon_tooltip_text (GTK_ENTRY (widget->confirm_passphrase_entry),
+                                   GTK_ENTRY_ICON_SECONDARY,

Please fix the alignment of those lines.

@@ +234,3 @@
+              gtk_entry_set_icon_tooltip_text (GTK_ENTRY (widget->confirm_passphrase_entry),
+                                   GTK_ENTRY_ICON_SECONDARY,
+                                   _("Check for typos"));

I would use something more neutral, like "The passwords do not match", which is already used in gnome-control-center when changing password.

Review of attachment 349265 [details] [review]:

Thanks! I think it works nicely now, but there are still some issues:

::: src/disks/gducreatefilesystemwidget.c
@@ +275,3 @@
+          max_utf8_length--;
+        }
+    }

I would rather see something like the following in order to avoid duplicating code: 
name = _gtk_entry_buffer_truncate_bytes (gtk_entry_get_buffer (GTK_ENTRY (widget->name_entry)));

@@ +334,3 @@
       g_object_notify (G_OBJECT (widget), "name");
     }
+  else if (max_utf8_length != max_length) /* name field was trimmed */

Isn't the previous if-statement enough? I don't see any case when this is needed...

@@ -346,3 @@
 }
 
-

Please do not make whitespace changes on places, which are not directly next to modified code...

::: src/disks/gdufilesystemdialog.c
@@ +32,3 @@
 {
   ChangeFilesystemLabelData *data = user_data;
+  const gchar *name = (gtk_entry_get_text (GTK_ENTRY (editable)));

There is no need for the surrounding parentheses...

Created attachment 349307 [details] [review]
Hint for passphrase confirmation field

Thanks, I took your wording :)

Created attachment 349308 [details] [review]
Restrict maximal label length if known

The additional notify check was needed because the local name variable and the entry text were the same but it still needed to be copied.
Anyway with the new _gtk_entry_buffer_truncate_bytes this changed differently since the local name variable is invalidated after the trim.
Hope it's ok now :)

P.S.: I filed a bug about the icon tooltip
https://bugzilla.gnome.org/show_bug.cgi?id=780938

Review of attachment 349307 [details] [review]:

Thanks, looks good to me now. The same should be done also for gduchangepassphrasedialog.c, sorry I didn't realize it earlier...

Thanks for that bug report.

Review of attachment 349308 [details] [review]:

Thanks. Sorry, I still see some issues...

::: src/disks/gducreatefilesystemwidget.c
@@ +316,3 @@
       g_object_notify (G_OBJECT (widget), "fstype");
     }
+  if (trimmed)

There is no need for separate if-statement, you can merge it with the next one...

@@ +322,3 @@
+      g_object_notify (G_OBJECT (widget), "name");
+    }
+  else if (g_strcmp0 (widget->name, name) != 0)

...but I am still convinced that this case is always true if trimmed is true.

::: src/libgdu/gduutils.c
@@ +801,3 @@
+
+gboolean
+_gtk_entry_buffer_truncate_bytes (GtkEntry* gtk_entry,

The space should be before the star, not after it (sorry, but we have to be consistent):
GtkEntry *gtk_entry

Please change the name to _gtk_entry_truncate_bytes, or use GtkEntryBuffer as the first parameter at the best (I think you don't need the GtkEntry, you can use gtk_entry_buffer_get_bytes instead of strlen)...

@@ +802,3 @@
+gboolean
+_gtk_entry_buffer_truncate_bytes (GtkEntry* gtk_entry,
+                                  guint     max_length)

nitpick: I would rather use max_bytes here :-)

::: src/libgdu/gduutils.h
@@ +65,3 @@
+guint gdu_utils_get_max_label_length (const gchar *fstype);
+
+gboolean _gtk_entry_buffer_truncate_bytes (GtkEntry* gtk_entry,

dtto

Created attachment 349424 [details] [review]
Hints for passphrase fields

Of course, I totally forgot about that dialog!

This iteration was also nice because I spotted some other things I missed (like calling it password instead of passphrase as already mentioned in the dialog).

Created attachment 349425 [details] [review]
Restrict maximal label length if known

Yes, sure, I've overlooked these things again :/ and by taking gtk_entry_buffer it even saves one variable :D

Sorry, I made it a bit confusing with the changes, you were right, in the first version it was not needed, I thought it would be the other variable being compared. But in the second it was needed in fact because I spared updating the local name variable. So now I hope to have made it more clear finally!

Thanks for reviewing again with care and all the tips.
Kai

Review of attachment 349424 [details] [review]:

Thanks for the updated patch, however, there are still some issues (because of the new hint, maybe better to make separate patch next time :-)...

Just a side note, please see Users panel in Settings and try to change your password. There are check marks instead of the warning symbols (positive feedback instead of negative). The icons aren't set immediately, but on activate, focus out, or when user stop writing (it is a bit confusing that the warning is shown when writing the confirm passphrase in your case). There are also dynamic hints explaining what needs to be done. This was implemented as per mockup from Allan Day. This is probably a good example, how it should be done in the best case... on the other hand, it requires much more work, not just one patch with few lines... I think your patch is good, wanted and much better than the current state, because it adds missing feedback, however, we should have in mind, that it can be done better, and think about such improvements for the future...

Would be also nice to print a better error message if the current passphrase is wrong, but that's also another issue...

::: src/disks/gduchangepassphrasedialog.c
@@ +111,3 @@
                                              passphrase);
 
+  if (strlen (existing_passphrase) > 0 && strlen (passphrase) > 0)

The confirm_passphrase should be checked regardless of the existing passphrase, because users may skip existing passphrase and start filling new passphrase...

@@ +126,3 @@
+          can_proceed = TRUE;
+        }
+      else

The following icon should be shown regardless of the confirm_passphrase, so "else" is not wanted here... so you have to reorder the conditions, or separate the icons logic from can_proceed logic...

Review of attachment 349425 [details] [review]:

Thanks. It looks good, just please make a separate patch for the icons (one patch = one feature). Apart from that, I pointed out some nitpicks (not mistakes, not necessary to fix them), how it can be done better, at least from my point of view (sorry I haven't told you this earlier)...

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

nitpick: You can alway set the name here and remove the initial definition...

::: src/libgdu/gduutils.c
@@ +784,3 @@
+gdu_utils_get_max_label_length (const gchar *fstype)
+{
+  guint max_length = 0;

nitpick: Maybe would be better to use G_MAXUINT... and consequently avoid max_bytes > 0 checks...

@@ +806,3 @@
+  guint max_utf8_length = max_bytes;
+
+  if (max_bytes > 0 && gtk_entry_buffer_get_bytes (gtk_entry_buffer) > max_bytes)

nitpick: Maybe would be better to remove the second part of this check, because it is immediately checked again in the loop...

Created attachment 349559 [details] [review]
Hints for passphrase fields

Hi,
yes, I think that would be an improvement, I like the checkmarks for existing and confirmed password and the behavior thanks to timeout! So this would need live testing of the entered password.
I hope this can be done in a new bug later.

From my point of view the approach with the checkmarks only works well if you expect them - in the control center it is a bit confusing because at first I don't know that there should be any checkmarks for my new password (there is already feedback below for the strength) and wonder why I can't set the password now in case it's not strong enough. So if it's mandatory then a visual indication of the field which needs work is helpful (but of course should not be too negative, maybe colors or pulsation help).

Created attachment 349560 [details] [review]
Restrict maximal label length if known

Nice, I changed it as suggested, the 0 as value for unlimited was more a remnant from the first version ;)

Created attachment 349561 [details] [review]
Change label dialog hint

Separate patch for the label changing hint.

Review of attachment 349559 [details] [review]:

Looks good to me, thanks!

Review of attachment 349560 [details] [review]:

Looks good to me, thanks!

Review of attachment 349561 [details] [review]:

Looks good to me, thanks!

(In reply to Kai Lüke from comment #20)
> Created attachment 349559 [details] [review] [review]
> Hints for passphrase fields
> 
> Hi,
> yes, I think that would be an improvement, I like the checkmarks for
> existing and confirmed password and the behavior thanks to timeout! So this
> would need live testing of the entered password.
> I hope this can be done in a new bug later.

Sure, it was just a note.
 
> From my point of view the approach with the checkmarks only works well if
> you expect them - in the control center it is a bit confusing because at
> first I don't know that there should be any checkmarks for my new password
> (there is already feedback below for the strength) and wonder why I can't
> set the password now in case it's not strong enough. So if it's mandatory
> then a visual indication of the field which needs work is helpful (but of
> course should not be too negative, maybe colors or pulsation help).

You are right, there is already Bug 780002 because of that...

Attachment 349559 [details] pushed as 847d6d2 - Hints for passphrase fields
Attachment 349560 [details] pushed as 39538a5 - Restrict maximal label length if known
Attachment 349561 [details] pushed as 42ca8a1 - Change label dialog hint