GNOME Bugzilla – Bug 747381
Ask for the directory name before creating it
Last modified: 2015-04-20 13:45:12 UTC
The new mockups include a new "New Folder" dialog. It should be implemented in order to achieve the (so needed) GNOME modernization.
Created attachment 301034 [details] [review] view: show "New Folder" dialog This commit introduces the "New Folder" dialog, which asks the folder name before actually creating it. With the introduced changes, the folder is created with the given name instead of creating it first with the generic "Unamed folder" and then renaming it. This dialog is part of the ongoing effort to modernize Nautilus to better fit GNOME standards, and the latest mockups can be found at [1]. [1] https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/nautilus/nautilus-next/new-folder.png
Review of attachment 301034 [details] [review]: I think is better to use GtkBuilder files whenever possible, and this is a clear one even if it is a simple dialog... =) Like action_show_move_to_trash_shortcut_changed_dialog does more or less. On the other hand, here we can be smarter, since we can now show a hint that the name is invalid and disable the OK button. The valid names are controlled in nautilus_file_rename in nautilus-file.c . You could extract that part of the code into a function like "validate_file_name" or so. Then call it from the validate_entry, and if not correct make the hint with a red entry and a dimmed text below the entry or something like that. That will look awesome! =) You can ask designers for this, and if you don't feel like doing it, you can tell me and skip it. But is a good moment for implement this. ::: libnautilus-private/nautilus-file-undo-operations.c @@ +730,3 @@ parent = g_file_get_parent (self->priv->target_file); parent_uri = g_file_get_uri (parent); + nautilus_file_operations_new_folder (NULL, NULL, parent_uri, NULL, NULL? then the folder name is "Untitled folder"... You will need a way here to use the old name. ::: src/nautilus-view.c @@ +1599,3 @@ + const gchar *text; + + g_assert (object && GTK_IS_ENTRY (object)); we usually use just g_assert(GTK_IS_ENTRY (object)), but yours looks better to me. Good idea @@ +1606,3 @@ + gtk_dialog_set_response_sensitive (GTK_DIALOG (user_data), + GTK_RESPONSE_OK, + g_assert (object && GTK_IS_ENTRY (object)); I guess g_utf8_strlen (text, 1) should be enough no? @@ +1620,3 @@ + text = gtk_entry_get_text (GTK_ENTRY (entry)); + + g_utf8_strlen (text, -1) > 0); If the button is inactive in validate_entry, this check shouldn't be necessary. If you want to keep it, I would put here a g_warning in an else statement, since seeing the button be able to be activated but doing nothing doesn't looks good in any case. @@ +1640,1 @@ + // Dialog label comments use the c89 style, /* */ @@ +1698,3 @@ + + data = new_folder_data_new (directory_view, with_selection); + gtk_container_add (GTK_CONTAINER (box), entry); From docs: "a pointer to the contents of the widget as a string. This string points to internally allocated storage in the widget and must not be freed, modified or stored." Although we don't destroy it until we set the name in nautilus_file_operations_new_folder, I think is better to g_strdup always in these cases. @@ +1714,3 @@ + parent_uri, + name, + G_CALLBACK (nautilus_view_add_file_dialog_validate_entry), You don't select the file and scroll to it, as it was doing before.
This should fix it Attachment 301034 [details] pushed as 6f4fc64 - view: show "New Folder" dialog
whops, obviously my intention was not to push it
Review of attachment 301034 [details] [review]: chaging status of patch
Created attachment 301922 [details] [review] view: show "New Folder" dialog With the introduced changes, the folder is created with the given name instead of creating it first with the generic "Unamed folder" and then renaming it. The dialog has a "delayed message" logic, where typos are immediatly recognized, but coincident folder names appear ~2s later.
Created attachment 301928 [details] [review] view: show "New Folder" dialog Fixed a memory leak and fixed a typo.
Review of attachment 301928 [details] [review]: Looks almost finished. Just some nitpicks. ::: src/nautilus-new-folder-dialog.ui @@ +13,3 @@ + <object class="GtkBox" id="vbox"> + <property name="orientation">vertical</property> + <property name="border_width">24</property> little less padding in top and bottom, as per the mockups. maybe 10? I normally measure them with gimp from the mockups. @@ +21,3 @@ + <property name="xalign">0</property> + <style> + <class name="dim-label"/> its not dimmed in the mockups @@ +44,3 @@ + </child> + <child> + <object class="GtkRevealer" id="revealer"> looks nice, but actually we don't want to resize the window, as per the mockups. Just make the label empty. @@ +87,3 @@ + </child> + <action-widgets> + <action-widget response="apply" default="true">create_button</action-widget> response="ok" ::: src/nautilus-view.c @@ +1634,3 @@ + +static gboolean +has_folder_cb (NewFolderDialogData *data) not needed to use _cb (in this class is not used much, and your other functions don't use it like activate, etc.) @@ +1647,3 @@ + +static void +nautilus_view_add_file_dialog_validate_content (GObject *object, validate_name looks more specific here. @@ +1661,3 @@ + gboolean has_folder; + + g_assert (object && GTK_IS_ENTRY (object)); *_IS_* (NULL) works fine. Not need to check for null. @@ +1662,3 @@ + + g_assert (object && GTK_IS_ENTRY (object)); + g_assert (user_data && data->view && data->dialog && data->revealer); I would say one per line, and you can use *_IS_*. @@ +1694,3 @@ + gtk_label_set_label (GTK_LABEL (label), _("A folder with that name already exists.")); + } else { + view->details->new_folder_name_timeout_id = g_timeout_add (1750, use a constant. And I would say 1 is ok... almost two seconds feel too long (although I know the design says 2 or 3) @@ +1710,3 @@ + + gtk_dialog_set_response_sensitive (GTK_DIALOG (dialog), + GTK_RESPONSE_APPLY, nitpicking, but I think here looks better GTK_RESPONSE_OK semantically (you are not aplying a prefenrece or something and letting the window open, but instead you are done and the window is closed.) @@ +1733,3 @@ + * it all again. Checking if the "Create" button is sensitive + * is enought and sufficiently fast to determine if the dialog + * can create this folder. redo/check enought/enough And I would remove the last part after "is enough" @@ +1739,3 @@ + GTK_RESPONSE_APPLY); + } else { + g_warning ("Invalid folder name."); I would say, set the error label immediately if this happens instead of a warning where the user can't see it. @@ +1772,3 @@ + "activated_cb", + G_CALLBACK (nautilus_view_add_file_dialog_entry_activate), + NULL); Just curious, is this necesary and not enough with gtk_builder_connect_signals and the application symbol table? @@ +1776,3 @@ + gtk_builder_connect_signals (builder, dialog_data); + + /* run the dialog */ comment not needed @@ +1787,3 @@ + /* To avoid any inconsistencies with the + * typed folder name, duplicate it. + */ I don't think the comment is necessary. Basically if you don't own something and you need to operate with it with the risk of it being freed, duplicate it
Created attachment 301983 [details] [review] view: show "New Folder" dialog Proposed changes applied. One thing that is bothering me is that this patch does one I/O block per check, and a check each time the folder name changes. I couldn't figure out how to fix it without adding a major maintance burden.
Created attachment 301987 [details] [review] view: show "New Folder" dialog As Carlos suggested, use the NautilusDirectory to check for file existance. This saves us some (maybe several) disk accesses.
Created attachment 301988 [details] [review] view: show "New Folder" dialog Remove an spurious warning
Created attachment 301990 [details] [review] view: show "New Folder" dialog Shows the label when the user tries to add a folder with collidant name.
Review of attachment 301990 [details] [review]: Make the dialog a little wider as per the mockups (and also it doesn't get resized when the warning is displayed) and looks good to commit.
Created attachment 301993 [details] [review] view: show "New Folder" dialog Make the dialog at least 50 chars wide.
Created attachment 301994 [details] [review] view: show "New Folder" dialog As Carlos requested, use GtkWidget::width-request property.
Created attachment 301995 [details] [review] view: show "New Folder" dialog Reduce timeout for error label. 1s is still too slow.
Review of attachment 301995 [details] [review]: LGTM thanks!
Attachment 301995 [details] pushed as 6ae3384 - view: show "New Folder" dialog