GNOME Bugzilla – Bug 724965
Implement a means to get the accessible text of the next/previous page
Last modified: 2014-06-10 17:02:29 UTC
Steps to reproduce: 1. Load a non-tagged multipage PDF with text in Evince 2. Launch Accerciser 3. Use Accerciser's Interface Viewer to examine for the document frame: a. the content shown under "Text" b. the Relations Actual Results: For step 3a, the only accessible text exposed is the content of the current page. For step 3b, there are no accessible relations shown. As a result, Orca's "SayAll" command can only present the current page; not all of the text. Expected Results: There would be some means to get the content. Exactly what those means are remains to be determined. Possibilities that spring to mind: 1. An accessible relationship (flows-to, flows-from) added to the accessible document frame. 2. New API added to AtkDocument to get the next/previous page (i.e. not the page number; the actual accessible page). The problem with both approaches seems to be that they would require each page to have its own accessible document frame -- something which is not currently the case. Right now, the same accessible document frame's text gets updated when the page changes.
After some analysis of the code, here it is my conclusions. (In reply to comment #0) > The problem with both approaches seems to be that they would require each page > to have its own accessible document frame -- something which is not currently > the case. Right now, the same accessible document frame's text gets updated > when the page changes. I think that the solution to this would be keeping EvView accessible object as just the document frame, but without content. After all, that is what EvView is, a frame for all the document pages. And when I mean "just" I mean that text would not come from EvViewAccessible. The text would be contained on individual accessible objects that represents each page (so with the role ATK_ROLE_PAGE)). Initially I thought that one solution could be create accessible objects from each EvPage, as it represents each individual page, and it would be just implement the usual object+accessible-proxy. But those pages are mostly transient, just used to render the pages, created during that process and unreffed after that. But the information of the page, specifically the information that we need, are stored at EvPageCache. In fact most of the methods at EvViewAccessible rely on EvPageCache specifying the current page. So the idea would be having individual accessible objects for each page, and instead of using the current page, each of those objects would use the page number they represent. It would be needed to be careful with that if we want to access to any given page, as the cache (as any cache) doesn't need to have the data for all the pages at any given moment. We would need to call ev_page_cache_schedule_job_if_needed to ensure that the info is updated. As this method is not public, we would need to call it indirectly, by calling ev_page_cache_set_page_range. So after this background, the idea would be: * Create a new accessible object (a name could be EvPageAccessible) with role ATK_ROLE_PAGE * EvViewAccessible would stop to implement ATK_TEXT. All their text related code would be moved to EvPageAccessible * EvViewAccessible will now expose those pages as children * I don't see the need for a new API for AtkDocument. If you want a specific page, you can just ask the specific child. * I don't see the need to create those pages beforehand. Can be created when someone (ATs) ask for a specific page. But this would mean that we will not have the relation (at least always), as at some point we could have one page, but not the prev/following one. * EvViewAccessible would still need to manage some of the cursor stuff, as is EvView which emits cursor-moved. EvViewAccessible would connect to it, and the callback would find the specific EvPageAccessible and ask it to emit the proper signal. But probably I would need the feedback from a seasoned Evince developer to be sure that all this makes sense.
Created attachment 271885 [details] [review] a11y: new EvPageAccessible Instead of just one big patch, Im splitting this on several patches, to make easier the work of the reviewer. But probably, when pushing, it would be better just one commit. This patch just add the minimum to have a compiling EvPageAccessible
Created attachment 271888 [details] [review] a11y: expose EvPageAccessible as children of EvViewAccessible Now EvViewAccessible has children, as many as pages. This is an straightforward patch. When created (or the model changes), it creates a GPtrArray with all the individual EvPageAccessible. It could be possible to do this under demand (so create the array but don't create the page accessibles yet). In any case, I didn't notice a really big difference on load time with this approach (even with a big pdf like the pdf-spec, with 756 pages)
Created attachment 271889 [details] [review] a11y: move AtkText implementation from EvViewAccessible to EvPageAccessible This is an straightforward move of the AtkText implemention of EvViewAccessible to EvPageAccessible. Some notes: * Bug 725003 is modifying that AtkText implementation, but the update would be easy. In the same way, I prefered to upload this, just in case someone (like Joanie) wanted to test this * I nuked get_selection, remove_selection and add_selection. Previous implementation was doing that on the internal GtkTextBuffer, so was just conceptually wrong (so not working). Moving it to EvPageAccessible was somewhat worthless to me.
Created attachment 271890 [details] [review] a11y: move AtkHyperText implementation from EvViewAccessible to EvPageAccessible Straightforward move of the AtkHypertext implementation from EvViewAccessible to EvPageAccessible. The only thing to comment is that I also needed to update ev-link-accessible
Created attachment 271892 [details] [review] a11y: add if applies ATK_RELATION_FLOWS_TO/FROM on EvPageAccessible Adds the relation ATK_RELATION_FLOWS_TO/FROM as suggested on comment 0. This was trivial as the page accessible are available everytime. If we decide to get the children under demand, when asking for the relation would be one of those "demand" situations, as we need to have the accessibles to add the relations.
Created attachment 271894 [details] [review] a11y: implement ref_state_set for EvPageAccessible We consider that the state_set for any given page is the same that for EvView, but removing ATK_STATE_SHOWING if the page is not on screen.
All the previous patches adds the accessible objects for each page, but there are still one missing thing. This is the cache issues that I mentioned on comment 1. Right now the text cames from the cache, and only the pages currently viewed by EvView are cached. I tried a quick hack of adding a call to ev_page_cache_set_page_range when getting the text, but as mentioned by Carlos Garcia, this is racy, and doesn't work always. This is because when you ask to update the cache, it is not synchronous. It is done on a different thread (scheduled by EvJobScheduler). So the outcome is that we wouldn't have the text available unless we wait that job to finish. I also tried, as suggested on IRC, to try to cache all the pages during the load time. I thought that the proper place would be ev_job_load_run, but at that moment we don't have available at all the page cache. On ev-jobs we also have the code that loads an individual page, ev_job_page_data_run, but that loads the data on a EvJobs defined structure, and is EvPageCache the one that gets this information from a callback and fills itself. What I'm going to do now, is simulate loading the data of all the pages, although without filling the cache, so we could have an estimation of the time cost. If that is assumable, then we can think on how to fill the cache with that information.
Created attachment 271933 [details] [review] Hack to estimate time cost for filling all the cache at load-document time This hacks copy function ev_job_page_data_run, used to fill under-demand the data for a given page, and call it during ev_job_load_run for all the pages of the document. I assume that filling all the cache would do something similar to this (for example, EvDocument doesn't have an API to retrieve more than one page at once). I tested it with some documents that I had in hand: PDF-Spec-2008 (all text, 756pages) Anima (tabletop game) small manual (text + images, 144pages) Anima (tabletop game) core manual (text + images, 346pages) A song of ice and fire roleplaying (tabletop) manual (all images, 224pages) Manual Fate (tabletop game) Core (text+images, 310 pages) And compiled evince with --enable-debug in order to be able to get the stats from the profiler (setting the envvar EV_PROFILE=1. Without this patch, the outcome for the previous documents are: [ EvJobLoad (0x287b8f0) ] 0.078572 s elapsed [ EvJobLoad (0x24b14f0) ] 0.150105 s elapsed [ EvJobLoad (0x28f54f0) ] 0.080548 s elapsed [ EvJobLoad (0x1f2bcf0) ] 0.136633 s elapsed [ EvJobLoad (0x1b27cf0) ] 0.177917 s elapsed With this patch, asking to get all the information from the pages (so EV_PAGE_DATA_INCLUDE_ALL): [ EvJobLoad (0xb56c50) ] 4.033755 s elapsed [ EvJobLoad (0x2582450) ] 2.060964 s elapsed [ EvJobLoad (0x153f050) ] 6.352579 s elapsed [ EvJobLoad (0x26f3050) ] 0.105226 s elapsed [ EvJobLoad (0xe16450) ] 2.777440 s elapsed I guessed if asking everything was an overkill, so I asked then for just the text (EV_PAGE_DATA_INCLUDE_TEXT) [ EvJobLoad (0x280d050) ] 3.880278 s elapsed [ EvJobLoad (0xb1b050) ] 2.062584 s elapsed [ EvJobLoad (0x1975050) ] 6.167668 s elapsed [ EvJobLoad (0x1ab1050) ] 0.103692 s elapsed [ EvJobLoad (0x199f190) ] 2.767253 s elapsed The difference between _ALL and _TEXT is small. But the difference between getting and not getting is big (IMHO), as for the bigger documents, you get a 5secs loading spinner. So unless the hack is wrong at something, and there is a better way to fill the cache, my plan would be working on getting updated the cache under demand, when the accessible object needs it.
Created attachment 272142 [details] [review] a11y: ensure text is cached when you request an EvPage As I mentioned, when I tried to update the cache on the get_text (or similar) was problematic because that is an async process, so get_text would fail to return the text. What this patch does is asking to update the cache when the accessible is referenced, assuming that if you ask for a page, eventually you would ask for the text in a following call. As far a I tested with Accerciser, this get the problem solved.
Created attachment 272153 [details] [review] a11y: move AtkText implementation from EvViewAccessible to EvPageAccessible Update after changes on bug 725003
Created attachment 272154 [details] [review] a11y: move AtkHyperText implementation from EvViewAccessible to EvPageAccessible Update after bug 725003
Created attachment 272155 [details] [review] a11y: ensure text is cached when you request an EvPage Reuploaded, in order to have all the patch file names in order (0001, 0002, etc)
Created attachment 272191 [details] [review] a11y: missing cleaning data On finalize EvViewAccessible needs to clean their children. That was also happening before, because Evince sets the model to NULL when cleaning, and set_model also cleans it. But it would be better to cleaning that always on finalize just in case. Also unrefs a temporal state_set used on EvPageAccessible->ref_state_set
Created attachment 272192 [details] [review] a11y: emit text selection changed from the page instead of the view As detected by Joanmarie, text selection was still being emitted from the view
Created attachment 272200 [details] [review] a11y: notify focus changes on EvPageAccessible
These are working for me / Orca. SayAll is unbroken. Thanks!! :) The one remaining and minor issue is that when you first get into a document or Alt+Tab back into one, and the page hasn't changed and the caret hasn't moved, the accessible Document Frame claims focus; the showing page does not. If this is not something we can sanely emit in Evince, I can deal with it in Orca.
Review of attachment 272155 [details] [review]: ::: libview/ev-view-accessible.c @@ +151,3 @@ + * modifying the cache range */ + ev_page_cache_set_page_range (page_cache, i, i); + if (page_cache) { I've got another use case for this: When you jump directly to a specified page using caret navigation (but not necessarily a11y support), there's an excellent chance we haven't cached that page. The caret navigation support sometimes positions the caret based on the pango log attributes associated with that page. (And it fails when the log attrs aren't there yet.)
(In reply to comment #18) > Review of attachment 272155 [details] [review]: > > ::: libview/ev-view-accessible.c > @@ +151,3 @@ > + * modifying the cache range */ > + ev_page_cache_set_page_range (page_cache, i, i); > + if (page_cache) { > > I've got another use case for this: When you jump directly to a specified page > using caret navigation (but not necessarily a11y support), there's an excellent > chance we haven't cached that page. I think that I'm not understanding this. Is what you found a new problem that happens with the patches at this bug? If you are jumping using caret navigation, you are using the usual ev-view support, that sets the range there, not using that specific call at ev-page-accesssible. So, unless I'm missing something, that problem shouldn't arise due the new EvPageAccessible object.
(In reply to comment #19) > Is what you found a new problem that > happens with the patches at this bug? No. > If you are jumping using caret > navigation, you are using the usual ev-view support, I know. > not using that specific call at ev-page-accesssible. I know. > that problem shouldn't arise due the new EvPageAccessible object. I never said it did. But when I saw your comment and call to ev_page_cache_set_page_range, I tried it in the code/problem I'm trying to fix (aka not this bug). And it largely fixed the problem there too. There are, thus, more than one use case/need for what you did. So as you commented in your patch, this might suggest we want API to cache a specified page upon demand. Sorry for being unclear. I hope this helps clarify things.
(In reply to comment #20) > But when I saw your comment and call to ev_page_cache_set_page_range, I tried > it in the code/problem I'm trying to fix (aka not this bug). And it largely > fixed the problem there too. There are, thus, more than one use case/need for > what you did. So as you commented in your patch, this might suggest we want API > to cache a specified page upon demand. Hmm, that is strange, because right now, ev_page_cache_set_page_range is also doing a pre_cache of the next and previous page: https://git.gnome.org/browse/evince/tree/libview/ev-page-cache.c#n367 So each time you move to a page, ev-view sets the range for that one, and in theory next and previous page should be also cached. > Sorry for being unclear. I hope this helps clarify things. Yes, thanks for the explanation.
(In reply to comment #21) > So each time you move to a page, ev-view sets the range for that one, and in > theory next and previous page should be also cached. That assumes page-by-page navigation; not a direct jump to a specified page. (But we're getting off track -- totally my fault.) Re-focusing on this bug here, the one relevant comment/question from me is the following: (In reply to comment #17) > These are working for me / Orca. SayAll is unbroken. Thanks!! :) > > The one remaining and minor issue is that when you first get into a document or > Alt+Tab back into one, and the page hasn't changed and the caret hasn't moved, > the accessible Document Frame claims focus; the showing page does not. If this > is not something we can sanely emit in Evince, I can deal with it in Orca. Do you think you can solve this within Evince as part of the fix for this bug? Or shall I make a change in Orca?
(In reply to comment #22) > (In reply to comment #17) > > These are working for me / Orca. SayAll is unbroken. Thanks!! :) > > > > The one remaining and minor issue is that when you first get into a document or > > Alt+Tab back into one, and the page hasn't changed and the caret hasn't moved, > > the accessible Document Frame claims focus; the showing page does not. If this > > is not something we can sanely emit in Evince, I can deal with it in Orca. > > Do you think you can solve this within Evince as part of the fix for this bug? > Or shall I make a change in Orca? Carlos Garcia mentioned that he would try to review this bug. And I think that will the patches (10!) that are already here he had plenty of stuff to review. So if Orca can deal with it for now, I think that it would be better to keep this as it is, without adding more patches.
Review of attachment 271888 [details] [review]: Looks good in general, I have just a few comments ::: libview/ev-view-accessible.c @@ +144,3 @@ + ev_document = ev_document_model_get_document (priv->model); + + return ev_document == NULL ? -1 : ev_document_get_n_pages (ev_document); Now that we have an array of children, we can simply this, but checking if children != NULL and return the array len. @@ +147,3 @@ +} + +static AtkObject* AtkObject* -> AtkObject * @@ +160,3 @@ + child = g_ptr_array_index (self->priv->children, i); + + return g_object_ref (child); You don't need the child local variable @@ +1169,2 @@ static void +clear_children (EvViewAccessible *self) I guess you should call this in the finalize method too. @@ +1189,3 @@ + gint i; + EvPageAccessible *child; + gint n_pages = ev_view_accessible_get_n_pages (self); You should use the document here. @@ +1192,3 @@ + + if (self->priv->children) + clear_children (self); This should be cleared already at this point. @@ +1216,2 @@ clear_cache (accessible); + initialize_children (accessible); I think we should call clear_children before the early return. @@ +1274,3 @@ ev_view_accessible_set_model (EV_VIEW_ACCESSIBLE (accessible), view->model); + initialize_children (EV_VIEW_ACCESSIBLE (accessible)); This shouldn't be needed, initialize_children will be called in document_changed_cb, we only want to call initialize_children only when we have a valid document.
Created attachment 272292 [details] Debug file a new Gedit related testcase Hi Joanie, Your fix works good the two I previous wrote testcases, both Libreoffice Writer and Gedit applications. I have got a little bad news: Unfortunately need examining few selection related issues before we closing this bug report with resolved, fixed state. First very interesting issue: I found an interesting issue with affecting last line deselecting related task, little modified testcase for Gedit: 1. Type the apple, cheese and potato words, separated with ENTER key. Please not press ENTER key after potato word, this is important now. 2. Press CTRL+HOME keystrokes, and select all lines with SHIFT+DOWN arrow keystrokes. This steps produce the expected result, Orca correct spokening the actual selected line and selected state message. 3. Press one SHIFT+UP keystroke. This step not producing the expected result. Expected result: Orca need announce the potato related line and not selected state message. Actual result: Orca announce cheese and potato lines, and announce the not selected state message. Me suspicious this issue, because if I this situation press CTRL+C keystroke, only the apple related line copyed to clipboard. So, without Orca you please verify this test visually to we absolute sure known what the problem this situation. Possible have big chance this bug a Gedit related bug, I using Gedit 3.10.3 version. Very interesting, if have a newline character after last potato word related line, and I doing the first Gedit related testcase, all works fine. After the potato line is selected when I press SHIFT+UP keystroke, Orca wonderful spokening the potato line and not selected state message. If I this situation press CTRL+C keystroke, the apple and cheese words wonderful copying to clipboard. Second issue with Libreoffice Writer related, this is interesting too, but I am not full sure this is an issue with Orca. Testcase: 1. In Libreoffice Writer please type the apple, cheese and potato words, separated with ENTER key. After potato word please not type a newline character. 2. Try selecting both three lines after CTRL+HOME key combination. The first two lines in Libreoffice Writer this situation the selection happening right, but the last line impossible to select. When I pressing third time the SHIFT+DOWN key combination, nothing happening. Possible this is a bug with Libreoffice Writer, need visually check this issue without Orca. I attaching now a debug.out file with the modified Gedit related testcase. If this remaining issues not related with Orca redesigned selection announcement related code parts, feel free you close this report with resolved, fixed state. Tomorrow I will be examining the basic paragraphs related selection and normal word navigation related selection. Short time I tested the CTRL+SHIFT+LEFT and CTRL+SHIFT+RIGHT keystrokes, I not experienced any problems. In Libreoffice Writer I tested the CTRL+SHIFT+UP and CTRL+SHIFT+DOWN keystrokes (select or deselect paragraphs), this part works good. Attila
Attila this is the wrong bug. This bug here is Evince related and not an Orca bug.
Created attachment 272303 [details] [review] a11y: expose EvPageAccessible as children of EvViewAccessible (In reply to comment #24) > Review of attachment 271888 [details] [review]: Updated after this review. > +clear_children (EvViewAccessible *self) > > I guess you should call this in the finalize method too. I made that in a following patch "a11y: missing cleaning data", but it is true that fits here. I will do that, add another cleaning thing on his place, and remove that patch.
Created attachment 272304 [details] [review] a11y: move AtkText implementation from EvViewAccessible to EvPageAccessible
Created attachment 272305 [details] [review] a11y: move AtkHyperText implementation from EvViewAccessible to EvPageAccessible
Created attachment 272306 [details] [review] a11y: add if applies ATK_RELATION_FLOWS_TO/FROM on EvPageAccessible
Created attachment 272307 [details] [review] a11y: implement ref_state_set for EvPageAccessible
Created attachment 272308 [details] [review] a11y: ensure text is cached when you request an EvPage
Created attachment 272309 [details] [review] a11y: emit text selection changed from the page instead of the view
Created attachment 272310 [details] [review] a11y: notify focus changes on EvPageAccessible
Note: Needed to reuploaded patches because after changing first on the chain, the rest didn't apply directly.
Comment on attachment 272292 [details] Debug file a new Gedit related testcase This debug output is not related to this bug.
Created attachment 273391 [details] [review] a11y: notify focus changes on EvPageAccessible Adding a null check, as evince was crashing if using a wrong filename.
Review of attachment 271885 [details] [review]: Looks good. ::: libview/ev-page-accessible.c @@ +23,3 @@ + +#include "ev-page-accessible.h" +#include "ev-view-private.h" Do you really need view-private? @@ +34,3 @@ + +G_DEFINE_TYPE_WITH_CODE (EvPageAccessible, ev_page_accessible, ATK_TYPE_OBJECT, + G_IMPLEMENT_INTERFACE (ATK_TYPE_TEXT, ev_page_accessible_text_iface_init)) If you are going to push these commits individually, I prefer if this is added when the interface is actually implemented. @@ +63,3 @@ + atk_page = g_object_new (EV_TYPE_PAGE_ACCESSIBLE, NULL); + atk_page->priv->view_accessible = view_accessible; + atk_page->priv->page = page; These should probably be construct-only properties. ::: libview/ev-page-accessible.h @@ +29,3 @@ +#include <gtk/gtk-a11y.h> +#include "ev-view-accessible.h" +#include "ev-page.h" You are not using EvPage here.
Review of attachment 272303 [details] [review]: Ok, I'm marking this as reviewed for now, because I'm not sure if these patches will be commited individually or squashed, so I won't know if this is good to commit until I review the other patches. ::: libview/ev-view-accessible.c @@ +110,3 @@ + * destroy func at creation */ + g_ptr_array_free (self->priv->children, TRUE); + self->priv->children = NULL; You can use g_ptr_array_unref instead (that will call _free with TRUE) and use g_clear_pointer (&self->priv->children, g_ptr_array_unref);
Review of attachment 272304 [details] [review]: This is just moving code, right? ::: libview/ev-page-accessible.c @@ +705,2 @@ static void ev_page_accessible_text_iface_init (AtkTextIface *iface) Please, move the text iface declaration to this patch as well if you are going to land the patches individually. ::: libview/ev-view-accessible.c @@ +442,3 @@ { EvViewAccessiblePrivate* priv = accessible->priv; + EvPageAccessible *page_accessible = NULL; This doesn't need to be NULL initialized.
Review of attachment 272305 [details] [review]: Ok ::: libview/ev-link-accessible.c @@ +109,3 @@ + /* parent of a EvPageAccessible needs to be EvViewAccessible */ + parent = atk_object_get_parent (ATK_OBJECT (impl_priv->page)); + widget = gtk_accessible_get_widget (GTK_ACCESSIBLE (parent)); This is repeated several times, but the comment is not repeated. Maybe you could add a helper function get_widget or even get_view, since we always get the widget to cast it to a EvView. ::: libview/ev-page-accessible.c @@ +872,3 @@ + +gint +ev_page_accessible_get_page (EvPageAccessible *page_accessible) This probably belongs to the first commit when EvPageAccesible is added, but I'm fine adding it here when first needed too.
Review of attachment 272306 [details] [review]: If I understand the code correctly, now it's impossible to have a EvPageAccessible object if the document doesn't have pages, or created with an invalid page, no? ::: libview/ev-page-accessible.c @@ +68,3 @@ + * atk_object_add_relationship on ev_page_accessible_new because at + * that moment not all the pages could be created, being easier add + * the relation under demand. on demand @@ +70,3 @@ + * the relation under demand. + */ +static AtkRelationSet* AtkRelationSet* -> AtkRelationSet * @@ +73,3 @@ +ev_page_accessible_ref_relation_set (AtkObject *accessible) +{ + gint n_pages = -1; This doesn't need to be initialized. @@ +74,3 @@ +{ + gint n_pages = -1; + EvPageAccessible *self = NULL; Ditto. @@ +75,3 @@ + gint n_pages = -1; + EvPageAccessible *self = NULL; + AtkRelationSet *relation_set = NULL; Ditto. @@ +79,3 @@ + AtkRelation *relation; + AtkObject *prev_page; + AtkObject *next_page; Move these to the if block where they are used. @@ +89,3 @@ + + n_pages = ev_view_accessible_get_n_pages (self->priv->view_accessible); + if (n_pages <= 0) You can probably check also that 0 < self->priv->page < n_pages and return early here to simplify the other ifs. However I wonder if we really that check, since page is construct only attribute, how can it be < 0, for example? or > n_pages? How can this object exist and ev_view_accessible_get_n_pages return <= 0? Do we need to check also that? @@ +98,3 @@ + relation = atk_relation_new (accessible_array, 1, ATK_RELATION_FLOWS_TO); + atk_relation_set_add (relation_set, relation); + /* unref relation so that it is not leaked. Now belongs to the relation_set*/ Please, remove these obvious comments
Review of attachment 272307 [details] [review]: ::: libview/ev-page-accessible.c @@ +120,3 @@ +/* State set is the same that ev-view, but removing SHOWING if the + * page is not on screen. */ I don't understand this comment. @@ +127,3 @@ + AtkStateSet *copy_set; + AtkStateSet *view_accessible_state_set; + EvPageAccessible *self = NULL; This doesn't need to be initialized @@ +128,3 @@ + AtkStateSet *view_accessible_state_set; + EvPageAccessible *self = NULL; + EvView *view = NULL; Ditto. @@ +132,3 @@ + g_return_val_if_fail (EV_IS_PAGE_ACCESSIBLE (accessible), NULL); + self = EV_PAGE_ACCESSIBLE (accessible); + view = EV_VIEW (gtk_accessible_get_widget (GTK_ACCESSIBLE (self->priv->view_accessible))); We always check that the returned widget is not null, can't this be null here too? @@ +146,3 @@ + + g_object_unref (state_set); + g_object_unref (view_accessible_state_set); I don't understand this code at all, but I don't know the ATK API, so I'll trust you here :-)
Review of attachment 272308 [details] [review]: Let's add the API we need in EvPageCache instead. ::: libview/ev-view-accessible.c @@ +149,3 @@ g_return_val_if_fail (i > 0 || i < ev_view_accessible_get_n_pages (self), NULL); + view = EV_VIEW (gtk_accessible_get_widget (GTK_ACCESSIBLE (obj))); We check the returned widget not null in other methods, can't it be null here too? @@ +154,3 @@ + * requested soon, so we update the cache.*/ + page_cache = view->page_cache; + if (page_cache) { We don't need a local variable for this, use view->page_cache directly @@ +160,3 @@ + * just request to cache a given page without + * modifying the cache range */ + ev_page_cache_set_page_range (page_cache, i, i); Maybe we need new EvPageCache API to request this directly, so that we don't need the comment either :-)
Review of attachment 272309 [details] [review]: I guess this belongs to the commit that moved the text implementation to EvPageAccessible, please squash them
Review of attachment 273391 [details] [review]: ::: libview/ev-view-accessible.c @@ +333,3 @@ EvPageAccessible *page_accessible = NULL; + AtkObject *previous_page = NULL; + AtkObject *current_page = NULL; Move these inside the if, and don't initialize them to NULL.
Created attachment 273727 [details] [review] a11y: new EvPageAccessible (In reply to comment #38) > @@ +23,3 @@ > + > +#include "ev-page-accessible.h" > +#include "ev-view-private.h" > > Do you really need view-private? No for this patch. Removed (although it will be added on a following patch). > @@ +34,3 @@ > + > +G_DEFINE_TYPE_WITH_CODE (EvPageAccessible, ev_page_accessible, > ATK_TYPE_OBJECT, > + G_IMPLEMENT_INTERFACE (ATK_TYPE_TEXT, > ev_page_accessible_text_iface_init)) > > If you are going to push these commits individually, I prefer if this is added > when the interface is actually implemented. Done. > @@ +63,3 @@ > + atk_page = g_object_new (EV_TYPE_PAGE_ACCESSIBLE, NULL); > + atk_page->priv->view_accessible = view_accessible; > + atk_page->priv->page = page; > > These should probably be construct-only properties. Done. > ::: libview/ev-page-accessible.h > @@ +29,3 @@ > +#include <gtk/gtk-a11y.h> > +#include "ev-view-accessible.h" > +#include "ev-page.h" > > You are not using EvPage here. True. #include removed.
Created attachment 273728 [details] [review] a11y: expose EvPageAccessible as children of EvViewAccessible (In reply to comment #39) > Review of attachment 272303 [details] [review]: > > Ok, I'm marking this as reviewed for now, because I'm not sure if these patches > will be commited individually or squashed, so I won't know if this is good to > commit until I review the other patches. As mentioned on IRC, it would be more logical (today I moved from 9 to 7 patches). But I still thinks that makes sense to have this one as a individual patch. > ::: libview/ev-view-accessible.c > @@ +110,3 @@ > + * destroy func at creation */ > + g_ptr_array_free (self->priv->children, TRUE); > + self->priv->children = NULL; > > You can use g_ptr_array_unref instead (that will call _free with TRUE) and use > g_clear_pointer (&self->priv->children, g_ptr_array_unref); Ok. Done.
Created attachment 273729 [details] [review] a11y: move AtkText implementation from EvViewAccessible to EvPageAccessible (In reply to comment #40) > Review of attachment 272304 [details] [review]: > > This is just moving code, right? The code was slighlty modified, as now the EvView was not part of the object itself, so it was needed to modify the code to get it. As suggested on a following review, that was refactored to a method ev_page_accessible_get_view, included with this patch. > ::: libview/ev-page-accessible.c > @@ +705,2 @@ > static void > ev_page_accessible_text_iface_init (AtkTextIface *iface) > > Please, move the text iface declaration to this patch as well if you are going > to land the patches individually. Done. > ::: libview/ev-view-accessible.c > @@ +442,3 @@ > { > EvViewAccessiblePrivate* priv = accessible->priv; > + EvPageAccessible *page_accessible = NULL; > > This doesn't need to be NULL initialized. Done.
Created attachment 273730 [details] [review] a11y: move AtkHyperText implementation from EvViewAccessible to EvPageAccessible (In reply to comment #41) > Review of attachment 272305 [details] [review]: > > Ok > > ::: libview/ev-link-accessible.c > @@ +109,3 @@ > + /* parent of a EvPageAccessible needs to be EvViewAccessible */ > + parent = atk_object_get_parent (ATK_OBJECT (impl_priv->page)); > + widget = gtk_accessible_get_widget (GTK_ACCESSIBLE (parent)); > > This is repeated several times, but the comment is not repeated. Maybe you > could add a helper function get_widget or even get_view, since we always get > the widget to cast it to a EvView. Done on the previous patch (move AtkText implementation). > ::: libview/ev-page-accessible.c > @@ +872,3 @@ > + > +gint > +ev_page_accessible_get_page (EvPageAccessible *page_accessible) > > This probably belongs to the first commit when EvPageAccesible is added, but > I'm fine adding it here when first needed too. Yes, I moved this get_page to the first commit, as part of adding view-accessible and page as (construction-only) properties of the page.
Created attachment 273731 [details] [review] a11y: add if applies ATK_RELATION_FLOWS_TO/FROM on EvPageAccessible (In reply to comment #42) > Review of attachment 272306 [details] [review]: > > If I understand the code correctly, now it's impossible to have a > EvPageAccessible object if the document doesn't have pages, or created with an > invalid page, no? That's the idea. In that case we still have an EvViewAccessible, but 0 children. If something else happens, should be a bug. > ::: libview/ev-page-accessible.c > @@ +68,3 @@ > + * atk_object_add_relationship on ev_page_accessible_new because at > + * that moment not all the pages could be created, being easier add > + * the relation under demand. > > on demand Done. > @@ +70,3 @@ > + * the relation under demand. > + */ > +static AtkRelationSet* > > AtkRelationSet* -> AtkRelationSet * Done. > @@ +73,3 @@ > +ev_page_accessible_ref_relation_set (AtkObject *accessible) > +{ > + gint n_pages = -1; > > This doesn't need to be initialized. Done. > @@ +74,3 @@ > +{ > + gint n_pages = -1; > + EvPageAccessible *self = NULL; > > Ditto. Done. > @@ +75,3 @@ > + gint n_pages = -1; > + EvPageAccessible *self = NULL; > + AtkRelationSet *relation_set = NULL; > > Ditto. Done. > @@ +79,3 @@ > + AtkRelation *relation; > + AtkObject *prev_page; > + AtkObject *next_page; > > Move these to the if block where they are used. Done. > @@ +89,3 @@ > + > + n_pages = ev_view_accessible_get_n_pages (self->priv->view_accessible); > + if (n_pages <= 0) > > You can probably check also that 0 < self->priv->page < n_pages and return > early here to simplify the other ifs. The other ifs are based on the current_page+1 or current_page-1, so the check is not the same. > However I wonder if we really that check, > since page is construct only attribute, how can it be < 0, for example? or > > n_pages? How can this object exist and ev_view_accessible_get_n_pages return <= > 0? Do we need to check also that? True. I made that <=0 check because ev_view_accessible_get_n_pages was returning -1 with children==NULL. But as you say, that is an error. Fixed ev_view_accessible_get_n_pages and changed the check to ==0 instead of <=0 > @@ +98,3 @@ > + relation = atk_relation_new (accessible_array, 1, > ATK_RELATION_FLOWS_TO); > + atk_relation_set_add (relation_set, relation); > + /* unref relation so that it is not leaked. Now belongs to the > relation_set*/ > > Please, remove these obvious comments Done.
Created attachment 273732 [details] [review] a11y: managing atk states on EvPageAccessible (In reply to comment #43) > Review of attachment 272307 [details] [review]: > > ::: libview/ev-page-accessible.c > @@ +120,3 @@ > > +/* State set is the same that ev-view, but removing SHOWING if the > + * page is not on screen. */ > > I don't understand this comment. I rewrote that comment. I hope that now is more understandable. > > @@ +127,3 @@ > + AtkStateSet *copy_set; > + AtkStateSet *view_accessible_state_set; > + EvPageAccessible *self = NULL; > > This doesn't need to be initialized Done. > > @@ +128,3 @@ > + AtkStateSet *view_accessible_state_set; > + EvPageAccessible *self = NULL; > + EvView *view = NULL; > > Ditto. Done. > > @@ +132,3 @@ > + g_return_val_if_fail (EV_IS_PAGE_ACCESSIBLE (accessible), NULL); > + self = EV_PAGE_ACCESSIBLE (accessible); > + view = EV_VIEW (gtk_accessible_get_widget (GTK_ACCESSIBLE > (self->priv->view_accessible))); > > We always check that the returned widget is not null, can't this be null here > too? Now uses ev_page_accessible_get_view > @@ +146,3 @@ > + > + g_object_unref (state_set); > + g_object_unref (view_accessible_state_set); > > I don't understand this code at all, but I don't know the ATK API, so I'll > trust you here :-) Ok. (In reply to comment #46) This one was squashed on this patch. > Review of attachment 273391 [details] [review]: > > ::: libview/ev-view-accessible.c > @@ +333,3 @@ > EvPageAccessible *page_accessible = NULL; > + AtkObject *previous_page = NULL; > + AtkObject *current_page = NULL; > > Move these inside the if, and don't initialize them to NULL. Done.
Comment on attachment 272309 [details] [review] a11y: emit text selection changed from the page instead of the view Squash as suggested done, marking as obsolete the patch.
Created attachment 273734 [details] [review] a11y: ensure text is cached when you request an EvPage (In reply to comment #44) > Review of attachment 272308 [details] [review]: > > Let's add the API we need in EvPageCache instead. I added a method called ev_page_cache_load_page instead of just adding to the header ev_page_cache_schedule_job_if_needed (the need to schedule jobs seems too much information to me). But in any case, the implementation just call that method. > ::: libview/ev-view-accessible.c > @@ +149,3 @@ > g_return_val_if_fail (i > 0 || i < ev_view_accessible_get_n_pages (self), > NULL); > > + view = EV_VIEW (gtk_accessible_get_widget (GTK_ACCESSIBLE (obj))); > > We check the returned widget not null in other methods, can't it be null here > too? Null check added. > @@ +154,3 @@ > + * requested soon, so we update the cache.*/ > + page_cache = view->page_cache; > + if (page_cache) { > > We don't need a local variable for this, use view->page_cache directly Done. > @@ +160,3 @@ > + * just request to cache a given page without > + * modifying the cache range */ > + ev_page_cache_set_page_range (page_cache, i, i); > > Maybe we need new EvPageCache API to request this directly, so that we don't > need the comment either :-) As mentioned, API added.
Review of attachment 273727 [details] [review]: Please, fix my comments and push the patch. ::: libview/ev-page-accessible.c @@ +3,3 @@ + * + * Copyright (C) 2014 Igalia S.L. + * Author: Alejandro Piñeiro Iglesias <apinheiro@igalia.com> Please, leave the copyright here and move the author at the end of the license comment. @@ +76,3 @@ + switch (prop_id) { + case PROP_VIEW_ACCESSIBLE: + accessible->priv->view_accessible = (EvViewAccessible*)g_value_get_object (value); (EvViewAccessible*) -> (EvViewAccessible *) or even EV_VIEW_ACCESSIBLE() @@ +95,3 @@ + + switch (prop_id) { + case PROP_VIEW_ACCESSIBLE: This property is defined writable, this will never be reached. I think it's clearly a readable property because you are adding a public getter for it. @@ +125,3 @@ + "The view accessible associated to this page", + EV_TYPE_VIEW_ACCESSIBLE, + G_PARAM_WRITABLE | This should probably be G_PARAM_READWRITE, otherwise remove the public getter and the case from the switch in ev_page_accessible_get_property @@ +132,3 @@ + g_param_spec_int ("page", + "Page", + "Current page", This is not the current page, but the page index this object represents. @@ +152,3 @@ + gint page) +{ + EvPageAccessible *atk_page; You should add g_return macros here to check view_accessible is a valid EvViewAccessible and page is at least > 0 ::: libview/ev-page-accessible.h @@ +22,3 @@ +#if !defined (EVINCE_COMPILATION) +#error "This is a private header." +#endif Do we really need this? This header was added to NOINST_H_SRC_FILES in the Makefile. If it's not installed, apps can't include it :-)
Review of attachment 273728 [details] [review]: Push it after fixing my comments please. Thanks. ::: libview/ev-view-accessible.c @@ +1219,3 @@ + + if (self->priv->model == NULL) + return; This shouldn't be possible, this is only called from document_changed_cb, so we should have a model. If you want to be sure, add an assert instead of an early return. @@ +1223,3 @@ + ev_document = ev_document_model_get_document (self->priv->model); + if (ev_document == NULL) + return; And same here, there's an early return in document_changed_cb when document is NULL. So, remove this check or use an assert instead.
Review of attachment 273729 [details] [review]: Ok
Review of attachment 273730 [details] [review]: Ok
Review of attachment 273731 [details] [review]: ::: libview/ev-view-accessible.c @@ +131,3 @@ ev_view_accessible_get_n_pages (EvViewAccessible *self) { + return self->priv->children == NULL ? 0 : self->priv->children->len; This looks like a fix to this method, you can probably move this to the commit that introduced this condition. And also check that there aren't any callers checking the return value is -1.
Review of attachment 273732 [details] [review]: Ok
Review of attachment 273734 [details] [review]: ::: libview/ev-page-cache.c @@ +657,3 @@ + +void +ev_page_cache_load_page (EvPageCache *cache, I agree on not exposing schedule_job_if_needed, but load_page sounds a bit confusing to me. The page might already be in cache, and we are not loading anything. We could use something like ev_page_cache_ensure_page(), what do you think? @@ +660,3 @@ + gint page) +{ + g_return_if_fail (EV_IS_PAGE_CACHE (cache)); Check also the page is valid. ::: libview/ev-view-accessible.c @@ +149,3 @@ + return NULL; + + /* If a given page is asked, we assume that the text would be asked -> requested?
Comment on attachment 273727 [details] [review] a11y: new EvPageAccessible Changes done. Committed.
Comment on attachment 273728 [details] [review] a11y: expose EvPageAccessible as children of EvViewAccessible Changes done. Committed.
Comment on attachment 273731 [details] [review] a11y: add if applies ATK_RELATION_FLOWS_TO/FROM on EvPageAccessible Changed done, and checked if comparison with -1 is done. Committed.
Created attachment 274113 [details] [review] a11y: ensure text is cached when you request an EvPage (In reply to comment #61) > Review of attachment 273734 [details] [review]: > > ::: libview/ev-page-cache.c > @@ +657,3 @@ > + > +void > +ev_page_cache_load_page (EvPageCache *cache, > > I agree on not exposing schedule_job_if_needed, but load_page sounds a bit > confusing to me. The page might already be in cache, and we are not loading > anything. We could use something like ev_page_cache_ensure_page(), what do you > think? Yes makes sense. This new patch uses that name. > > @@ +660,3 @@ > + gint page) > +{ > + g_return_if_fail (EV_IS_PAGE_CACHE (cache)); > > Check also the page is valid. If you mean checking that the page is in the proper range (as the other page-cache public methods are doing), then done. > ::: libview/ev-view-accessible.c > @@ +149,3 @@ > + return NULL; > + > + /* If a given page is asked, we assume that the text would be > > asked -> requested? I updated the comment, with this and some small text changes.
Review of attachment 274113 [details] [review]: Thanks
(In reply to comment #66) > Review of attachment 274113 [details] [review]: > > Thanks Thank you very much. Closing bug.
Btw, I have just realized that I only committed these patches on master. But as we talked to try to get this for 3.12.1, should I also have committed all those patches to gnome-3-12 branch?
Yes, please
(In reply to comment #69) > Yes, please Patches just pushed to gnome-3-12 branch.
*** Bug 701718 has been marked as a duplicate of this bug. ***
*** Bug 701720 has been marked as a duplicate of this bug. ***
*** Bug 704620 has been marked as a duplicate of this bug. ***