GNOME Bugzilla – Bug 769336
files-view: move file name widget logic to separate controllers
Last modified: 2016-08-19 12:53:01 UTC
See patch.
Created attachment 332407 [details] [review] files-view: move file name widget logic to separate controllers The rename file popover and the new folder dialog share common logic for validating file names entered by the user. The control logic was implemented with a simple structure in files-view. Besides common logic, the structure also held parameters specific to only one of the operations. Another problem is that the current implementation does not allow flexibility in obtaining the file name from the widgets and displaying error messages. In order to fix this, reimplement the structure as an abstract class and create two subclasses for the "Rename" and "New Folder" widgets.
Created attachment 332442 [details] [review] files-view: move file name widget logic to separate controllers The rename file popover and the new folder dialog share common logic for validating file names entered by the user. The control logic was implemented with a simple structure in files-view. Besides common logic, the structure also held parameters specific to only one of the operations. Another problem is that the current implementation does not allow flexibility in obtaining the file name from the widgets and displaying error messages. In order to fix this, reimplement the structure as an abstract class and create two subclasses for the "Rename" and "New Folder" widgets.
Created attachment 332443 [details] [review] files-view: correct method name The method 'compute_rename_popover_relative_to' has a misleading name because it actually computes the item that the popover will point to.
Review of attachment 332443 [details] [review]: well, gtk+ uses the method gtk_popover_set_relative_to precisely for that, so the name sounds appropriate. Why do you think is not appropriate?
Review of attachment 332443 [details] [review]: As Razvan pointed on IRC, we actually use pointing_to.
Review of attachment 332442 [details] [review]: First review, thanks for this work! ::: src/nautilus-file-name-widget-controller.c @@ +282,3 @@ + "changed", + (GCallback)file_name_widget_controller_on_changed, + controller); you have to disconnect from the previous entry no? @@ +290,3 @@ + "clicked", + (GCallback)file_name_widget_controller_on_activate, + controller); ditto @@ +293,3 @@ + break; + case PROP_CONTAINING_DIRECTORY: + priv->containing_directory = NAUTILUS_DIRECTORY (g_value_get_object (value)); no ref here? what happens if the directory goes unrefed somewhere else? @@ +317,3 @@ + file_name_widget_entry_on_directory_info_ready, + self); + g_clear_object (&priv->containing_directory); unrefing here but you don't ref on set_property? ::: src/nautilus-file-name-widget-controller.h @@ +15,3 @@ + GObjectClass parent_class; + + gchar * (*get_new_name) (NautilusFileNameWidgetController *controller); could you align these signals? @@ +26,3 @@ + void (*name_accepted) (NautilusFileNameWidgetController *controller); + + gpointer padding[10]; Nautilus doesn't have public API, we don't need this. ::: src/nautilus-files-view.c @@ +1780,3 @@ + g_signal_connect (view->details->rename_file_controller, + "name-accepted", + (GCallback)rename_file_popover_controller_name_accepted, on_name_accepted. @@ +1784,3 @@ + g_signal_connect (view->details->rename_file_controller, + "cancelled", + (GCallback)rename_file_popover_controller_cancelled, dito @@ +1860,3 @@ + g_signal_connect (view->details->new_folder_controller, + "name-accepted", + (GCallback)new_folder_dialog_controller_name_accepted, *_on_* @@ +1864,3 @@ + g_signal_connect (view->details->new_folder_controller, + "cancelled", + (GCallback)new_folder_dialog_controller_cancelled, dito ::: src/nautilus-new-folder-dialog-controller.c @@ +19,3 @@ + +static void +nautilus_new_folder_dialog_controller_name_accepted (NautilusFileNameWidgetController *controller) _on_ @@ +25,3 @@ + self = NAUTILUS_NEW_FOLDER_DIALOG_CONTROLLER (controller); + + if (self->response_handler_id) { can this be 0? @@ +34,3 @@ +nautilus_new_folder_dialog_controller_name_is_valid (NautilusFileNameWidgetController *self, + gchar *name, + gchar **error_message) aligment ::: src/nautilus-rename-file-popover-controller.c @@ +109,3 @@ + GtkWidget *name_label; + NautilusDirectory *containing_directory; + gint start_offset, end_offset; one per line @@ +159,3 @@ + gtk_popover_set_default_widget (GTK_POPOVER (rename_file_popover), activate_button); + gtk_popover_set_pointing_to (GTK_POPOVER (rename_file_popover), pointing_to); + gtk_popover_set_relative_to (GTK_POPOVER (rename_file_popover), relative_to); what if both are set? @@ +162,3 @@ + + gtk_widget_show (rename_file_popover); + gtk_widget_grab_focus (name_entry); is this needed? Why doesn't take it automatically? @@ +180,3 @@ +nautilus_rename_file_popover_controller_get_target_file (NautilusRenameFilePopoverController *self) +{ + g_assert (NAUTILUS_IS_RENAME_FILE_POPOVER_CONTROLLER (self)); crashing the application on pourpose if the rename cannot be done sounds like too much :) use g_return_val_if_fail.
(In reply to Carlos Soriano from comment #6) > Review of attachment 332442 [details] [review] [review]: > > First review, thanks for this work! > > ::: src/nautilus-file-name-widget-controller.c > @@ +282,3 @@ > + "changed", > + > (GCallback)file_name_widget_controller_on_changed, > + controller); > > you have to disconnect from the previous entry no? The property can only be set during construction, so there will never be a previous entry, right? > > @@ +290,3 @@ > + "clicked", > + > (GCallback)file_name_widget_controller_on_activate, > + controller); > > ditto > > @@ +293,3 @@ > + break; > + case PROP_CONTAINING_DIRECTORY: > + priv->containing_directory = NAUTILUS_DIRECTORY > (g_value_get_object (value)); > > no ref here? what happens if the directory goes unrefed somewhere else? Well, in my use cases, the directory parameter was sent transfer full. Changed it to ref on set_property and added unrefs in clients for better code clarity. > > @@ +317,3 @@ > + > file_name_widget_entry_on_directory_info_ready, > + self); > + g_clear_object (&priv->containing_directory); > > unrefing here but you don't ref on set_property? > > ::: src/nautilus-file-name-widget-controller.h > @@ +15,3 @@ > + GObjectClass parent_class; > + > + gchar * (*get_new_name) (NautilusFileNameWidgetController > *controller); > > could you align these signals? Not quite sure how they should be aligned but I gave it a shot! > > @@ +26,3 @@ > + void (*name_accepted) (NautilusFileNameWidgetController > *controller); > + > + gpointer padding[10]; > > Nautilus doesn't have public API, we don't need this. Right. > > ::: src/nautilus-files-view.c > @@ +1780,3 @@ > + g_signal_connect (view->details->rename_file_controller, > + "name-accepted", > + > (GCallback)rename_file_popover_controller_name_accepted, > > on_name_accepted. > > @@ +1784,3 @@ > + g_signal_connect (view->details->rename_file_controller, > + "cancelled", > + > (GCallback)rename_file_popover_controller_cancelled, > > dito > > @@ +1860,3 @@ > + g_signal_connect (view->details->new_folder_controller, > + "name-accepted", > + > (GCallback)new_folder_dialog_controller_name_accepted, > > *_on_* > > @@ +1864,3 @@ > + g_signal_connect (view->details->new_folder_controller, > + "cancelled", > + (GCallback)new_folder_dialog_controller_cancelled, > > dito > > ::: src/nautilus-new-folder-dialog-controller.c > @@ +19,3 @@ > + > +static void > +nautilus_new_folder_dialog_controller_name_accepted > (NautilusFileNameWidgetController *controller) > > _on_ > > @@ +25,3 @@ > + self = NAUTILUS_NEW_FOLDER_DIALOG_CONTROLLER (controller); > + > + if (self->response_handler_id) { > > can this be 0? Yeah, it can be 0. If the activate_button is clicked, it will trigger the response signal on the dialog and the handler is disconnected there. It will also call *_call_when_ready, and the "name-accepted" signal is emitted in its callback. But when the signal is emitted, the response handler will already be disconnected, so the response is not sent again. > > @@ +34,3 @@ > +nautilus_new_folder_dialog_controller_name_is_valid > (NautilusFileNameWidgetController *self, > + gchar > *name, > + gchar > **error_message) > > aligment What's wrong with this? I even tried it in builder and it indented it the same. > > ::: src/nautilus-rename-file-popover-controller.c > @@ +109,3 @@ > + GtkWidget *name_label; > + NautilusDirectory *containing_directory; > + gint start_offset, end_offset; > > one per line Woops. > > @@ +159,3 @@ > + gtk_popover_set_default_widget (GTK_POPOVER (rename_file_popover), > activate_button); > + gtk_popover_set_pointing_to (GTK_POPOVER (rename_file_popover), > pointing_to); > + gtk_popover_set_relative_to (GTK_POPOVER (rename_file_popover), > relative_to); > > what if both are set? I don't understand, what scenario do you have in mind? > > @@ +162,3 @@ > + > + gtk_widget_show (rename_file_popover); > + gtk_widget_grab_focus (name_entry); > > is this needed? Why doesn't take it automatically? Hmm, it is done automatically indeed. Removed it and made name_entry the default widget for the popover. > > @@ +180,3 @@ > +nautilus_rename_file_popover_controller_get_target_file > (NautilusRenameFilePopoverController *self) > +{ > + g_assert (NAUTILUS_IS_RENAME_FILE_POPOVER_CONTROLLER (self)); > > crashing the application on pourpose if the rename cannot be done sounds > like too much :) :( can I at least set the user's PC on fire? > > use g_return_val_if_fail.
Created attachment 332490 [details] [review] files-view: move file name widget logic to separate controllers The rename file popover and the new folder dialog share common logic for validating file names entered by the user. The control logic was implemented with a simple structure in files-view. Besides common logic, the structure also held parameters specific to only one of the operations. Another problem is that the current implementation does not allow flexibility in obtaining the file name from the widgets and displaying error messages. In order to fix this, reimplement the structure as an abstract class and create two subclasses for the "Rename" and "New Folder" widgets.
> Well, in my use cases, the directory parameter was sent transfer full. > Changed it to ref on set_property and added unrefs in clients for better > code clarity. > Set's never transfer full, instead the client has the responsibility of managing those. It's mostly about not transferring responsibilities if not necessary. > > > > @@ +317,3 @@ > > + > > file_name_widget_entry_on_directory_info_ready, > > + self); > > + g_clear_object (&priv->containing_directory); > > > > unrefing here but you don't ref on set_property? > > > > ::: src/nautilus-file-name-widget-controller.h > > @@ +15,3 @@ > > + GObjectClass parent_class; > > + > > + gchar * (*get_new_name) (NautilusFileNameWidgetController > > *controller); > > > > could you align these signals? > > Not quite sure how they should be aligned but I gave it a shot! > > > > > @@ +26,3 @@ > > + void (*name_accepted) (NautilusFileNameWidgetController > > *controller); > > + > > + gpointer padding[10]; > > > > Nautilus doesn't have public API, we don't need this. > > Right. > > > > ::: src/nautilus-files-view.c > > @@ +1780,3 @@ > > + g_signal_connect (view->details->rename_file_controller, > > + "name-accepted", > > + > > (GCallback)rename_file_popover_controller_name_accepted, > > > > on_name_accepted. > > > > @@ +1784,3 @@ > > + g_signal_connect (view->details->rename_file_controller, > > + "cancelled", > > + > > (GCallback)rename_file_popover_controller_cancelled, > > > > dito > > > > @@ +1860,3 @@ > > + g_signal_connect (view->details->new_folder_controller, > > + "name-accepted", > > + > > (GCallback)new_folder_dialog_controller_name_accepted, > > > > *_on_* > > > > @@ +1864,3 @@ > > + g_signal_connect (view->details->new_folder_controller, > > + "cancelled", > > + (GCallback)new_folder_dialog_controller_cancelled, > > > > dito > > > > ::: src/nautilus-new-folder-dialog-controller.c > > @@ +19,3 @@ > > + > > +static void > > +nautilus_new_folder_dialog_controller_name_accepted > > (NautilusFileNameWidgetController *controller) > > > > _on_ > > > > @@ +25,3 @@ > > + self = NAUTILUS_NEW_FOLDER_DIALOG_CONTROLLER (controller); > > + > > + if (self->response_handler_id) { > > > > can this be 0? > > Yeah, it can be 0. If the activate_button is clicked, it will trigger the > response signal on the dialog and the handler is disconnected there. It will > also call *_call_when_ready, and the "name-accepted" signal is emitted in > its callback. But when the signal is emitted, the response handler will > already be disconnected, so the response is not sent again. This only happen if the dialog is cancelled right? If it's ok, why do we allow to accept the dialog if the call_when_ready is not ready? In case of cancellation, why don't we cancel the call_when_ready? > > What's wrong with this? I even tried it in builder and it indented it the > same. Try setting different tab width, it will have wrong aligment I think. At least here in bugzilla is wrong aligned. > I don't understand, what scenario do you have in mind? None, just safe programming. Give preference to one (probably to the relative_to). > > :( can I at least set the user's PC on fire? Yes please :)
Created attachment 332578 [details] [review] files-view: move file name widget logic to separate controllers The rename file popover and the new folder dialog share common logic for validating file names entered by the user. The control logic was implemented with a simple structure in files-view. Besides common logic, the structure also held parameters specific to only one of the operations. Another problem is that the current implementation does not allow flexibility in obtaining the file name from the widgets and displaying error messages. In order to fix this, reimplement the structure as an abstract class and create two subclasses for the "Rename" and "New Folder" widgets.
(In reply to Carlos Soriano from comment #9) > Set's never transfer full, instead the client has the responsibility of > managing those. > It's mostly about not transferring responsibilities if not necessary. Oh, I see. Thanks for pointing this out :) > This only happen if the dialog is cancelled right? > If it's ok, why do we allow to accept the dialog if the call_when_ready is > not ready? > In case of cancellation, why don't we cancel the call_when_ready? Ahh.. my mind played tricks on me on this one. What I had in mind was some complicated scenario which is no longer the case. I think I can safely remove the handler because the possible cases are: 1) if the name_entry gets activated, the dialog will be destroyed with no response 2) if the activate_button is clicked, the dialog will receive GTK_RESPONSE_OK and then be destroyed 3) if the dialog is canceled / deleted, the dialog will receive a different signal and "cancelled" will be emitted by the controller So the "name-accepted" handler is not actually needed, right? > > > > > What's wrong with this? I even tried it in builder and it indented it the > > same. > > Try setting different tab width, it will have wrong aligment I think. At > least here in bugzilla is wrong aligned. Nope, still nothing. I'm reaaaally confused. Could you apply the latest patch I submitted and see if the misalignment is actually there? I really don't know what to do. > > > I don't understand, what scenario do you have in mind? > > None, just safe programming. Give preference to one (probably to the > relative_to). I still don't get it. Am I not expected to set both relative_to and pointing_to?
> 1) if the name_entry gets activated, the dialog will be destroyed with no > response I'm confused, why not response if the entry gets activated? > 2) if the activate_button is clicked, the dialog will receive > GTK_RESPONSE_OK and then be destroyed > 3) if the dialog is canceled / deleted, the dialog will receive a different > signal and "cancelled" will be emitted by the controller > > So the "name-accepted" handler is not actually needed, right? I don't think so, yeah. > Nope, still nothing. I'm reaaaally confused. Could you apply the latest > patch I submitted and see if the misalignment is actually there? I really > don't know what to do. Nevermind :) > > > > > > I don't understand, what scenario do you have in mind? > > > > None, just safe programming. Give preference to one (probably to the > > relative_to). > > I still don't get it. Am I not expected to set both relative_to and > pointing_to? No, both are mutual exclusive afaik.
Created attachment 332605 [details] [review] files-view: move file name widget logic to separate controllers The rename file popover and the new folder dialog share common logic for validating file names entered by the user. The control logic was implemented with a simple structure in files-view. Besides common logic, the structure also held parameters specific to only one of the operations. Another problem is that the current implementation does not allow flexibility in obtaining the file name from the widgets and displaying error messages. In order to fix this, reimplement the structure as an abstract class and create two subclasses for the "Rename" and "New Folder" widgets.
Review of attachment 332605 [details] [review]: Looking good ::: src/nautilus-file-name-widget-controller.c @@ +73,3 @@ + gchar **error_message) +{ + g_assert (*error_message == NULL); Crashing nautilus for a small error again? :) @@ +151,3 @@ + + if (existing_file != NULL) { + nautilus_file_unref (existing_file); Not need to check for null. ::: src/nautilus-files-view.c @@ +1792,1 @@ + pos = context_menu_to_file_operation_position (view); Don't abbreviate ::: src/nautilus-new-folder-dialog-controller.c @@ +23,3 @@ + gchar **error_message) +{ + g_assert (*error_message == NULL); meeeh ::: src/nautilus-rename-file-popover-controller.c @@ +48,3 @@ + NautilusRenameFilePopoverController *self; + + g_assert (*error_message == NULL); MEEEEEEEEEEEEEEEEEEH @@ +68,3 @@ + } + else { + *error_message = _("A file can not be called “.”."); cannot
(In reply to Carlos Soriano from comment #14) > Crashing nautilus for a small error again? :) It is actually going to crash anyway with a null pointer. The behavior of strlen() is implementation-defined in this regard.
(In reply to Ernestas Kulik from comment #15) > (In reply to Carlos Soriano from comment #14) > > Crashing nautilus for a small error again? :) > > It is actually going to crash anyway with a null pointer. The behavior of > strlen() is implementation-defined in this regard. And I just realized that I am talking nonsense. Ignore my comment. :)
Created attachment 333224 [details] [review] files-view: move file name widget logic to separate controllers The rename file popover and the new folder dialog share common logic for validating file names entered by the user. The control logic was implemented with a simple structure in files-view. Besides common logic, the structure also held parameters specific to only one of the operations. Another problem is that the current implementation does not allow flexibility in obtaining the file name from the widgets and displaying error messages. In order to fix this, reimplement the structure as an abstract class and create two subclasses for the "Rename" and "New Folder" widgets.
Review of attachment 333224 [details] [review]: Can you give it a look Ernestas? I think it looks good to me! But my brain is kind of dead of reviewing :( Thanks Razvan!
Review of attachment 333224 [details] [review]: LGTM aside from the nitpicks. ::: src/nautilus-new-folder-dialog-controller.c @@ +59,3 @@ +{ + NautilusNewFolderDialogController *self; + GtkBuilder *builder; Why not an autoptr? @@ +132,3 @@ + } + gtk_widget_destroy (self->new_folder_dialog); + self->new_folder_dialog = NULL; Wouldn’t “g_clear_pointer (&self->new_folder_dialog, gtk_widget_destroy)” work here? ::: src/nautilus-rename-file-popover-controller.c @@ +100,3 @@ +{ + NautilusRenameFilePopoverController *self; + GtkBuilder *builder; Why not an autoptr?
Created attachment 333615 [details] [review] files-view: move file name widget logic to separate controllers The rename file popover and the new folder dialog share common logic for validating file names entered by the user. The control logic was implemented with a simple structure in files-view. Besides common logic, the structure also held parameters specific to only one of the operations. Another problem is that the current implementation does not allow flexibility in obtaining the file name from the widgets and displaying error messages. In order to fix this, reimplement the structure as an abstract class and create two subclasses for the "Rename" and "New Folder" widgets.
Attachment 332443 [details] pushed as e993190 - files-view: correct method name Attachment 333615 [details] pushed as fd873c5 - files-view: move file name widget logic to separate controllers