Created attachment 263052 [details] [review]
Adding atk dependency 

Some of the stuff implemented is available since atk 2.11.3, released today, so a dependency to atk is in place.

Created attachment 263053 [details] [review]
Implement AtkDocument on EvViewAccessible

It adds the implementation of AtkDocument on EvViewAccessible and implements some of the methods. Specifically all related with page numbering. This includes:
 * get current page 
 * get total page count
 * emit a page-changed signal each time a page changes while moving with the caret

The only missing thing that could make sense is exposing the locale of the accessible object.

Review of attachment 263052 [details] [review]:

Would it be possible to use ATK_CHECK_VERSION, or whatever version check ATK has instead of bumping the requirements?

Review of attachment 263053 [details] [review]:

Looks good, but I prefer if this is done conditionally depending on the ATK version instead of bumping requirements.

::: libview/ev-view-accessible.c
@@ +868,3 @@
+	ev_document = ev_document_model_get_document (priv->model);
+
+	return ev_document_get_n_pages (ev_document);

Is this guaranteed to be called after ev_view_accessible_set_model? Note that you can create a view without a model and set a model afterwards. We can add an early return or and assert if it's not expected here when document is NULL.

@@ +882,3 @@
+		return -1;
+
+	//+1 as user start to count on 1, but evince starts on 0

Please, don't use C++ style comments. user start -> user starts

@@ +1137,3 @@
 		priv->previous_cursor_page = page;
 		clear_cache (accessible);
+		//+1 as user start to count on 1, but evince starts on 0

Ditto.

@@ +1138,3 @@
 		clear_cache (accessible);
+		//+1 as user start to count on 1, but evince starts on 0
+		g_signal_emit_by_name (accessible, "page-changed", page + 1);

This will only work when caret navigation is enabled, I don't know whether it's on purpose or not. You should connect to page-changed signal of the document model to make sure you always emit the corresponding ATK signal when caret navigation is not enabled. I understand that when caret navigation is enabled, what you always want is the cursor page.

Created attachment 263120 [details] [review]
Implement AtkDocument on EvViewAccessible

(In reply to comment #4)
> Review of attachment 263053 [details] [review]:
> 
> Looks good, but I prefer if this is done conditionally depending on the ATK
> version instead of bumping requirements.

I used ATK_CHECK_VERSION to guard the implementation of get_page_count, get_current_page_number and the emission of page-changed.

I maintained the interface implementation, so the accessible is exposed as being of type document (something that makes sense) and also would allow to implement the rest (and old) AtkDocument methods if needed.

> ::: libview/ev-view-accessible.c
> @@ +868,3 @@
> +    ev_document = ev_document_model_get_document (priv->model);
> +
> +    return ev_document_get_n_pages (ev_document);
> 
> Is this guaranteed to be called after ev_view_accessible_set_model? Note that
> you can create a view without a model and set a model afterwards. We can add an
> early return or and assert if it's not expected here when document is NULL.

Added a NULL check on ev_document. -1 is returned in that case. -1 is the value used when for any reason that information is not available.

> @@ +882,3 @@
> +        return -1;
> +
> +    //+1 as user start to count on 1, but evince starts on 0
> 
> Please, don't use C++ style comments. user start -> user starts

Solved.


> @@ +1137,3 @@
>          priv->previous_cursor_page = page;
>          clear_cache (accessible);
> +        //+1 as user start to count on 1, but evince starts on 0
> 
> Ditto.

Solved, and also on the big explanation of why we emit page-reload and load-complete in a row.


> @@ +1138,3 @@
>          clear_cache (accessible);
> +        //+1 as user start to count on 1, but evince starts on 0
> +        g_signal_emit_by_name (accessible, "page-changed", page + 1);
> 
> This will only work when caret navigation is enabled, I don't know whether it's
> on purpose or not. You should connect to page-changed signal of the document
> model to make sure you always emit the corresponding ATK signal when caret
> navigation is not enabled. 

We already had the callback to that signal, so I emit page-changed only if caret navigation is not enabled, to avoid double emissions if it is enabled. I don't do the check on the caret-move callback because that only happens when the caret moves.

> I understand that when caret navigation is enabled,
> what you always want is the cursor page.

Yes. That "page+1" is "cursor_page + 1" in that context.

Review of attachment 263120 [details] [review]:

Great, thanks!

::: libview/ev-view-accessible.c
@@ +1190,3 @@
+	   signal. We emit both in a row, as usual ATs uses reload to
+	   know that current content has changed, and load-complete to
+	   know that the content is already available. */

Please, use the format 
/*
 * 
 *
 */

Comment on attachment 263120 [details] [review]
Implement AtkDocument on EvViewAccessible

(In reply to comment #6)
> Review of attachment 263120 [details] [review]:
> 
> Great, thanks!
> 
> ::: libview/ev-view-accessible.c
> @@ +1190,3 @@
> +       signal. We emit both in a row, as usual ATs uses reload to
> +       know that current content has changed, and load-complete to
> +       know that the content is already available. */
> 
> Please, use the format 
> /*
>  * 
>  *
>  */

Committed after doing this change. Thanks.