GNOME Bugzilla – Bug 775092
Port from eel
Last modified: 2021-06-18 15:52:38 UTC
Most of the things there are already in glib, or they are already easy enough to have it directly in the code, or we should move them to glib. So port the functions in eel inside nautilus in order to remove the whole eel. Good for newcomers, you can choose most of functions in there.
Created attachment 345044 [details] [review] nautilus-ui-utilities: Removed unused header file
Created attachment 345050 [details] [review] nautilus-window-slot: Port from eel to glib
Created attachment 345088 [details] [review] nautilus-window-slot: Port from eel to glib
Review of attachment 345044 [details] [review]: Well spotted! The commit message is missing though (although I agree there is not much to say), try to follow https://wiki.gnome.org/Newcomers/CodeContributionWorkflow#Commit_guidelines. In this case, more in a academic way than anything, I would say something like: nautilus-ui-utilities: Removed unused header file Currently ui-utilities includes a header that no longer exist. Although it's just omitted by the compiler, it's unnecessary and pollutes the code. This patch removes the unused header
Review of attachment 345088 [details] [review]: Good newcomer bug, code looks pretty good. The commit message is missing as in the previous one. Try to say why we want this change (if you don't know, feel free to ask). Also: ::: src/nautilus-window-slot.c @@ +1449,3 @@ + G_CALLBACK (gtk_widget_destroy), NULL); + + g_object_set (dialog, instead of running it, just show it. Running blocks the main thread, so it would block other nautilus windows too, which is undesirable.
Created attachment 345108 [details] [review] nautilus-ui-utilities: Removed unused header file Currently ui-utilities includes a header that no longer exist. Although it's just omitted by the compiler, it's unnecessary and pollutes the code. This patch removes the unused header.
Created attachment 345109 [details] [review] ui-utilities: Removed unused header file Currently ui-utilities includes a header that no longer exist. Although it's just omitted by the compiler, it's unnecessary and pollutes the code. This patch removes the unused header.
Created attachment 345114 [details] [review] window-slot: Port from eel to glib Most of the things in eel are already in glib, or are already easy enough to have them directly in the code. So we should remove eel which is just another layer of abstraction that we don't need as it makes it hard to follow the code. This patch removes the eel functions and eel headers. And replaces them with equivalent glib code.
Review of attachment 345114 [details] [review]: eel_show_error_dialog () is used in quite a few places, so an ad-hoc approach like this would result in quite a bit of duplicate code. I would say, go big, write an equivalent shared function somewhere in Nautilus and replace the call everywhere. What do you think, Carlos? ::: src/nautilus-window-slot.c @@ +1444,3 @@ + "text", error_message, + "secondary-text", detail_message, + NULL); I wouldn’t set properties directly when equivalent API is available. It will also result in shorter code, so it’s a win-win. @@ +1447,3 @@ + + g_signal_connect (dialog, "response", + G_CALLBACK (gtk_widget_destroy), NULL); I’m fairly sure this will result in UB, as the callback will be called from an incompatible pointer (gtk_widget_destroy () takes one argument, whereas the callback is expected to have three params).
(In reply to Ernestas Kulik from comment #9) > Review of attachment 345114 [details] [review] [review]: > > eel_show_error_dialog () is used in quite a few places, so an ad-hoc > approach like this would result in quite a bit of duplicate code. > I would say, go big, write an equivalent shared function somewhere in > Nautilus and replace the call everywhere. > > What do you think, Carlos? Yes, you are right. It's too much duplicated effort, I overlook all the places where it is used. > > ::: src/nautilus-window-slot.c > @@ +1444,3 @@ > + "text", error_message, > + "secondary-text", detail_message, > + NULL); > > I wouldn’t set properties directly when equivalent API is available. It will > also result in shorter code, so it’s a win-win. I was going to say the same comment in my review, but I couldn't find the API for those properties. Where is it? > > @@ +1447,3 @@ > + > + g_signal_connect (dialog, "response", > + G_CALLBACK (gtk_widget_destroy), NULL); > > I’m fairly sure this will result in UB, as the callback will be called from > an incompatible pointer (gtk_widget_destroy () takes one argument, whereas > the callback is expected to have three params). it's pretty usual to do the connection to response and then gtk_widget_destroy when there is more than one parameter. This is fine as long as the first parameter is the widget that you will destroy, and that's why g_signal_connect_swapped exists in the first place (also what is UB?)
(In reply to Carlos Soriano from comment #10) > (In reply to Ernestas Kulik from comment #9) > > ::: src/nautilus-window-slot.c > > @@ +1444,3 @@ > > + "text", error_message, > > + "secondary-text", detail_message, > > + NULL); > > > > I wouldn’t set properties directly when equivalent API is available. It will > > also result in shorter code, so it’s a win-win. > > I was going to say the same comment in my review, but I couldn't find the > API for those properties. Where is it? The format string for the primary text can be passed to gtk_message_dialog_new (). gtk_message_dialog_format_secondary_text () can be used for the secondary text. > > > > @@ +1447,3 @@ > > + > > + g_signal_connect (dialog, "response", > > + G_CALLBACK (gtk_widget_destroy), NULL); > > > > I’m fairly sure this will result in UB, as the callback will be called from > > an incompatible pointer (gtk_widget_destroy () takes one argument, whereas > > the callback is expected to have three params). > > it's pretty usual to do the connection to response and then > gtk_widget_destroy > when there is more than one parameter. This is fine as long as the first > parameter is the widget that you will destroy, and that's why > g_signal_connect_swapped exists in the first place (also what is UB?) UB is undefined behavior. This is what I had in mind: ISO/IEC 9899:1999 (E) 6.3.2.3.8: A pointer to a function of one type may be converted to a pointer to a function of another type and back again; the result shall compare equal to the original pointer. If a converted pointer is used to call a function whose type is not compatible with the pointed-to type, the behavior is undefined.
(In reply to Ernestas Kulik from comment #11) > UB is undefined behavior. > > This is what I had in mind: > > ISO/IEC 9899:1999 (E) 6.3.2.3.8: > A pointer to a function of one type may be converted to a pointer to a > function of another type and back again; the result shall compare equal to > the original pointer. If a converted pointer is used to call a function > whose type is not compatible with the pointed-to type, the behavior is > undefined. Apparently, it largely depends on the calling convention and should work fine on most platforms. I was just being overly strict regarding correctness. :)
Created attachment 345140 [details] [review] ui-utilities: Add show_error_dialog function Most of the things in eel are already in glib, or are already easy enough to have them directly in the code. So we should remove eel which is just another layer of abstraction that we don't need as it makes it hard to follow the code. This patch adds show_error_dialog function which is to be used in place of eel_show_error_dialog.
Created attachment 345141 [details] [review] window-slot: Port from eel to glib Most of the things in eel are already in glib, or are already easy enough to have them directly in the code. So we should remove eel which is just another layer of abstraction that we don't need as it makes it hard to follow the code. This patch replaces eel_show_error_dialog with show_error_dialog to make it independent of eel.
Review of attachment 345140 [details] [review]: looks good, good work!
Review of attachment 345141 [details] [review]: the title of the commit is misleading, the dialog is not ported to glib. Rather say somthing like: "window-slot: don't use eel to show a dialog"
Created attachment 345219 [details] [review] window-slot: Don't use eel to show a dialog Most of the things in eel are already in glib, or are already easy enough to have them directly in the code. So we should remove eel which is just another layer of abstraction that we don't need as it makes it hard to follow the code. This patch replaces eel_show_error_dialog with show_error_dialog to make it independent of eel.
Review of attachment 345219 [details] [review]: now it's good, congrats!!
Created attachment 345507 [details] [review] window: Don't use eel to show a dialog Most of the things in eel are already in glib, or are already easy enough to have them directly in the code. So we should remove eel which is just another layer of abstraction that we don't need as it makes it hard to follow the code. This patch replaces eel_show_error_dialog with show_error_dialog to make it independent of eel.
Review of attachment 345140 [details] [review]: don't commit it. we need to replace eel_timed_wait_start, eel_timed_wait_start_with_duration, eel_timed_wait_stop, eel_run_simple_dialog, eel_create_question_dialog, eel_show_error_dialog, eel_show_warning_dialog, eel_show_yes_no_dialog. so if you commit it in the current state that would only result in more duplicate code. let me create/replace all the functions first so that duplicate code can be kept to minimum.
Created attachment 345966 [details] [review] ui-utilities: Add custom dialogs Most of the things in eel are already in glib, or are already easy enough to have them directly in the code. So we should remove eel which is just another layer of abstraction that we don't need as it makes it hard to follow the code. This patch adds functions for custom dialogs which are to be used in place of eel-stock-dialogs.
Created attachment 345968 [details] [review] application: Don't use eel to show a dialog Most of the things in eel are already in glib, or are already easy enough to have them directly in the code. So we should remove eel which is just another layer of abstraction that we don't need as it makes it hard to follow the code. This patch replaces eel_show_error_dialog with show_error_dialog.
Created attachment 345975 [details] [review] canvas-dnd: Removed unused header file Currently canvas-dnd includes a header that is not used. Although it's just omitted by the compiler, it's unnecessary and pollutes the code. This patch removes the unused header.
Created attachment 345980 [details] [review] error-reporting: Don't use eel to show dialogs Most of the things in eel are already in glib, or are already easy enough to have them directly in the code. So we should remove eel which is just another layer of abstraction that we don't need as it makes it hard to follow the code. This patch replaces eel's custom dialogs with ui-utilities custom dialogs.
Created attachment 345983 [details] [review] file-operations: Don't use eel to show dialogs Most of the things in eel are already in glib, or are already easy enough to have them directly in the code. So we should remove eel which is just another layer of abstraction that we don't need as it makes it hard to follow the code. This patch replaces eel_show_error_dialog with show_error_dialog.
Created attachment 345985 [details] [review] file-utilities: Don't use eel to show dialogs Most of the things in eel are already in glib, or are already easy enough to have them directly in the code. So we should remove eel which is just another layer of abstraction that we don't need as it makes it hard to follow the code. This patch replaces eel's custom dialogs with ui-utilities custom dialogs.
Created attachment 346082 [details] [review] files-view-dnd: Don't use eel to show dialogs Most of the things in eel are already in glib, or are already easy enough to have them directly in the code. So we should remove eel which is just another layer of abstraction that we don't need as it makes it hard to follow the code. This patch replaces eel's custom dialogs with ui-utilities custom dialogs.
Created attachment 346083 [details] [review] files-view: Don't use eel to show dialogs Most of the things in eel are already in glib, or are already easy enough to have them directly in the code. So we should remove eel which is just another layer of abstraction that we don't need as it makes it hard to follow the code. This patch replaces eel's custom dialogs with ui-utilities custom dialogs.
Created attachment 346084 [details] [review] global-preferences: Removed unused header file Currently canvas-dnd includes a header that is not used. Although it's just omitted by the compiler, it's unnecessary and pollutes the code. This patch removes the unused header.
Created attachment 346085 [details] [review] location-bar: Don't use eel to show a dialog Most of the things in eel are already in glib, or are already easy enough to have them directly in the code. So we should remove eel which is just another layer of abstraction that we don't need as it makes it hard to follow the code. This patch replaces eel custom dialog with ui-utilities custom dialog.
Created attachment 346088 [details] [review] mime-actions: Don't use eel to show dialogs Most of the things in eel are already in glib, or are already easy enough to have them directly in the code. So we should remove eel which is just another layer of abstraction that we don't need as it makes it hard to follow the code. This patch replaces eel custom dialogs with ui-utilities custom dialogs.
Created attachment 346089 [details] [review] mime-application-chooser: Don't use eel to show dialogs Most of the things in eel are already in glib, or are already easy enough to have them directly in the code. So we should remove eel which is just another layer of abstraction that we don't need as it makes it hard to follow the code. This patch replaces eel custom dialogs with ui-utilities custom dialogs.
Created attachment 346091 [details] [review] program-choosing: Don't use eel to show dialogs Most of the things in eel are already in glib, or are already easy enough to have them directly in the code. So we should remove eel which is just another layer of abstraction that we don't need as it makes it hard to follow the code. This patch replaces eel custom dialogs with ui-utilities custom dialogs.
Created attachment 346092 [details] [review] properties-window: Don't use eel to show dialogs Most of the things in eel are already in glib, or are already easy enough to have them directly in the code. So we should remove eel which is just another layer of abstraction that we don't need as it makes it hard to follow the code. This patch replaces eel custom dialogs with ui-utilities custom dialogs.
Created attachment 346094 [details] [review] mime-actions: Don't use eel to show dialogs Most of the things in eel are already in glib, or are already easy enough to have them directly in the code. So we should remove eel which is just another layer of abstraction that we don't need as it makes it hard to follow the code. This patch replaces eel custom dialogs with ui-utilities custom dialogs.
Created attachment 346324 [details] [review] error-reporting: Don't use eel to show dialogs Most of the things in eel are already in glib, or are already easy enough to have them directly in the code. So we should remove eel which is just another layer of abstraction that we don't need as it makes it hard to follow the code. This patch replaces eel's custom dialogs with ui-utilities custom dialogs.
Review of attachment 345507 [details] [review]: LGTM
Review of attachment 345966 [details] [review]: I thin we should go one by one, I believe some of them can go directly into the code. Definitely the timed dialog is useful, but not sure about others. My intention is to move one by one this functions, modernizing them, making sure we use the proper API and the style (in the headers you were using tabs, although I know most of them use tabs we try to not to). So what do you think of evaluating and making a patch for one function at a time? And we can see the options we have for that specifically.
Created attachment 347206 [details] [review] ui-utilities: Add custom function to display error dialog Most of the things in eel are already in glib, or are already easy enough to have them directly in the code. So we should remove eel which is just another layer of abstraction that we don't need as it makes it hard to follow the code. This patch adds function to show error dialog (show_error_dialog) which is to be used in place of eel_show_error_dialog.
Created attachment 347209 [details] [review] ui-utilities: Add custom function to display error dialog Most of the things in eel are already in glib, or are already easy enough to have them directly in the code. So we should remove eel which is just another layer of abstraction that we don't need as it makes it hard to follow the code. This patch adds function to show error dialog (show_error_dialog) which is to be used in place of eel_show_error_dialog.
Created attachment 347211 [details] [review] ui-utilities: Add custom function to display error dialog Most of the things in eel are already in glib, or are already easy enough to have them directly in the code. So we should remove eel which is just another layer of abstraction that we don't need as it makes it hard to follow the code. This patch adds function to show error dialog (show_error_dialog) which is to be used in place of eel_show_error_dialog.
Review of attachment 347211 [details] [review]: Heya! This looks almost perfect, this is the way I intended this bug report to be. Sorry for the confusion before.... Some details: ::: src/nautilus-ui-utilities.c @@ +464,3 @@ +static GtkWidget * +create_message_dialog (const gchar *primary_text, + const gchar *secondary_text, this code is easy enough to have it just in the other funtion @@ +475,3 @@ + GTK_BUTTONS_OK, + "%s", primary_text); +{ we should always have a parent. So this should be removed. Or is there any case that we need a dialog without parent? @@ +486,3 @@ +} + + GTK_BUTTONS_OK, I'm not sure why we should return a GtkDialog instead of what gtk_message_dialog_new returns. I actually like using the specific type, but I wonder if it makes sense to do this here but not in other places. For now let's leave it like this.
(In reply to Carlos Soriano from comment #42) > Review of attachment 347211 [details] [review] [review]: > > Heya! This looks almost perfect, this is the way I intended this bug report > to be. Sorry for the confusion before.... > Some details: > > ::: src/nautilus-ui-utilities.c > @@ +464,3 @@ > +static GtkWidget * > +create_message_dialog (const gchar *primary_text, > + const gchar *secondary_text, > > this code is easy enough to have it just in the other funtion > > @@ +475,3 @@ > + GTK_BUTTONS_OK, > + "%s", primary_text); > +{ > > we should always have a parent. So this should be removed. Or is there any > case that we need a dialog without parent? nautilus-application.c: in check_required_directories dialog = eel_show_error_dialog (error_string, detail_string, NULL); i found this occurence so thought it must be possible :) n that is why i used the create_message_dialog function cz it was putting more redundant code in the file. > @@ +486,3 @@ > +} > + > + GTK_BUTTONS_OK, > > I'm not sure why we should return a GtkDialog instead of what > gtk_message_dialog_new returns. I actually like using the specific type, but > I wonder if it makes sense to do this here but not in other places. For now > let's leave it like this. okay, left like this.
Created attachment 347319 [details] [review] ui-utilities: Add custom function to display error dialog Most of the things in eel are already in glib, or are already easy enough to have them directly in the code. So we should remove eel which is just another layer of abstraction that we don't need as it makes it hard to follow the code. This patch adds function to show error dialog (show_error_dialog) which is to be used in place of eel_show_error_dialog.
Review of attachment 347319 [details] [review]: The code is starting to look good! Would be good to replace one of the current eel_show_error_dialog functions used in the code with this one, otherwise the compiler could warn about unused function present in the code. Also it serves as a testing. Also few last details: ::: src/nautilus-ui-utilities.c @@ +482,3 @@ + gtk_dialog_set_default_response (GTK_DIALOG (dialog), GTK_RESPONSE_OK); + + dialog = gtk_message_dialog_new (parent, You don't need the casting here, as it is already a GtkWidget @@ +484,3 @@ + gtk_widget_show (GTK_WIDGET (dialog)); + + GTK_DIALOG_MODAL, dialog here is a widget, not a GtkDialog. This needs casting to a GtkDialog. Surprisingly this doesn't raise a warning, but it should :/ something to report to glib I guess.
Created attachment 347392 [details] [review] ui-utilities: Add custom function to display error dialog Most of the things in eel are already in glib, or are already easy enough to have them directly in the code. So we should remove eel which is just another layer of abstraction that we don't need as it makes it hard to follow the code. This patch adds function to show error dialog (show_error_dialog) which is to be used in place of eel_show_error_dialog.
Review of attachment 345968 [details] [review]: let's reject since the intention is to go one by one. The code can still be consulted.
Review of attachment 345975 [details] [review]: let's reject since the intention is to go one by one. The code can still be consulted.
Review of attachment 345985 [details] [review]: let's reject since the intention is to go one by one. The code can still be consulted.
Review of attachment 346082 [details] [review]: let's reject since the intention is to go one by one. The code can still be consulted.
Review of attachment 346084 [details] [review]: let's reject since the intention is to go one by one. The code can still be consulted.
Review of attachment 346085 [details] [review]: let's reject since the intention is to go one by one. The code can still be consulted.
Review of attachment 346089 [details] [review]: let's reject since the intention is to go one by one. The code can still be consulted.
Review of attachment 346091 [details] [review]: let's reject since the intention is to go one by one. The code can still be consulted.
Review of attachment 347392 [details] [review]: Excellent, congrats!!
Those commited now, congrats! Attachment 345219 [details] pushed as eb23c63 - window-slot: Don't use eel to show a dialog Attachment 345507 [details] pushed as df262b5 - window: Don't use eel to show a dialog Attachment 347392 [details] pushed as 460235b - ui-utilities: Add custom function to display error dialog
Created attachment 347498 [details] [review] application: Don't use eel to show dialog Most of the things in eel are already in glib, or are already easy enough to have them directly in the code. So we should remove eel which is just another layer of abstraction that we don't need as it makes it hard to follow the code. This patch uses show_error_dialog in place of eel_show_error_dialog.
Review of attachment 347498 [details] [review]: ::: src/nautilus-application.c @@ +197,2 @@ g_string_free (directories_as_string, TRUE); + g_free (active_window); This object is ref counted, you don't want to free it like that. Also, the documentation for gtk_application_get_active_window says "Transfer None", that means the ownership of the window is not from this part of the code. Supposedly this code should crash actually. Was it working for you?
(In reply to Carlos Soriano from comment #58) > Review of attachment 347498 [details] [review] [review]: > > ::: src/nautilus-application.c > @@ +197,2 @@ > g_string_free (directories_as_string, TRUE); > + g_free (active_window); > > This object is ref counted, you don't want to free it like that. Also, the > documentation for gtk_application_get_active_window says "Transfer None", > that means the ownership of the window is not from this part of the code. > Supposedly this code should crash actually. Was it working for you? CRITICAL **: show_error_dialog: assertion 'parent != NULL' failed is raised.
Created attachment 349126 [details] [review] files-view: Replace eel_show_error_dialog Now that show_error_dialog is implemented we should not use eel_show_error_dialog anymore. This patch removes dependency of eel by using buildin function show_error_dialog.
Review of attachment 349126 [details] [review]: Hey! Sorry for the late response. The code looks good! but the identation is off, make sure arguments are aligned.
Hello, Kartikeya Sharma, Stanca Robert, can you update the patches according to the review?
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version of Files (nautilus), then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new ticket at https://gitlab.gnome.org/GNOME/nautilus/-/issues/ Thank you for your understanding and your help.