Re: [evolution-patches] Patch to enable cursor and keyboard navigation in mail reader,(v3) revoke previous ones

[Radek Doulík <rodo ximian com>]

Hi York,

	I am sorry for the late reply.

	Your patch looks mostly OK. The problem I see there is that your patch
enables cursor in view mode by default. I am Cc-ing Anna as it's a UI
issue. I think it will be better to enable cursor in view mode only by
keyboard binded command - similar to what Mozilla does - it uses F7
IIRC.

	Anna, what do you think?

Cheers
Radek

On Sat, 2003-06-14 at 12:46, yuedong du wrote:
> Hi all,
> 
> Sorry for bother again. The previous patch's change to 
> html_engine_update_event() is wrong, and cause crash. shy...,
> 
> Create a new patch remove this part. Tested, should be ok now.
> 
> Regards
> York
> 
> 
> On Fri, 2003-06-13 at 09:42, Yuedong Du wrote:
> > Hi all,
> > 
> > A better patch. Simpler, and robuster.
> > 
> > When blinking, previously I try to let each html engine to remember the
> > hide count and blink status. This is actually unnecessary complexity.
> > 
> > So in this patch, just use the top level html engine's blink status and
> > hide count. Only when actually draw, we need to find the embedded widget
> > if there is one. It works well.
> > 
> > Other parts left unchanged.
> > 
> > 
> > Regards
> > York
> > 
> > 
> > 
> > On Thu, 2003-06-12 at 21:01, Yuedong Du wrote:
> > > Hi all,
> > > 
> > > The patch is for evolution a11y project. See bug #44607.
> > > 
> > > According to Calum Benson's suggestion, always enable cursor. And when
> > > reading through a mail by pageup/pagedown,etc., the cursor navigate
> > > correspondingly.
> > > 
> > > There still many work left, they are expected to resolved in further
> > > patches. For example, currently cursor can navigate on almost any
> > > element. Actually some element maybe meaningless, should we remove them
> > > ? And <Tab> is not processed in this patch. We want the Tab can navigate
> > > through embedded element in the mail, ...
> > > 
> > > 
> > > Now I try to explain the attached patch, comment begin with >>>>,...
> > > 
> > > 
> > > Index: src/gtkhtml.c
> > > ===================================================================
> > > RCS file: /cvs/gnome/gtkhtml/src/gtkhtml.c,v
> > > retrieving revision 1.514
> > > diff -u -4 -r1.514 gtkhtml.c
> > > -- src/gtkhtml.c 20 May 2003 18:25:14 -0000 1.514
> > > +++ src/gtkhtml.c 12 Jun 2003 10:12:33 -0000
> > > @@ -1475,9 +1475,9 @@
> > > if (obj && ((HTML_IS_IMAGE (obj) && HTML_IMAGE (obj)->url && *HTML_IMAGE
> > > (obj)->url)
> > >     || HTML_IS_LINK_TEXT (obj)))
> > > html_engine_set_focus_object (orig_e, obj);
> > > else
> > > - html_engine_set_focus_object (orig_e, NULL);
> > > + html_engine_jump_at (engine, x, y); 
> > > }
> > > if (html->allow_selection) {
> > > if (event->state & GDK_SHIFT_MASK)
> > > html_engine_select_region (engine,
> > > 
> > > >>>> When mouse click somewhere in the mail, focus should jump to there.
> > > 
> > > @@ -3782,10 +3782,11 @@
> > > cursor_move (GtkHTML *html, GtkDirectionType dir_type,
> > > GtkHTMLCursorSkipType skip)
> > > {
> > > gint amount;
> > > 
> > > - if (!html_engine_get_editable (html->engine))
> > > + /* if (!html_engine_get_editable (html->engine))
> > > return;
> > > + */
> > > 
> > > if (html->engine->selection_mode) {
> > > if (!html->engine->mark)
> > > html_engine_set_mark (html->engine);
> > > 
> > > >>>> Not only editor now need move cursor. Just remove the check, and it
> > > works!
> > > 
> > > Index: src/htmlengine-edit-cursor.c
> > > ===================================================================
> > > RCS file: /cvs/gnome/gtkhtml/src/htmlengine-edit-cursor.c,v
> > > retrieving revision 1.22
> > > diff -u -4 -r1.22 htmlengine-edit-cursor.c
> > > -- src/htmlengine-edit-cursor.c 2 Apr 2003 05:41:47 -0000 1.22
> > > +++ src/htmlengine-edit-cursor.c 12 Jun 2003 10:12:37 -0000
> > > @@ -28,8 +28,9 @@
> > > #include "htmlengine-edit-cursor.h"
> > > #include "htmlengine-edit-table.h"
> > > #include "htmlengine-edit-tablecell.h"
> > > #include "htmlimage.h"
> > > +#include "htmliframe.h"
> > > #include "htmlobject.h"
> > > #include "htmltable.h"
> > > 
> > > #define BLINK_TIMEOUT 500
> > > @@ -46,39 +47,79 @@
> > > 
> > > static GdkColor image_stipple_active_on      = { 0, 0xffff, 0,      0 };
> > > static GdkColor image_stipple_active_off     = { 0, 0xffff, 0xffff,
> > > 0xffff };
> > > 
> > > +static HTMLEngine *
> > > +get_direct_engine(HTMLObject * obj)
> > > +{
> > > +        HTMLObject * parent;
> > > + int i = 0;
> > > +                                                                                
> > > +        parent = obj;
> > > +        while (parent) {
> > > +                if ((HTML_OBJECT_TYPE (parent) == HTML_TYPE_FRAME)
> > > +                    || (HTML_OBJECT_TYPE (parent) == HTML_TYPE_IFRAME))
> > > {
> > > +                       return html_object_get_engine (parent, NULL);
> > > +                }
> > > +                parent = parent->parent;
> > > +        }
> > > +                                                                                
> > > +        return NULL;
> > > +}
> > > +
> > > 
> > > >>>> For html mail, when cursor is in mail content, it is actually
> > > located in a embedded gtkhtml widget. We need to draw cursor on the
> > > embedded gtkhtml widget. This function just return the engine the cursor
> > > is actually in.
> > > 
> > > void
> > > html_engine_hide_cursor  (HTMLEngine *engine)
> > > {
> > > + HTMLEngine * e;
> > > +
> > > g_return_if_fail (engine != NULL);
> > > g_return_if_fail (HTML_IS_ENGINE (engine));
> > > 
> > > - if (engine->editable && engine->cursor_hide_count == 0)
> > > + if (!engine->editable) {
> > > +        e = get_direct_engine(engine->cursor->object);
> > > + if (e) engine = e;
> > > + }
> > > +
> > > + if (engine->cursor_hide_count == 0)
> > > html_engine_draw_cursor_in_area (engine, 0, 0, -1, -1);
> > > 
> > > engine->cursor_hide_count++;
> > > }
> > > 
> > > void
> > > html_engine_show_cursor  (HTMLEngine *engine)
> > > {
> > > +        HTMLEngine * e;
> > > +
> > > g_return_if_fail (engine != NULL);
> > > g_return_if_fail (HTML_IS_ENGINE (engine));
> > > + g_return_if_fail (engine->cursor != NULL);
> > > +
> > > + if (!engine->editable) {
> > > +        e = get_direct_engine(engine->cursor->object);
> > > + if (e) {
> > > +                html_cursor_copy(e->cursor, engine->cursor);
> > > + engine = e;
> > > + }
> > > + }
> > > 
> > > if (engine->cursor_hide_count > 0) {
> > > engine->cursor_hide_count--;
> > > - if (engine->editable && engine->cursor_hide_count == 0)
> > > + if (engine->cursor_hide_count == 0)
> > > html_engine_draw_cursor_in_area (engine, 0, 0, -1, -1);
> > > }
> > > }
> > > 
> > > >>>> As explained by last comment, show/hide cursor may actually happen
> > > on a embedded gtkhtml widget, so we need to find the engine before
> > > actually draw/hide.
> > > 
> > > static gboolean
> > > clip_rect (HTMLEngine *engine, gint x, gint y, gint width, gint height,
> > > gint *x1, gint *y1, gint *x2, gint *y2)
> > > {
> > > - if (*x1 >= x + width || *y1 >= y + height || *x2 < x || *y2 < y)
> > > + if (*x1 > x + width || *y1 > y + height || *x2 < x || *y2 < y)
> > > return FALSE;
> > > 
> > > + if (*x1 == x + width)
> > > + *x1 = x + width - 1;
> > > + if (*y1 == y + width)
> > > + *y1 = y + height - 1;
> > > if (*x2 >= x + width)
> > > *x2 = x + width - 1;
> > > if (*y2 >= y + height)
> > > *y2 = y + height - 1;
> > > 
> > > >>>> clip_rect() may fail if draw a cursor at end of a document. Here we
> > > take care of this case.
> > > 
> > > @@ -264,17 +305,15 @@
> > > guint offset;
> > > gint x1, y1, x2, y2;
> > > GdkRectangle pos;
> > > 
> > > - g_assert (engine->editable);
> > > -
> > > - if (engine->editable && (engine->cursor_hide_count <= 0 &&
> > > !engine->thaw_idle_id)) {
> > > + if ((engine->cursor_hide_count <= 0 && !engine->thaw_idle_id)) {
> > > html_engine_draw_table_cursor (engine);
> > > html_engine_draw_cell_cursor (engine);
> > > html_engine_draw_image_cursor (engine);
> > > }
> > > 
> > > - if (!cursor_enabled || engine->cursor_hide_count > 0 || !
> > > engine->editable || engine->thaw_idle_id)
> > > + if (!cursor_enabled || engine->cursor_hide_count > 0 ||
> > > engine->thaw_idle_id)
> > > return;
> > > 
> > > obj = engine->cursor->object;
> > > if (obj == NULL)
> > > @@ -288,17 +327,26 @@
> > > x = 0;
> > > y = 0;
> > > }
> > > 
> > > - 
> > > html_object_get_cursor (obj, engine->painter, offset, &x1, &y1, &x2,
> > > &y2);
> > > - 
> > > pos.x = x1; 
> > > pos.y = y1;
> > > pos.width = x2 - x1;
> > > pos.height = x2 - x1;
> > > gtk_im_context_set_cursor_location (GTK_HTML
> > > (engine->widget)->priv->im_context, &pos);
> > > 
> > > + obj =
> > > g_object_get_data(G_OBJECT(gtk_widget_get_parent(GTK_WIDGET(engine->widget))), "embeddedelement"); 
> > > + if (obj) {
> > > + HTMLEmbedded * element;
> > > +
> > > + element = HTML_EMBEDDED(obj);
> > > + x1 -= element->abs_x;
> > > + x2 -= element->abs_x;
> > > + y1 -= element->abs_y;
> > > + y2 -= element->abs_y;
> > > + }
> > > +
> > > if (clip_rect (engine, x, y, width, height, &x1, &y1, &x2, &y2)) {
> > > gdk_draw_line (engine->window, engine->invert_gc, x1, y1, x2, y2);
> > > }
> > > }
> > > 
> > > >>>> When drawing a cursor,html_object_get_cursor() returned location of
> > > the cursor is relative the top level gtkhtml widget. So when the cursor
> > > actually on a embedded gtkhtml widget, we need to subtract the offset of
> > > embedded gtkhtml widget to the top level gtkthml widget.
> > > 
> > > @@ -308,13 +356,18 @@
> > > 
> > > static gint
> > > blink_timeout_cb (gpointer data)
> > > {
> > > - HTMLEngine *engine;
> > > + HTMLEngine *engine, *e;
> > > 
> > > g_return_val_if_fail (HTML_IS_ENGINE (data), FALSE);
> > > 
> > > engine = HTML_ENGINE (data);
> > > +
> > > + if (!engine->editable) {
> > > + e = get_direct_engine(engine->cursor->object);
> > > + if (e) engine = e;
> > > + }
> > > 
> > > engine->blinking_status = ! engine->blinking_status;
> > > 
> > > if (engine->blinking_status)
> > > 
> > > >>>> The blinking status is remembered on each engine. We use
> > > get_direct_engine() to find it. If we do not find the right one, we may
> > > do wrong html_engine_cursor_show()/html_engine_cursor_hide(). Thus it is
> > > possible cursor's hide counter wrong, and cursor disappear. We need to
> > > find the engine cursor is currently on. 
> > > 
> > > 
> > > Index: src/htmlengine-edit-movement.c
> > > ===================================================================
> > > RCS file: /cvs/gnome/gtkhtml/src/htmlengine-edit-movement.c,v
> > > retrieving revision 1.15
> > > diff -u -4 -r1.15 htmlengine-edit-movement.c
> > > -- src/htmlengine-edit-movement.c 25 Apr 2001 21:31:53 -0000 1.15
> > > +++ src/htmlengine-edit-movement.c 12 Jun 2003 12:40:07 -0000
> > > @@ -274,10 +274,12 @@
> > >    now.  */
> > > if (new_y == y)
> > > break;
> > > 
> > > - if (new_y < start_y)
> > > + if (new_y < start_y) {
> > > + html_engine_show_cursor (engine);
> > > return 0;
> > > + }
> > > 
> > > if (new_y - start_y >= amount) {
> > > html_cursor_copy (cursor, &prev_cursor);
> > > break;
> > > 
> > > >>>> Ensure the show_cursor()/hide_cursor() always called by pairs.
> > > 
> > > Index: src/htmlengine.c
> > > ===================================================================
> > > RCS file: /cvs/gnome/gtkhtml/src/htmlengine.c,v
> > > retrieving revision 1.542
> > > diff -u -4 -r1.542 htmlengine.c
> > > -- src/htmlengine.c 19 May 2003 19:10:36 -0000 1.542
> > > +++ src/htmlengine.c 12 Jun 2003 12:40:33 -0000
> > > @@ -3930,15 +3930,15 @@
> > > DI (printf ("html_engine_update_event idle %p\n", e);)
> > > 
> > > e->updateTimer = 0;
> > > 
> > > - if (html_engine_get_editable (e))
> > > - html_engine_hide_cursor (e);
> > > + html_engine_hide_cursor (e);
> > > html_engine_calc_size (e, FALSE);
> > > 
> > > if (GTK_LAYOUT (e->widget)->vadjustment == NULL
> > >     || ! html_gdk_painter_realized (HTML_GDK_PAINTER (e->painter))) {
> > > e->need_update = TRUE;
> > > + html_engine_show_cursor (e);
> > > return FALSE;
> > > }
> > > 
> > > e->need_update = FALSE;
> > > @@ -3975,10 +3975,9 @@
> > > html_image_factory_deactivate_animations (e->image_factory);
> > > gtk_container_forall (GTK_CONTAINER (e->widget), update_embedded,
> > > e->widget);
> > > html_engine_queue_redraw_all (e);
> > > 
> > > - if (html_engine_get_editable (e))
> > > - html_engine_show_cursor (e);
> > > + html_engine_show_cursor (e);
> > > 
> > > return FALSE;
> > > }
> > > 
> > > @@ -4579,15 +4578,12 @@
> > > if (editable) {
> > > html_engine_ensure_editable (e);
> > > html_cursor_home (e->cursor, e);
> > > e->newPage = FALSE;
> > > + } 
> > > 
> > > - if (e->have_focus)
> > > - html_engine_setup_blinking_cursor (e);
> > > - } else {
> > > - if (e->have_focus)
> > > - html_engine_stop_blinking_cursor (e);
> > > - }
> > > + if (e->have_focus)
> > > + html_engine_setup_blinking_cursor (e);
> > > }
> > > 
> > > gboolean
> > > html_engine_get_editable (HTMLEngine *e)
> > > @@ -4616,14 +4612,12 @@
> > > {
> > > g_return_if_fail (engine != NULL);
> > > g_return_if_fail (HTML_IS_ENGINE (engine));
> > > 
> > > - if (engine->editable) {
> > > - if (! engine->have_focus && have_focus)
> > > - html_engine_setup_blinking_cursor (engine);
> > > - else if (engine->have_focus && ! have_focus)
> > > - html_engine_stop_blinking_cursor (engine);
> > > - }
> > > + if (! engine->have_focus && have_focus)
> > > + html_engine_setup_blinking_cursor (engine);
> > > + else if (engine->have_focus && ! have_focus)
> > > + html_engine_stop_blinking_cursor (engine);
> > > 
> > > engine->have_focus = have_focus;
> > > 
> > > html_painter_set_focus (engine->painter, engine->have_focus);
> > > @@ -4656,10 +4650,12 @@
> > > gint x1, y1, x2, y2, xo, yo;
> > > 
> > > g_return_val_if_fail (e != NULL, FALSE);
> > > 
> > > + /*
> > > if (! e->editable)
> > > return FALSE;
> > > + */
> > > 
> > > if (e->cursor->object == NULL)
> > > return FALSE;
> > > 
> > > >>>> Just enable cursor and blinking even when reading.
> > > 
> > > Index: src/htmliframe.c
> > > ===================================================================
> > > RCS file: /cvs/gnome/gtkhtml/src/htmliframe.c,v
> > > retrieving revision 1.74
> > > diff -u -4 -r1.74 htmliframe.c
> > > -- src/htmliframe.c 19 May 2003 13:12:04 -0000 1.74
> > > +++ src/htmliframe.c 12 Jun 2003 12:40:34 -0000
> > > @@ -591,8 +591,10 @@
> > > html_iframe_set_scrolling (iframe, GTK_POLICY_AUTOMATIC);
> > > 
> > > new_widget = gtk_html_new ();
> > > new_html = GTK_HTML (new_widget);
> > > + /* Hacking, make it show cursor. */
> > > + new_html->engine->cursor_hide_count = 0;
> > > 
> > > new_tokenizer = html_tokenizer_clone (parent_html->engine->ht);
> > > 
> > > html_engine_set_tokenizer (new_html->engine, new_tokenizer);
> > > 
> > > 
> > > Best Regards
> > > York