GNOME Bugzilla – Bug 701732
Implement AtkText signals for caret navigation within document content
Last modified: 2013-07-08 11:21:43 UTC
Once caret navigation has been implemented (see bug 638905), the following two AtkText signals/events should be implemented: * atk_text-caret-moved * atk_text-selection-changed
Apologies for the c&p error. The above are, of course, text-caret-moved and text-selection-changed respectively.
Created attachment 246394 [details] [review] proposed patch: emit text-caret-moved signal
Review of attachment 246394 [details] [review]: Thanks. ::: libview/ev-view.c @@ +4783,3 @@ } + g_signal_emit_by_name (ATK_TEXT (view->accessible), "text_caret_moved", view->cursor_offset); This would be emitted even if the caret hasn't moved, for example if you press HOME key and you are already at line start. Also, I think ATK signals should be emitted by EvViewAccessible. We could add a readable property to EvView, cursor-position for example, and EvView accesible could connect to notify signal. I think it could be useful for other EvView users as well. Maybe we can make all caret functions return the new offset (or -1 in case of error) instead of returning TRUE/FALSE, and have a function set the new offset that checks wheter it has actually changed and emits the notify signal in such case. It will be useful also when the offsert can be changed by clicking, for example.
Created attachment 246529 [details] [review] proposed patch (In reply to comment #3) > Review of attachment 246394 [details] [review]: Thanks for the review! > This would be emitted even if the caret hasn't moved, for example if you press > HOME key and you are already at line start. Ugh. Fixed. > Also, I think ATK signals should be emitted by EvViewAccessible. Done. > We could add a readable property to EvView, > cursor-position for example, EvView has cursor-offset already. What would cursor-position contain? Because I didn't understand this, I've not done it in this new patch. > and EvView accesible could connect to notify signal. Related to my previous comment, because I didn't understand the cursor-position, this new patch does emit a signal, but it's a new caret-moved signal rather than a notify signal on the proposed property. > make all caret functions return the new offset (or -1 in case of error) instead > of returning TRUE/FALSE, Not done yet, as I'm curious as to what you think about my new patch. > and have a function set the new offset that checks > wheter it has actually changed and emits the notify signal in such case. Done. > will be useful also when the offsert can be changed by clicking, for example. Verified it works when clicking.
Review of attachment 246529 [details] [review]: Now that we keep both the page and cursor offset, it makes more sense to use a signal instead of a property, because the cursor might be moved to the same offset of a different page, for example. ::: libview/ev-view-accessible.c @@ +904,3 @@ + EvViewAccessible *accessible) +{ + g_signal_emit_by_name (ATK_TEXT (accessible), "text_caret_moved", view->cursor_offset); I think the use of underscore for signal names is deprecated, use text-caret-moved instead. I think the current offset should be received as paramater. I wonder what happens with the current page, though. ::: libview/ev-view.c @@ +4372,3 @@ + return FALSE; + + view->cursor_offset = new_offset; This has changed a bit, cursor is positioned differently when using keybindins or by clicking. In both cases we already check whether the cursor has actually changed before queuing a redraw, so we could just emit the signal directly there instead of using a helper function to reposition the cursor. @@ +5480,3 @@ + G_TYPE_FROM_CLASS (object_class), + G_SIGNAL_RUN_LAST | G_SIGNAL_ACTION, + G_STRUCT_OFFSET (EvViewClass, layers_changed), layers_changed? :-) Since this is just a notification signal we can use NULL here and not add a vmethod to the class struct. We should also remove the G_SIGNAL_ACTION. @@ +5484,3 @@ + g_cclosure_marshal_VOID__VOID, + G_TYPE_NONE, 0, + G_TYPE_NONE); I wonder if it would be useful to pass the cursor page and offset as parameters.
Created attachment 248328 [details] [review] Emit the ATK text-caret-moved signal
Created attachment 248337 [details] [review] Emit the ATK text-caret-moved signal Updating patch.
Review of attachment 248337 [details] [review]: ::: libview/ev-view-accessible.c @@ +915,3 @@ + EvViewAccessible *accessible) +{ + ev_view_accessible_set_caret_offset (ATK_TEXT (accessible), offset); Do we really want to set the caret offset here? @@ +916,3 @@ +{ + ev_view_accessible_set_caret_offset (ATK_TEXT (accessible), offset); + g_signal_emit_by_name (ATK_TEXT (accessible), "text_caret_moved", offset); I think the use of underscore for signal names is deprecated, use text-caret-moved instead. g_signal_emit_by_name receives a gpointer so you don't need to cast to AtkText here. @@ +929,3 @@ atk_object_initialize (accessible, widget); + g_signal_connect (EV_VIEW (widget), "cursor-moved", Same here, g_signal_connect expects a gpointer.
Created attachment 248399 [details] [review] Emit AtkText text-caret-moved and text-selection changed signals
Comment on attachment 248399 [details] [review] Emit AtkText text-caret-moved and text-selection changed signals Pushed to git master.