New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement caret navigation #281

Merged
merged 78 commits into from Sep 13, 2017

Conversation

Projects
None yet
6 participants
@raveit65
Member

raveit65 commented Sep 7, 2017

This will enable caret navigation inside a pdf dokument and it works with orca.
The F7 key enables the caret-navigation.
I know this is a huge PR but the commit for enabling F7 was done very late at upstream.
And without the key you can't test something.
Mainly i want to know that i didn't broke any existing functions.
If something goes wrong i can split the PR.
At least this is a first step and i will port more commits from upstream.
This won't work with epubs. But i noticed that keyboard navigation was already enabled before with ebubs if you have a text selection.
Please test.

José Aliste and others added some commits Feb 8, 2013

libview: Refactor code for drawing page and selection surfaces
The code necessary to draw a page surface or a selection
surface is the same. We factor this out to  a new static method
called draw_surface

origin commit:
https://git.gnome.org/browse/evince/commit/?h=gnome-3-8&id=3ab6ac1

libview: Fix rendering of the first visible page while resizing

While we are resizing the view widget and waiting for a new
surface rendered at the right size, we use old surfaces
scaled to match the target size. When the begining of a page is not
visible, the scaled surfaces are not placed correctly.

origin commit:
https://git.gnome.org/browse/evince/commit/?h=gnome-3-8&id=72f2ae4
libview: Initial implementation of caret navigation
Navigation by character, word, next/previous line and beginning/end of
the line using the caret cursor.

The routines to move the cursor don't use GtkTextBuffer to
avoid the duplication of the text for every page.
  - Left/right arrow: Move one character to the left/right.
  - Up/down arrow: Move up/down one line.
  - Ctrl + left/right arrow: Move to the beginning/end of
    the previous/next word.
  - Home/End: Move to the beginning/end of the current line.

https://bugzilla.gnome.org/show_bug.cgi?id=638905

taken from:
https://git.gnome.org/browse/evince/commit/?h=gnome-3-10&id=1dc10fe
libview: Add a way to get the text logical attributes from the page c…
…ache

It returns an array of PangoLogAttr with the logical attributes of the
text for the given page.

taken from:
https://git.gnome.org/browse/evince/commit/?h=gnome-3-10&id=6870279
libview: Avoid to update the current page and scroll to the cursor po…
…sition, if the caret cursor was not updated

Move the caret cursor only when requested by the user. Keep the page
where the caret cursor is in addition to the offset inside that page.

https://bugzilla.gnome.org/show_bug.cgi?id=702068
origin commit:
https://git.gnome.org/browse/evince/commit/?h=gnome-3-10&id=4d15ab5
libview: Make caret cursor blink
Based on GtkEntry and GtkTextView implementation, the caret cursor
blinks when the view is active and caret navigation is enabled. It stops
blinking after a while if there's no user interaction. It uses
GtkSettings:gtk-cursor-blink-time and
GtkSettings:gtk-cursor-blink-timeout.

https://bugzilla.gnome.org/show_bug.cgi?id=702076

taken from:
https://git.gnome.org/browse/evince/commit/?h=gnome-3-10&id=e6f7250
libview: Actually update the page when moving the care from a differe…
…nt page

I removed the line to set the current page after moving the caret cursor
by mistake. Add it back and change cursor_go_to_next/previous_page to
not update the page, but only update the cursor_page so that we have a
single place where the page is updated after caret cursor moves.

origin commit:
https://git.gnome.org/browse/evince/commit/?h=gnome-3-10&id=bf85030
libview: Fix the damage area used to redraw the caret cursor
Move to a helper function the code to get the exact area where the caret
should be drawn and use it to draw the care cursor and to invalidate the
area when blinking.

origin commit:
https://git.gnome.org/browse/evince/commit/?h=gnome-3-10&id=f5e7eb4
libview: Draw selection highlight from region
Allows a fallback for backends which can implement get_selection_region
but not render_selection.  Changes ev-pixbuf-cache so a redraw is only
done when the scale changes.

https://bugzilla.gnome.org/show_bug.cgi?id=669022

origin commit:
https://git.gnome.org/browse/evince/commit/?h=gnome-3-10&id=9e89fb1
libview: Split ev_pixbuf_cache_get_selection_surface into two functions
ev_pixbuf_cache_get_selection_surface returns the selection surface, and
new function ev_pixbuf_cache_get_selection_region returns the selection
region.

https://bugzilla.gnome.org/show_bug.cgi?id=669022

origin commit:
https://git.gnome.org/browse/evince/commit/?h=gnome-3-10&id=90d6580
libview: Use EvPixbufCache to find selection region
Using view->selection_info.selections is inaccurate if the zoom
changes, causing drag and drop of selected text to fail.  This changes
location_in_selected_text function to use the more accurate
ev_pixbuf_cache_get_selection_region to find the selection region.

https://bugzilla.gnome.org/show_bug.cgi?id=702406

origin commit:
https://git.gnome.org/browse/evince/commit/?h=gnome-3-10&id=abedcbd
libview: Rename EvView::binding-activated signal as EvView::scroll
EvView::binding-activated is too generic name for scroll key bindings.
Renamed as EvView::scroll and changed to use GtkOrientable instead of a
gboolean.

origin commit:
https://git.gnome.org/browse/evince/commit/?h=gnome-3-10&id=b70e413
libview: Use GtkBindings for caret navigation
This allows themes to override the key bindings and API users to move
the caret cursor programmatically using g_signal_emit_by_name.

origin commit:
https://git.gnome.org/browse/evince/commit/?h=gnome-3-10&id=1206ff1
libview: merge get_caret_cursor_rect_from_offset and get_caret_cursor…
…_area

Into a single function and use get_caret_cursor_area everywhere.
get_caret_cursor_rect_from_offset() was a bit confusing, because it
didn't return the cursor_rect, but the bbox of the character where the
cursor should be at. The method that actually returns the cursor_rect is
get_caret_cursor_area.

origin commit:
https://git.gnome.org/browse/evince/commit/?h=gnome-3-10&id=13a5978
libview: Make sure the cursor blinks after moving it by a click
The cursor blink is reset when the cursor is moved with the keyboard,
but not when positioned with the mouse.

origin commit:
https://git.gnome.org/browse/evince/commit/?h=gnome-3-10&id=1435ace
libview: Update the caret cursor after selecting text with the mouse
If there's an active selection move the caret cursor to the location
where the mouse is released.

origin commit:
https://git.gnome.org/browse/evince/commit/?h=gnome-3-10&id=619c100
libview: Use GSlice to allocate EvViewSelection
These structs are allocated and deallocated a lot while selecting text
and merging selection regions.

orign commit:
https://git.gnome.org/browse/evince/commit/?h=gnome-3-10&id=dc1e75b7be87
libview: Position the caret cursor at beginning/end of the line
when  clicking outside the line

Position the caret cursor also when not clicking over text if the line
contains text.

origin commit:
https://git.gnome.org/browse/evince/commit/?h=gnome-3-10&id=040a42d
libview: Remove unused in_selection member from SelectionInfo struct
This is currently unused and redundant, to know whether there are
selections we use selection_info.selections.

origin commit:
https://git.gnome.org/browse/evince/commit/?h=gnome-3-10&id=5503827

raveit65 and others added some commits Sep 6, 2017

Fix crashes of epub documents with caret-navigation
This is a quick fix for getting caret-navigation working in atril.
All this code needs to be reworked for epubs.
Note: caret-navigation is always enabled if text is selected with epubs,
for some reasons.
@raveit65

This comment has been minimized.

Member

raveit65 commented Sep 7, 2017

@alexarnaud @ksamak
please test....

@lukefromdc

Atril basically worked as it used to with this, but hitting F7 to invoke caret navigation triggered a segfault.

@lukefromdc

This comment has been minimized.

Member

lukefromdc commented Sep 7, 2017

Here's what I got from gdb

Reading symbols from atril...done.
(gdb) run
Starting program: /usr/bin/atril 
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
[New Thread 0x7fffe19fe700 (LWP 28100)]
[New Thread 0x7fffe078d700 (LWP 28101)]
[New Thread 0x7fffdff8c700 (LWP 28102)]
[New Thread 0x7fffdf78b700 (LWP 28103)]
[New Thread 0x7fffdef8a700 (LWP 28104)]
[New Thread 0x7fffdd7ce700 (LWP 28105)]
[New Thread 0x7fffdcfcd700 (LWP 28106)]
[New Thread 0x7fff87ffd700 (LWP 28107)]
[New Thread 0x7fff877fc700 (LWP 28108)]
[New Thread 0x7fff86ffb700 (LWP 28111)]
[New Thread 0x7fff85fbd700 (LWP 28120)]
[New Thread 0x7fff857bc700 (LWP 28121)]
[Thread 0x7fffdf78b700 (LWP 28103) exited]
[New Thread 0x7fffdf78b700 (LWP 28152)]
[New Thread 0x7fff84d9e700 (LWP 28153)]
[New Thread 0x7fff67fff700 (LWP 28154)]
[New Thread 0x7fff677fe700 (LWP 28155)]
[Thread 0x7fffdef8a700 (LWP 28104) exited]
[Thread 0x7fff677fe700 (LWP 28155) exited]
[Thread 0x7fff67fff700 (LWP 28154) exited]
[Thread 0x7fff84d9e700 (LWP 28153) exited]
[New Thread 0x7fff84d9e700 (LWP 28198)]
[New Thread 0x7fff67fff700 (LWP 28199)]
[New Thread 0x7fff677fe700 (LWP 28201)]
[Thread 0x7fff857bc700 (LWP 28121) exited]

(atril:28076): Gtk-WARNING **: Allocating size to EvSidebar 0x555555b75f70 without calling gtk_widget_get_preferred_width/height(). How does the code know the size to allocate?
[Thread 0x7fff677fe700 (LWP 28201) exited]
[Thread 0x7fff84d9e700 (LWP 28198) exited]

Thread 1 "atril" received signal SIGSEGV, Segmentation fault.
strlen () at ../sysdeps/x86_64/strlen.S:106
106	../sysdeps/x86_64/strlen.S: No such file or directory.
(gdb) 

And this backtrace:


(gdb) thread apply all bt full

cut

Thread 1 (Thread 0x7ffff7f90ac0 (LWP 28076)):
#0  0x00007fffeca31676 in strlen () at ../sysdeps/x86_64/strlen.S:106
#1  0x00007fffed2d3b13 in g_strdup () at /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
#2  0x00007fffeea5649d in  () at /usr/lib/x86_64-linux-gnu/libatk-1.0.so.0
#3  0x00007fffeea578ad in atk_object_set_name ()
    at /usr/lib/x86_64-linux-gnu/libatk-1.0.so.0
#4  0x0000555555578b5f in ev_message_area_set_image_for_type (type=<optimized out>, area=0x555555f222d0) at ev-message-area.c:184
        item = 
          {stock_id = 0x54552e53555f6e65 <error: Cannot access memory at address 0x54552e53555f6e65>, label = 0x555500382d46 <error: Cannot access memory at address 0x555500382d46>, modifier = 0, keyval = 0, translation_domain = 0x7fffec9de8e4 <__dcigettext+180> "H\215P\001H\203\300\037H\211\336H\203\340\360H)\304H\215|$\017H\203\347\360\350\214e\005"}
        icon_name = <optimized out>
        atk_obj = 0x555555b67440
        widget = 0x555555f222d0
#5  0x0000555555578b5f in ev_message_area_new (type=type@entry=GTK_MESSAGE_QUESTION, text=---Type <return> to continue, or q <return> to quit---
<optimized out>, first_button_text=first_button_text@entry=0x5555555a5b66 "gtk-no")
    at ev-message-area.c:280
        widget = 0x555555f222d0
#6  0x00005555555857c1 in ev_window_cmd_view_toggle_caret_navigation (action=<optimized out>, window=0x5555558c73b0) at ev-window.c:5947
        message_area = <optimized out>
        box = <optimized out>
        hbox = <optimized out>
        enabled = 0
        action = <optimized out>
        window = 0x5555558c73b0
        enabled = 0
#7  0x00007fffed58d69d in g_closure_invoke ()
    at /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#8  0x00007fffed5a05fe in  () at /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#9  0x00007fffed5a8fa5 in g_signal_emit_valist ()
    at /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#10 0x00007fffed5a9962 in g_signal_emit ()
    at /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#11 0x00007fffef45b270 in  () at /usr/lib/x86_64-linux-gnu/libgtk-3.so.0
#12 0x00007fffef45b7f9 in  () at /usr/lib/x86_64-linux-gnu/libgtk-3.so.0
#13 0x00007fffed58d69d in g_closure_invoke ()
    at /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
---Type <return> to continue, or q <return> to quit---
#14 0x00007fffed5a05fe in  () at /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#15 0x00007fffed5a895c in g_signal_emit_valist ()
    at /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#16 0x00007fffed5a9962 in g_signal_emit ()
    at /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#17 0x00007fffef4b0034 in gtk_accel_group_activate ()
    at /usr/lib/x86_64-linux-gnu/libgtk-3.so.0
#18 0x00007fffef4b183d in gtk_accel_groups_activate ()
    at /usr/lib/x86_64-linux-gnu/libgtk-3.so.0
#19 0x00007fffef73d692 in gtk_window_activate_key ()
    at /usr/lib/x86_64-linux-gnu/libgtk-3.so.0
#20 0x00007fffef73d8c1 in  () at /usr/lib/x86_64-linux-gnu/libgtk-3.so.0
#21 0x00007fffef5d9b52 in  () at /usr/lib/x86_64-linux-gnu/libgtk-3.so.0
#22 0x00007fffed58d8d6 in  () at /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#23 0x00007fffed5a8c74 in g_signal_emit_valist ()
    at /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#24 0x00007fffed5a9962 in g_signal_emit ()
    at /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#25 0x00007fffef71a1b4 in  () at /usr/lib/x86_64-linux-gnu/libgtk-3.so.0
#26 0x00007fffef5d6b9f in  () at /usr/lib/x86_64-linux-gnu/libgtk-3.so.0
#27 0x00007fffef5d8b98 in gtk_main_do_event () at /usr/lib/x86_64-linux-gnu/libgtk-3.so.0
#28 0x00007fffef0fe8f5 in  () at /usr/lib/x86_64-linux-gnu/libgdk-3.so.0
#29 0x00007fffef12eba2 in  () at /usr/lib/x86_64-linux-gnu/libgdk-3.so.0
---Type <return> to continue, or q <return> to quit---
#30 0x00007fffed2b5a07 in g_main_context_dispatch ()
    at /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
#31 0x00007fffed2b5c18 in  () at /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
#32 0x00007fffed2b5f22 in g_main_loop_run ()
    at /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
#33 0x00007fffef5d7ce5 in gtk_main () at /usr/lib/x86_64-linux-gnu/libgtk-3.so.0
#34 0x0000555555573268 in main (argc=<optimized out>, argv=<optimized out>) at main.c:277
        context = <optimized out>
        error = 0x0

@raveit65

This comment has been minimized.

Member

raveit65 commented Sep 8, 2017

Hmm, can you test it with stable glib2 ?

Thread 1 (Thread 0x7ffff7f90ac0 (LWP 28076)):
#0  0x00007fffeca31676 in strlen () at ../sysdeps/x86_64/strlen.S:106
#1  0x00007fffed2d3b13 in g_strdup () at /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
#2  0x00007fffeea5649d in  () at /usr/lib/x86_64-linux-gnu/libatk-1.0.so.0
#3  0x00007fffeea578ad in atk_object_set_name ()
    at /usr/lib/x86_64-linux-gnu/libatk-1.0.so.0

I will check this for myself with unstable glib2 in fedora 27 (pre)-beta.

@raveit65

This comment has been minimized.

Member

raveit65 commented Sep 8, 2017

And do you have this gsettings key?

[rave@mother ~]$ gsettings get org.mate.Atril show-caret-navigation-message
true
@lukefromdc

This comment has been minimized.

Member

lukefromdc commented Sep 8, 2017

I have show-caret-navigation-message enabled (the default) and the segfault occurs before the dialog can appear

@lukefromdc

This comment has been minimized.

Member

lukefromdc commented Sep 8, 2017

If I disable show-caret-navigation-message (set it to false), caret navigation works both with stable glib2.52.0 and with glib 2.53.6.

If I enable the dialog I get this message in gdb as soon as I hit F7, with either glib version, looks like a file is not being found:

Thread 1 "atril" received signal SIGSEGV, Segmentation fault.
strlen () at ../sysdeps/x86_64/strlen.S:106
106	../sysdeps/x86_64/strlen.S: No such file or directory.
(gdb) 

@raveit65

This comment has been minimized.

Member

raveit65 commented Sep 8, 2017

I tested it here with generating a new 1.19.4 tarball with './autogen.sh && make && mate distcheck'.
Then i built a rpm from that tarball.
Not sure how you installed packages for testing, but did you run ./autogen.sh before compiling?

@lukefromdc

This comment has been minimized.

Member

lukefromdc commented Sep 8, 2017

Yes I had to run the ./autogen.sh script because this was a build from a new directory copied from my git repository with the .git folder removed as though for a tarball. It was not a matter of dropping a new file into an existing build directory as it was entirely too complex for that approach.

These are my normal build options:

./autogen.sh --prefix=/usr --libdir=/usr/lib/x86_64-linux-gnu --sysconfdir=/etc --enable-shared=yes --disable-maintainer-mode

and for building Debian packages I add --disable-schemas-compile because the postinst script recompiles the schemas. If recompiling schemas is in the build rather than in postinst, the /usr/share/glib-2.0/schemas/gschemas.compiled file gets picked up by checkinstall and copied into the Debian package.

In this case, my initial build (for make install) was without recompiling schemas and I got the usual error about the key not being present. I then ran my usual postinst script as though a Debian package had been installed so as to force recompilation of schemas, then it showed up in dconf-editor and could be used.

Only with the dialog disabled though could I sucessfully use caret navigation within a document (a pdf file). It seem the dialog contains the problem.

@raveit65

This comment has been minimized.

Member

raveit65 commented Sep 9, 2017

Ok, i can reproduce it in f27 pre-beta + glib2-2.53.6-1.fc27.x86_64 + show-caret-navigation-message enabled.

Thread 1 (Thread 0x7f4744780b00 (LWP 14579)):
#0  __strlen_sse2 () at ../sysdeps/x86_64/multiarch/../strlen.S:120
No locals.
#1  0x00007f473cf11c13 in g_strdup (str=str@entry=0x697274612f534547 <error: Cannot access memory at address 0x697274612f534547>) at gstrfuncs.c:362
        new_str = <optimized out>
        length = <optimized out>
#2  0x00007f473e6a20dd in atk_object_real_set_name (object=0x5591d0a43830, name=0x697274612f534547 <error: Cannot access memory at address 0x697274612f534547>) at atkobject.c:1463
No locals.
#3  0x00007f473e6a351e in atk_object_set_name (accessible=accessible@entry=0x5591d0a43830, name=0x697274612f534547 <error: Cannot access memory at address 0x697274612f534547>) at atkobject.c:1056
        klass = <optimized out>
        notify = 1
        __func__ = "atk_object_set_name"
#4  0x00005591cf0c1c27 in ev_message_area_set_image_for_type (type=<optimized out>, area=0x5591d0a2eba0) at ev-message-area.c:184
        item = {stock_id = 0x415353454d5f434c <error: Cannot access memory at address 0x415353454d5f434c>, label = 0x697274612f534547 <error: Cannot access memory at address 0x697274612f534547>, modifier = 1869426284, keyval = 21760, translation_domain = 0x5591d0408f90 "de_DE.utf8"}
        icon_name = <optimized out>
        atk_obj = 0x5591d0a43830
#5  ev_message_area_new (type=type@entry=GTK_MESSAGE_QUESTION, text=<optimized out>, first_button_text=first_button_text@entry=0x5591cf0efe86 "gtk-no") at ev-message-area.c:280
        widget = 0x5591d0a2eba0
#6  0x00005591cf0ced31 in ev_window_cmd_view_toggle_caret_navigation (action=<optimized out>, window=0x5591d0553f50) at ev-window.c:5947
        message_area = <optimized out>
        box = <optimized out>
        hbox = <optimized out>
        enabled = 0
        action = <optimized out>
        window = 0x5591d0553f50
        enabled = 0

I will investigate why i don't have the crash in stable f26.
The f27 rpm is simply a rebuild from working f26 rpm.

PS: i will shortened you first stackstrace in your post for making the report better readable. Usually only Thread 1 is from interest.

@raveit65

This comment has been minimized.

Member

raveit65 commented Sep 9, 2017

I found the reason for the crash but i am still wondering why i don't get the crash with f26.
In an older commit stock_id for gtk_stock_lookup was replaced with icon_name.
Please test again.

@lukefromdc

This comment has been minimized.

Member

lukefromdc commented Sep 9, 2017

This fixed the crash, but for some reason I didn't get the caret after a rebuild with this added

@raveit65

This comment has been minimized.

Member

raveit65 commented Sep 10, 2017

Hmm, this is weird.
I did only change this little code snipset for fixing gtk_stock_lookup.
After i enable the caret in message_area and clicking somewhere in the pdf document it works here.
For some reasons the caret starts always with the first word in the document here, don't matter which side is selected in sidebar.
I think this is fixed later in evince.

@lukefromdc

This comment has been minimized.

Member

lukefromdc commented Sep 10, 2017

I just found out that rather or not you can get the caret seems to depend on the document. My tests today were with a different document, and that one didn't give the caret. I was able to get it with a bit of fiddling on some other documents. Some documents let the caret come up right away, some with a bit of fiddling and maybe a spike in CPU use, and some of them not at all. All the documents in question are random .pdf files I have around.

Anyway, this latest commit didn't break the caret, the test document just happened to be one that did not support it and the previous test document (used when the dialog caused a crash) did support it.

@raveit65

This comment has been minimized.

Member

raveit65 commented Sep 10, 2017

Good, this might be possible. A good reference is to check it with evince.
Currently this is the state from evince-3.10, so i need to back port more, but first this PR should work without breaking other functions.

@lukefromdc

This comment has been minimized.

Member

lukefromdc commented Sep 10, 2017

So far I have not found any problems using Atril with caret navigation not enabled with this, but then again, I haven't been spending all day working with pdf files either

@raveit65

This comment has been minimized.

Member

raveit65 commented Sep 11, 2017

Same for me, i do not not work with PDFs whole the day.
Well, i will wait a while with merging and start with finishing caret support local.
@lukefromdc
Can you approve it or do you see more problems?

@lukefromdc

This comment has been minimized.

Member

lukefromdc commented Sep 11, 2017

@raveit65 raveit65 merged commit 52283c0 into master Sep 13, 2017

@raveit65 raveit65 deleted the dev-caret-navigation-2 branch Sep 13, 2017

@monsta

This comment has been minimized.

Member

monsta commented on 52283c0 Sep 19, 2017

Hmm, stock_id is always NULL in this case

This comment has been minimized.

Member

raveit65 replied Sep 19, 2017

This was simply a revert from a faulty GtkStock replacement.
f2064cf
Otherwise atril crashes if you press F7.
And the NULL was already there.
f2064cf#diff-0006a168a62728ff41e77b348bd4d910L150

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment