Re: [evolution-patches] GTKHTML Patch for #306238 (new)

[Mengjie Yu <Meng-Jie Yu Sun COM>]

Dear kaushal,

Thanks a lot about your review and comment.

Please see my explanation inline below.

On Tue, 2005-06-28 at 19:13 +0530, Kaushal Kumar wrote:
> Hi Mengjie,
> 
> sorry about some delay in review. I was busy completing the remaining 
> GAL retirement work.
> 
> > http://bugzilla.gnome.org/show_bug.cgi?id=306238
> > The patch for this bug is also available on the above URL.
> > 
> > As to your proposal, I signal the cursor position and get the offset of the text
> > inserted or deleted in the a11y part. 
> Okay.
> 
> > 
> > Because gnopernicus need to check the text deleted before it is really deleted,
> > I emit the text_changed::delete signal before the deleing really happens.
> Fine.
> 
> Please find some comments inline below:-
> 
> > @@ -362,7 +366,7 @@ gtk_html_a11y_insert_object_cb (GtkWidge
> >  	}
> >  
> >  	if (G_IS_HTML_A11Y_TEXT(a11y)) {
> > -		g_signal_emit_by_name (a11y, "text_changed::insert", pos, len);
> > +		g_signal_emit_by_name (a11y, "text_changed::insert", cursor->offset - len, len);
> 
> Would the value of cursor->offset - len be passed as negative when we 
> are at the beginning of the line and delete a character. (please refer 
> my comments in the bug report for a similar situation in the last patch).
> 
the value of 'cursor->offset - len' could not be negative because when I
emit the insert signal, the cursor->offset has been set to the offset
after the insering. Actually, 'cursor->offset - len' means the offset
from which we insert the text. We use 'curosr->offset - len' and  len to
identify the start_offset and end_offset of the text been inserted.

On the other hand, when we delete a character, we will emit the
text::delete signal, this will invoke another callback but not our 
gtk_html_a11y_insert_object_cb.

> >  
> >  	} 
> >  }
> > @@ -371,6 +375,7 @@ static void
> >  gtk_html_a11y_delete_object_cb (GtkWidget * widget, int pos, int len) 
> >  {
> >  	AtkObject * a11y, *obj;
> > +	HTMLCursor *cursor = GTK_HTML (widget)->engine->cursor;
> >  
> >          obj = gtk_widget_get_accessible (widget);
> >  	a11y = gtk_html_a11y_get_focus_object (widget);
> > @@ -383,7 +388,7 @@ gtk_html_a11y_delete_object_cb (GtkWidge
> >  	}
> >  
> >  	if (G_IS_HTML_A11Y_TEXT(a11y)) {
> > -		g_signal_emit_by_name (a11y, "text_changed::delete", pos, len);
> > +		g_signal_emit_by_name (a11y, "text_changed::delete", cursor->offset, len);
> do we need to minus len from cursor->offset being passed here?
We do not need to minus len from curor->offset here because we are in
callback gtk_html_a11y_delete_object_cb which connects to text::delete
signal.

Here, cursor->offset means the start_offset of the text being deleted.
while len means the length of the text being deleted, therefore,
'cursor->offset + len' here means the end_offset of the text being
deleted.

Btw: although I emit the text_changed::delete signal before the deleing
really happens. The offset of the cursor at that moment has been changed
to the proper offset after the deleting. So we don't need to minus len
here.
> 
> >  	}
> >  }
> >  
> > Index: text.c
> > ===================================================================
> > RCS file: /cvs/gnome/gtkhtml/a11y/text.c,v
> > retrieving revision 1.15
> > diff -u -p -r1.15 text.c
> > --- text.c	17 May 2005 14:47:56 -0000	1.15
> > +++ text.c	21 Jun 2005 09:12:10 -0000
> > @@ -532,29 +532,39 @@ html_a11y_text_get_text_at_offset (AtkTe
> >  	switch (boundary_type) {
> >  	case ATK_TEXT_BOUNDARY_LINE_START:
> >  		start_slave = html_text_get_slave_at_offset (to, NULL, offset);
> > -		g_return_val_if_fail (start_slave, NULL);
> > -		end_slave = (HTMLTextSlave *) HTML_OBJECT (start_slave)->next;
> >  
> > -		if (end_slave && HTML_IS_TEXT_SLAVE (end_slave)) {
> > -			*end_offset = end_slave->posStart;
> > +		if (start_slave == NULL) {
> > +			*start_offset = 0;
> > +			*end_offset = to->text_len;
> could you elaborate on these offset values set as above. Just needed to 
> understand it better. Earlier we used to return on a null start_slave. 
> Now, we proceed with these values.
> - can we proceed with a null start_slave instead of returning null?
> - how do we decide on these preset values of offsets?
Actually, it aims to fix another trivial bug. Sometimes, in a single
line, the text exists while its start_slave doesn't exist. On this
situation and in this function, we still need to get the text, so I
preset these values for this situation.

> 
> >  	case ATK_TEXT_BOUNDARY_LINE_END:
> >  		end_slave = html_text_get_slave_at_offset (to, NULL, offset);
> > -		g_return_val_if_fail (end_slave, NULL);
> > -		start_slave = (HTMLTextSlave *) HTML_OBJECT (end_slave)->prev;
> >  
> > -		if (start_slave && HTML_IS_TEXT_SLAVE (start_slave)) {
> > -			*start_offset = start_slave->posStart + start_slave->posLen;
> > +		if (end_slave == NULL) {
> > +			*start_offset = 0;
> > +			*end_offset = to->text_len;
> 
> same query as above for this change.
> 
> > --- htmlengine-edit-cut-and-paste.c	13 Apr 2005 15:35:32 -0000	1.111
> > +++ htmlengine-edit-cut-and-paste.c	21 Jun 2005 08:57:38 -0000
> > @@ -1769,6 +1769,10 @@ html_engine_delete (HTMLEngine *e)
> >  		gint start_position = start->position;
> >  		gint end_position = end->position;
> >  
> > +		if (end_position - start_position > 0) {
> > +			int len = end_position - start_position;
> > +			g_signal_emit_by_name (e->widget, "object_deleted", start_position, len);
> > +		}
> >  
> So we emit the object_deleted signal before the delete actually happens.
> Radek: Would you kindly give your opinion. Thanks.
> 
> >  		while (start->position < end->position) {
> >  			if (check_for_simple_delete (start->object, end->object)) {
> 
> -- 
> Kaushal Kumar  <kakumar novell com>
> _______________________________________________
> evolution-patches mailing list
> evolution-patches lists ximian com
> http://lists.ximian.com/mailman/listinfo/evolution-patches