Created attachment 246648 [details] [review]
Enable/disable the caret navigation with F7

Review of attachment 246648 [details] [review]:

Ok, I think caret navigation is stable enough now to allow users to enable it :-)

::: data/org.gnome.Evince.gschema.xml.in
@@ +31,3 @@
       <_description>The maximum size that will be used to cache rendered pages, limits maximum zoom level.</_description>
     </key>
+    <key name="caret-navigation" type="b">

Do not use gsettings for this, I think this should be per document, so use the metadata.

@@ +34,3 @@
+      <default>false</default>
+    </key>
+    <key name="caret-navigation-show-msg" type="b">

show-caret-navigation-message, maybe?. Also please add a summary and/or description explaining what this is for

::: libview/ev-view.c
@@ +3135,3 @@
 
+void
+ev_view_toggle_caret_navigation_enabled (EvView *view)

I don't think we need this, ev_view_set_caret_navigation_enabled and ev_view_get_caret_navigation_enabled should be enough for users

::: shell/ev-window.c
@@ +223,3 @@
 
+	/* Caret navigation */
+	gboolean caret_show_msg;

I don't think we need to save this, we can just use the gsettings to ask for the value when needed.

@@ +367,3 @@
 							 gpointer          user_data);
+static void     ev_window_cmd_view_toggle_caret_navigation (GtkAction     *action,
+							 EvWindow         *window);

Move the implementation before the accelerators declaration and we don't need another prototype.

@@ +1408,2 @@
         ev_window_ensure_settings (ev_window);
+	ev_window->priv->caret_show_msg = g_settings_get_boolean (ev_window->priv->settings, "caret-navigation-show-msg");

Don't need to cache this, this is only needed when the user presses F7

@@ +1409,3 @@
+	ev_window->priv->caret_show_msg = g_settings_get_boolean (ev_window->priv->settings, "caret-navigation-show-msg");
+	ev_view_set_caret_navigation_enabled (EV_VIEW (ev_window->priv->view),
+					      g_settings_get_boolean (ev_window->priv->settings, "caret-navigation"));

This should be saved in the document metadata, together with the caret position (page + offset). You can leave this for a follow up patch, though.

@@ +6956,3 @@
+	priv->caret_show_msg = !gtk_toggle_button_get_active (button);
+	g_settings_set_boolean (priv->settings, "caret-navigation-show-msg", priv->caret_show_msg);
+	g_settings_apply (priv->settings);

We don't need to connect to the toggle signal of the check button, save the value in the message area response callback once the user has decided.

@@ +6970,3 @@
+			       "This feature places a moveable cursor in text "
+			       "pages, and allows you to select text with the "
+			       "keyboard.");

This should be const. I don't think this should be an info message, but a question, and the user can cancel it as well.

@@ +6977,3 @@
+	/*  Enable/disable the caret navigation */
+	settings = ev_window_ensure_settings (window);
+	ev_view_toggle_caret_navigation_enabled (view);

This should be done in the response depending on whether the user accepted it or not.

@@ +6986,3 @@
+	/* Show informative dialog */
+	hbox = gtk_box_new (GTK_ORIENTATION_HORIZONTAL, 12);
+	check_button = gtk_check_button_new_with_label (_("Don't show this again"));

"Don't show this message again" or even "Don't ask this again" or something like that.

@@ +6990,3 @@
+	gtk_widget_show_all (hbox);
+
+	area = ev_message_area_new (GTK_MESSAGE_INFO, message, GTK_STOCK_CLOSE, GTK_RESPONSE_CLOSE, NULL);

I think the message here is actually the secondary text, the main message should be the question, something like: "Enable caret navigation mode?" and two buttons "Cancel" "Enable"

@@ +7559,3 @@
+
+	/* Caret navigation */
+	ev_window->priv->caret_show_msg = TRUE;

We don't need to initialize this, use the gsettings default value.

@@ +7560,3 @@
+	/* Caret navigation */
+	ev_window->priv->caret_show_msg = TRUE;
+	if (ev_window->priv->settings) {

We don't need to check this, you can use ev_window_ensure_settings to make sure settings are intialized.

Created attachment 250056 [details] [review]
Turn the caret navigation on/off using F7

Patch updated according to comments in comment 2

Review of attachment 250056 [details] [review]:

I think we also need a way to know whether the current document supports caret navigation. Something like ev_view_supports_caret_navigation() that checks if the backend of the current document implements get_text and get_text_layout

::: shell/ev-window.c
@@ +1263,3 @@
+	caret_navigation = FALSE;
+	ev_metadata_get_boolean (window->priv->metadata, "caret-navigation-mode", &caret_navigation);
+	ev_view_set_caret_navigation_enabled (EV_VIEW (window->priv->view), caret_navigation);

Instead of this you could check return value of ev_metadata_get_boolean, caret_navigation will always be initialized when it returns TRUE

if (ev_metadata_get_boolean (window->priv->metadata, "caret-navigation-mode", &caret_navigation))
        ev_view_set_caret_navigation_enabled (EV_VIEW (window->priv->view), caret_navigation);

The key could be called just caret-navigation, without the -mode.

@@ +5565,3 @@
+			gboolean active;
+
+			active = gtk_toggle_button_get_active (GTK_TOGGLE_BUTTON (grandchildren->data));

This is too much complexity to get the toggle button. Just save it in the window private struct when it's created and use it here directly instead of iterating children and grandchildren of the message area.

@@ +5592,3 @@
+					"This feature places a moveable cursor in text "
+					"pages, allowing you to move around and select text with your "
+					"keyboard. Do you want to enable the caret navigation on?");

These variables are not reused, so I think it will be easier tot read if they are used directly in ev_message_area_new and ev_message_area_set_secondary_text

@@ +5602,3 @@
+		if (window->priv->metadata && !ev_window_is_empty (window))
+			ev_metadata_set_boolean (window->priv->metadata, "caret-navigation-mode", !caret_enabled);
+		ev_view_set_caret_navigation_enabled (view, !caret_enabled);

This could be factored out into a helper function ev_window_set_caret_navigation_enabled() and called also from the message are response callback.

@@ +5617,3 @@
+	ev_message_area_set_secondary_text (EV_MESSAGE_AREA (message_area), secondary_text);
+
+	check_button = gtk_check_button_new_with_label (_("Don't show this message again"));

Save this in the window private struct and use it in the response callback directly.

Created attachment 250196 [details] [review]
Turn the caret navigation on/off pressing F7

Patch updated according to comments in comment 4

Review of attachment 250196 [details] [review]:

Split in two patches, fixed some remaining issues I mention below and pushed to git master. Thanks!

::: shell/ev-window.c
@@ +233,3 @@
+
+	/* Caret navigation */
+	GtkWidget *show_message_area_again;

This sounds too generic, we use the message area for more things.

@@ +663,2 @@
 		gtk_widget_destroy (window->priv->message_area);
+		window->priv->show_message_area_again = NULL;

I think it's better to do this only for the caret navigation message area.

@@ +1266,3 @@
+
+	/* Caret navigation mode */
+	if (ev_view_supports_caret_navigation (EV_VIEW (window->priv->view)) &&

This is not enough, we should disable the F7 action when caret navigation is not supported, to make sure the ev_window_cmd_view_toggle_caret_navigation callback is never called when unsupported.

@@ +5551,3 @@
+					gboolean enabled)
+{
+	if (window->priv->metadata && !ev_window_is_empty (window))

If we disable the F7 action when caret navigation is unsupported, the window can't be empty here.

@@ +5568,3 @@
+	/* Turn the confirmation dialog off if the user has requested not to show it again */
+	if (window->priv->show_message_area_again &&
+	    gtk_toggle_button_get_active (GTK_TOGGLE_BUTTON (window->priv->show_message_area_again))) {

window->priv->show_message_area_again can't be NULL at this point

@@ +5599,3 @@
+	message_area = ev_message_area_new (GTK_MESSAGE_QUESTION, _("Enable caret navigation mode?"),
+					    GTK_STOCK_YES, GTK_RESPONSE_YES,
+					    GTK_STOCK_NO,  GTK_RESPONSE_NO,

Order should be the opposite, and we could use Enable instead of Yes.