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

[Larry Ewing <lewing ximian com>]

I haven't had time to look at this completely but I'll comment on the
what I've noticed so far before I get dinner.

On Wed, 2003-07-02 at 15:59, Radek Doulík wrote:
> > @@ -47,14 +48,42 @@
> >  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;
> > +}
> > +

Isn't this the same as calling html_object_engine (obj, NULL); ?

> >  void
> >  html_engine_hide_cursor  (HTMLEngine *engine)
> >  {
> > +       HTMLEngine *e = engine;
> > +
> >         g_return_if_fail (engine != NULL);
> >         g_return_if_fail (HTML_IS_ENGINE (engine));
> >  
> > -       if (engine->editable && engine->cursor_hide_count == 0)
> > -               html_engine_draw_cursor_in_area (engine, 0, 0, -1, -1);
> > +       if ((engine->editable || engine->caret_mode) && engine->cursor_hide_count == 0) {
> > +               if (!engine->editable) {
> > +                       e = get_direct_engine(engine->cursor->object);
> > +                       if (e) {
> > +                               e->caret_mode = engine->caret_mode;
> > +                               html_cursor_copy(e->cursor, engine->cursor);
> > +                       } else  e = engine;
> > +               }
> > +               html_engine_draw_cursor_in_area (e, 0, 0, -1, -1);
> > +       }
> >  
> >         engine->cursor_hide_count++;
> >  }
> > @@ -62,22 +91,37 @@
> >  void
> >  html_engine_show_cursor  (HTMLEngine *engine)
> >  {
> > +        HTMLEngine * e = engine;
> > +
> >         g_return_if_fail (engine != NULL);
> >         g_return_if_fail (HTML_IS_ENGINE (engine));
> > +       g_return_if_fail (engine->cursor != NULL);
> >  
> >         if (engine->cursor_hide_count > 0) {
> >                 engine->cursor_hide_count--;
> > -               if (engine->editable && engine->cursor_hide_count == 0)
> > -                       html_engine_draw_cursor_in_area (engine, 0, 0, -1, -1);
> > +               if ((engine->editable || engine->caret_mode) && engine->cursor_hide_count == 0) {
> > +                       if (!engine->editable) {
> > +                               e = get_direct_engine(engine->cursor->object);
> > +                               if (e) {
> > +                                       e->caret_mode = engine->caret_mode;
> > +                                       html_cursor_copy(e->cursor, engine->cursor);
> > +                               } else e = engine;
> > +                       }
> > +                       html_engine_draw_cursor_in_area (e, 0, 0, -1, -1);
> > +               }
> >         }
> >  }
> >  
> >  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;
> 
> Larry, please could you check this doesn't break clipping?

It is only used here so it shouldn't really cause problems, but I'd
probably prefer it be written something like:

static gboolean
clip_cursor (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)
		return FALSE;

	*x1 = CLAMP (*x1, x, x + width -1);
	*x2 = CLAMP (*x2, x, x + width -1);
	*y1 = CLAMP (*y1, y, y + height -1);
	*y2 = CLAMP (*y2, y, y + height -1);

	return TRUE;
}

to make the logic a little clearer. Also, looking at the code I'm not
really sure why we are passing a rectangle around as the cursor shape
when we only ever really draw a line, but that doesn't have anthing to
to do with this patch.

The rest of Radek's comments look good, I'll try to look closer at the
rest of the patch tonight.

--Larry