Created attachment 316917 [details] [review]
EphyEncodingDialog: Use a GtkSwitch.

Review of attachment 316916 [details] [review]:

I think it's fine to do this, but if so, we should reload the page with the new encoding when closing the dialog (unless it has already happened in response to a click), otherwise you're out of luck if you want to change the encoding using only a keyboard.

::: src/ephy-encoding-dialog.c
@@ -293,3 @@
 	activate_choice (dialog);
-
-	g_object_unref (dialog);

Hm, I also do not see why this was present; it looks like a bug to me. But I am confused why the dialog did not crash previously.

Does your new patch introduce a crash when this line is not removed?

Review of attachment 316917 [details] [review]:

I see a warning when clicking the toggle:

** (epiphany:4478): CRITICAL **: activate_choice: assertion 'EPHY_IS_EMBED (dialog->embed)' failed

::: src/ephy-encoding-dialog.c
@@ +275,3 @@
 		activate_choice (dialog);
 	}
+	// TODO select safe default in list, or find another solution

ISO-8859-1

(In reply to Michael Catanzaro from comment #2)
> I think it's fine to do this, but if so, we should reload the page with the
> new encoding when closing the dialog (unless it has already happened in
> response to a click), otherwise you're out of luck if you want to change the
> encoding using only a keyboard.

You can always activate with keyboard with “enter”. I even think it’s more natural than to have the page reload each time you click an “array” key.

> Hm, I also do not see why this was present; it looks like a bug to me. But I
> am confused why the dialog did not crash previously.

I think it is because the work was done in the selection function, blocking the one of the activation one.

> Does your new patch introduce a crash when this line is not removed?

Yeps.

Created attachment 316925 [details] [review]
EphyEncodingDialog: Use a GtkSwitch.

My first implementation of the GtkSwitch was a bit stupi^Wnaive, and that caused the warnings you saw. Here is a more correct patch.

About the default encoding to select in the list, I agree, but that will be another story (no regression there).

Created attachment 317052 [details] [review]
EphyEncodingDialog: Rework.

So. This patch is ugly, it does more than two^Wtwenty things, but it solves (I think) most of the problem related to the EphyEncodingDialog class. I’m presenting it for getting advices, tracking bugs, etc. If someone has an idea on how to split it, it might be a thing to do also…

Review of attachment 317052 [details] [review]:

Accepted after comments.

::: src/ephy-encoding-dialog.c
@@ +49,3 @@
 	/* from the UI file */
+	GtkStack   *type_stack;
+	GtkSwitch  *default_switch;

I prefer not to align these.

@@ +308,3 @@
+
+static void
+free_row_user_data (gpointer user_data)

As Carlos would say, we don't need a function just to call g_free, so please remove this and use g_free directly.

But, why is this correct? Is not the user data the EphyEncodingRow? Then it should be freed with g_object_unref, no?

@@ +340,3 @@
+sort_encodings (gconstpointer a, gconstpointer b)
+{
+	EphyEncoding *enc1 = (EphyEncoding*)a;

(EphyEncoding *)

@@ +386,3 @@
+	WebKitWebView *view;
+	EphyEncoding *enc_node;
+	EphyLanguageGroup groups;	// int

No need for this comment.

@@ +387,3 @@
+	EphyEncoding *enc_node;
+	EphyLanguageGroup groups;	// int
+	GList *recent, *related = NULL;

Put these on separate lines.

@@ +401,3 @@
+	if (recent != NULL)
+	{
+		recent = g_list_sort (recent, (GCompareFunc) sort_encodings);

No space after the cast.

@@ +402,3 @@
+	{
+		recent = g_list_sort (recent, (GCompareFunc) sort_encodings);
+		g_list_foreach (recent, (GFunc) add_list_item, dialog->recent_list_box);

Same.

::: src/ephy-encoding-row.c
@@ +34,3 @@
+
+	/* from the UI file */
+	GtkLabel     *encoding_label;

I prefer not to add extra spaces before the asterisks except in parameter lists.

@@ +126,3 @@
+	object_class->get_property = ephy_encoding_row_get_property;
+
+	g_object_class_install_property (object_class,

Please instead keep a static array of property IDs, and use g_object_class_install_properties once at the bottom of class_init instead. Look at any of my recent "use g_object_class_install_properties" commits as an example. The advantage of this approach is that we can refer to properties within the class using a constant rather than a string literal. (There is also a performance advantage to doing that, not that it would matter for the encodings dialog.)

@@ +132,3 @@
+	                                                      "encoding",
+	                                                      EPHY_TYPE_ENCODING,
+	                                                      G_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY | G_PARAM_STATIC_NAME | G_PARAM_STATIC_NICK | G_PARAM_STATIC_BLURB));

G_PARAM_STATIC_STRINGS

::: src/window-commands.c
@@ +1699,3 @@
+			      EphyWindow *window)
+{
+	EphyEncodingDialog* dialog;

EphyEncodingDialog *dialog;

Commited, thanks. =)