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 775092 - Port from eel
Port from eel
Status: RESOLVED OBSOLETE
Product: nautilus
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Nautilus Maintainers
Nautilus Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-11-25 15:56 UTC by Carlos Soriano
Modified: 2021-06-18 15:52 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
nautilus-ui-utilities: Removed unused header file (695 bytes, patch)
2017-02-06 17:22 UTC, Kartikeya Sharma
none Details | Review
nautilus-window-slot: Port from eel to glib (1.95 KB, patch)
2017-02-06 19:02 UTC, Kartikeya Sharma
none Details | Review
nautilus-window-slot: Port from eel to glib (1.89 KB, patch)
2017-02-07 09:28 UTC, Kartikeya Sharma
reviewed Details | Review
nautilus-ui-utilities: Removed unused header file (882 bytes, patch)
2017-02-07 12:45 UTC, Kartikeya Sharma
none Details | Review
ui-utilities: Removed unused header file (873 bytes, patch)
2017-02-07 12:49 UTC, Kartikeya Sharma
none Details | Review
window-slot: Port from eel to glib (2.21 KB, patch)
2017-02-07 13:08 UTC, Kartikeya Sharma
none Details | Review
ui-utilities: Add show_error_dialog function (2.62 KB, patch)
2017-02-07 19:40 UTC, Kartikeya Sharma
needs-work Details | Review
window-slot: Port from eel to glib (1.63 KB, patch)
2017-02-07 19:48 UTC, Kartikeya Sharma
reviewed Details | Review
window-slot: Don't use eel to show a dialog (1.64 KB, patch)
2017-02-08 13:54 UTC, Kartikeya Sharma
committed Details | Review
window: Don't use eel to show a dialog (1.32 KB, patch)
2017-02-11 04:58 UTC, Kartikeya Sharma
committed Details | Review
ui-utilities: Add custom dialogs (18.64 KB, patch)
2017-02-16 16:59 UTC, Kartikeya Sharma
none Details | Review
application: Don't use eel to show a dialog (1.50 KB, patch)
2017-02-16 17:15 UTC, Kartikeya Sharma
rejected Details | Review
canvas-dnd: Removed unused header file (946 bytes, patch)
2017-02-16 17:32 UTC, Kartikeya Sharma
rejected Details | Review
error-reporting: Don't use eel to show dialogs (3.72 KB, patch)
2017-02-16 17:44 UTC, Kartikeya Sharma
none Details | Review
file-operations: Don't use eel to show dialogs (2.29 KB, patch)
2017-02-16 17:56 UTC, Kartikeya Sharma
none Details | Review
file-utilities: Don't use eel to show dialogs (1.90 KB, patch)
2017-02-16 18:01 UTC, Kartikeya Sharma
rejected Details | Review
files-view-dnd: Don't use eel to show dialogs (3.09 KB, patch)
2017-02-17 16:15 UTC, Kartikeya Sharma
rejected Details | Review
files-view: Don't use eel to show dialogs (4.55 KB, patch)
2017-02-17 16:22 UTC, Kartikeya Sharma
none Details | Review
global-preferences: Removed unused header file (954 bytes, patch)
2017-02-17 16:27 UTC, Kartikeya Sharma
rejected Details | Review
location-bar: Don't use eel to show a dialog (2.71 KB, patch)
2017-02-17 16:31 UTC, Kartikeya Sharma
rejected Details | Review
mime-actions: Don't use eel to show dialogs (8.34 KB, patch)
2017-02-17 16:41 UTC, Kartikeya Sharma
none Details | Review
mime-application-chooser: Don't use eel to show dialogs (3.10 KB, patch)
2017-02-17 16:46 UTC, Kartikeya Sharma
rejected Details | Review
program-choosing: Don't use eel to show dialogs (3.03 KB, patch)
2017-02-17 16:51 UTC, Kartikeya Sharma
rejected Details | Review
properties-window: Don't use eel to show dialogs (5.42 KB, patch)
2017-02-17 16:57 UTC, Kartikeya Sharma
none Details | Review
mime-actions: Don't use eel to show dialogs (7.90 KB, patch)
2017-02-17 17:00 UTC, Kartikeya Sharma
none Details | Review
error-reporting: Don't use eel to show dialogs (3.79 KB, patch)
2017-02-21 13:35 UTC, Kartikeya Sharma
none Details | Review
ui-utilities: Add custom function to display error dialog (23.65 KB, patch)
2017-03-04 15:14 UTC, Kartikeya Sharma
none Details | Review
ui-utilities: Add custom function to display error dialog (3.08 KB, patch)
2017-03-04 15:30 UTC, Kartikeya Sharma
none Details | Review
ui-utilities: Add custom function to display error dialog (3.09 KB, patch)
2017-03-04 15:36 UTC, Kartikeya Sharma
none Details | Review
ui-utilities: Add custom function to display error dialog (2.64 KB, patch)
2017-03-06 14:58 UTC, Kartikeya Sharma
none Details | Review
ui-utilities: Add custom function to display error dialog (4.33 KB, patch)
2017-03-07 13:39 UTC, Kartikeya Sharma
committed Details | Review
application: Don't use eel to show dialog (1.95 KB, patch)
2017-03-08 19:15 UTC, Kartikeya Sharma
needs-work Details | Review
files-view: Replace eel_show_error_dialog (3.06 KB, patch)
2017-04-01 22:32 UTC, Stanca Robert
reviewed Details | Review

Description Carlos Soriano 2016-11-25 15:56:22 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.
Comment 1 Kartikeya Sharma 2017-02-06 17:22:41 UTC
Created attachment 345044 [details] [review]
nautilus-ui-utilities: Removed unused header file
Comment 2 Kartikeya Sharma 2017-02-06 19:02:21 UTC
Created attachment 345050 [details] [review]
nautilus-window-slot: Port from eel to glib
Comment 3 Kartikeya Sharma 2017-02-07 09:28:18 UTC
Created attachment 345088 [details] [review]
nautilus-window-slot: Port from eel to glib
Comment 4 Carlos Soriano 2017-02-07 11:55:14 UTC
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
Comment 5 Carlos Soriano 2017-02-07 12:04:02 UTC
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.
Comment 6 Kartikeya Sharma 2017-02-07 12:45:23 UTC
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.
Comment 7 Kartikeya Sharma 2017-02-07 12:49:00 UTC
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.
Comment 8 Kartikeya Sharma 2017-02-07 13:08:17 UTC
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.
Comment 9 Ernestas Kulik 2017-02-07 15:39:08 UTC
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).
Comment 10 Carlos Soriano 2017-02-07 18:54:10 UTC
(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?)
Comment 11 Ernestas Kulik 2017-02-07 19:12:52 UTC
(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.
Comment 12 Ernestas Kulik 2017-02-07 19:38:07 UTC
(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. :)
Comment 13 Kartikeya Sharma 2017-02-07 19:40:43 UTC
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.
Comment 14 Kartikeya Sharma 2017-02-07 19:48:30 UTC
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.
Comment 15 Carlos Soriano 2017-02-08 13:48:11 UTC
Review of attachment 345140 [details] [review]:

looks good, good work!
Comment 16 Carlos Soriano 2017-02-08 13:49:51 UTC
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"
Comment 17 Kartikeya Sharma 2017-02-08 13:54:26 UTC
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.
Comment 18 Carlos Soriano 2017-02-08 13:55:51 UTC
Review of attachment 345219 [details] [review]:

now it's good, congrats!!
Comment 19 Kartikeya Sharma 2017-02-11 04:58:47 UTC
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.
Comment 20 Kartikeya Sharma 2017-02-11 05:39:44 UTC
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.
Comment 21 Kartikeya Sharma 2017-02-16 16:59:12 UTC
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.
Comment 22 Kartikeya Sharma 2017-02-16 17:15:05 UTC
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.
Comment 23 Kartikeya Sharma 2017-02-16 17:32:50 UTC
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.
Comment 24 Kartikeya Sharma 2017-02-16 17:44:46 UTC
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.
Comment 25 Kartikeya Sharma 2017-02-16 17:56:22 UTC
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.
Comment 26 Kartikeya Sharma 2017-02-16 18:01:54 UTC
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.
Comment 27 Kartikeya Sharma 2017-02-17 16:15:33 UTC
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.
Comment 28 Kartikeya Sharma 2017-02-17 16:22:29 UTC
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.
Comment 29 Kartikeya Sharma 2017-02-17 16:27:04 UTC
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.
Comment 30 Kartikeya Sharma 2017-02-17 16:31:45 UTC
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.
Comment 31 Kartikeya Sharma 2017-02-17 16:41:58 UTC
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.
Comment 32 Kartikeya Sharma 2017-02-17 16:46:08 UTC
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.
Comment 33 Kartikeya Sharma 2017-02-17 16:51:35 UTC
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.
Comment 34 Kartikeya Sharma 2017-02-17 16:57:25 UTC
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.
Comment 35 Kartikeya Sharma 2017-02-17 17:00:19 UTC
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.
Comment 36 Kartikeya Sharma 2017-02-21 13:35:32 UTC
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.
Comment 37 Carlos Soriano 2017-02-23 15:54:18 UTC
Review of attachment 345507 [details] [review]:

LGTM
Comment 38 Carlos Soriano 2017-02-23 16:05:42 UTC
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.
Comment 39 Kartikeya Sharma 2017-03-04 15:14:21 UTC
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.
Comment 40 Kartikeya Sharma 2017-03-04 15:30:05 UTC
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.
Comment 41 Kartikeya Sharma 2017-03-04 15:36:00 UTC
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.
Comment 42 Carlos Soriano 2017-03-06 12:08:38 UTC
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.
Comment 43 Kartikeya Sharma 2017-03-06 13:58:04 UTC
(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.
Comment 44 Kartikeya Sharma 2017-03-06 14:58:03 UTC
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.
Comment 45 Carlos Soriano 2017-03-07 10:33:52 UTC
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.
Comment 46 Kartikeya Sharma 2017-03-07 13:39:35 UTC
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.
Comment 47 Carlos Soriano 2017-03-08 16:03:21 UTC
Review of attachment 345968 [details] [review]:

let's reject since the intention is to go one by one. The code can still be consulted.
Comment 48 Carlos Soriano 2017-03-08 16:03:37 UTC
Review of attachment 345975 [details] [review]:

let's reject since the intention is to go one by one. The code can still be consulted.
Comment 49 Carlos Soriano 2017-03-08 16:03:53 UTC
Review of attachment 345985 [details] [review]:

let's reject since the intention is to go one by one. The code can still be consulted.
Comment 50 Carlos Soriano 2017-03-08 16:04:10 UTC
Review of attachment 346082 [details] [review]:

let's reject since the intention is to go one by one. The code can still be consulted.
Comment 51 Carlos Soriano 2017-03-08 16:04:48 UTC
Review of attachment 346084 [details] [review]:

let's reject since the intention is to go one by one. The code can still be consulted.
Comment 52 Carlos Soriano 2017-03-08 16:05:07 UTC
Review of attachment 346085 [details] [review]:

let's reject since the intention is to go one by one. The code can still be consulted.
Comment 53 Carlos Soriano 2017-03-08 16:05:22 UTC
Review of attachment 346089 [details] [review]:

let's reject since the intention is to go one by one. The code can still be consulted.
Comment 54 Carlos Soriano 2017-03-08 16:05:36 UTC
Review of attachment 346091 [details] [review]:

let's reject since the intention is to go one by one. The code can still be consulted.
Comment 55 Carlos Soriano 2017-03-08 16:06:25 UTC
Review of attachment 347392 [details] [review]:

Excellent, congrats!!
Comment 56 Carlos Soriano 2017-03-08 16:18:21 UTC
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
Comment 57 Kartikeya Sharma 2017-03-08 19:15:24 UTC
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.
Comment 58 Carlos Soriano 2017-03-27 18:40:53 UTC
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?
Comment 59 Kartikeya Sharma 2017-03-28 19:29:21 UTC
(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.
Comment 60 Stanca Robert 2017-04-01 22:32:13 UTC
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.
Comment 61 Carlos Soriano 2017-04-20 16:06:02 UTC
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.
Comment 62 António Fernandes 2018-02-07 08:42:46 UTC
Hello, Kartikeya Sharma, Stanca Robert, can you update the patches according to the review?
Comment 63 André Klapper 2021-06-18 15:52:38 UTC
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.