Re: [Emacs-diffs] xwidget updated (1d8b8a2 -> 5f46725)

Re: [Emacs-diffs] xwidget updated (1d8b8a2 -> 5f46725)

[Stefan Monnier]

Hi Joakim,

BTW, what's the status of this branch (w.r.t to its mergeability into master)?


        Stefan

Re: [Emacs-diffs] xwidget updated (1d8b8a2 -> 5f46725)

[joakim]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Hi Joakim,
>
> BTW, what's the status of this branch (w.r.t to its mergeability into master)?

Short answer: I'm trying to figure that out now.

Longer answer:

I think its pretty okay. Theres a problem with automatic
resizing of webkit that sometimes get so big emacs crashe I would like
to fix though.

I'm not sure how to do the actual merge. I think cherry picking or
something would be better than a plain merge.

Also, since I'm pretty blind to the flaws the code has by now, it would
be nice with some maintainer criticism.




>
>
>         Stefan

-- 
Joakim Verona

Re: [Emacs-diffs] xwidget updated (1d8b8a2 -> 5f46725)

[Eli Zaretskii]

> From: joakim@verona.se
> Date: Sat, 27 Dec 2014 16:48:44 +0100
> Cc: emacs-devel@gnu.org
> 
> Stefan Monnier <address@hidden> writes:
> 
> > Hi Joakim,
> >
> > BTW, what's the status of this branch (w.r.t to its mergeability into master)?
> 
> Short answer: I'm trying to figure that out now.
> 
> Longer answer:
> 
> I think its pretty okay. Theres a problem with automatic
> resizing of webkit that sometimes get so big emacs crashe I would like
> to fix though.
> 
> I'm not sure how to do the actual merge. I think cherry picking or
> something would be better than a plain merge.
> 
> Also, since I'm pretty blind to the flaws the code has by now, it would
> be nice with some maintainer criticism.

Thank you for your work.  Please find a few comments and questions
below, all based solely on reading the source.

1) In dispextern.h:'struct it' you made this addition to the iterator
   structure:

  @@ -500,6 +504,9 @@ struct glyph
       /* Image ID for image glyphs (type == IMAGE_GLYPH).  */
       int img_id;

  +#ifdef HAVE_XWIDGETS
  +    struct xwidget* xwidget;
  +#endif
       /* Sub-structure for type == STRETCH_GLYPH.  */
       struct
       {

  This might be a problem, because several places in the display
  engine make local copies of the 'struct it' object, which will
  duplicate the pointer you added, so you will have 2 or more pointers
  to the same object.  If one of the copies of the pointer is used to
  modify the 'struct xwidget' object, or free it, the other copies
  will be affected, which the code doesn't expect.  Note that images,
  for example, store only their numerical ID in the iterator
  structure, not a direct pointer to an image.

  Also, you added a similar pointer to the iterator stack entry:

  @@ -2379,6 +2396,13 @@ struct it
	 struct {
	  Lisp_Object object;
	 } stretch;
  +#ifdef HAVE_XWIDGETS
  +      /* method == GET_FROM_XWIDGET */
  +      struct {
  +       Lisp_Object object;
  +        struct xwidget* xwidget;
  +      } xwidget;
  +#endif
       } u;

   But that pointer seems to be unused, so I guess it should be
   deleted.

2) In dispnew.c:update_window you added a call to
   xwidget_end_redisplay.  I think this call should be made before we
   call update_window_end_hook, because when that call is made, the
   redisplay interface implementation assumes the window is already up
   to date, whereas xwidget_end_redisplay still manipulates portions
   of the display (AFAIU).

3) A few places (for example, xdisp.c:handle_single_display_spec)
   process xwidget display elements even on non-GUI frames -- does
   that mean xwidget.c will be compiled even in --without-x
   configurations of Emacs?  If not, you need to condition that code
   on HAVE_WINDOW_SYSTEM, like we do with images, for example.

4) xdisp.c:produce_xwidget_glyph seems to need some cleanup: it's
   basically a copy of produce_image_glyph, and at least some of the
   code there is not needed with xwidgets, I think.
   OTOH, if indeed xwidgets are very similar to images, perhaps we
   should have only one method that handles both.

5) xdisp.c:produce_xwidget_glyph needs to account for bidirectional
   display in the same way produce_image_glyph does: swao the left and
   right box edges, and populate the bidi members of struct glyph.

6) Did you test what happens with xwidgets when the lines are
   truncated, and only part of the xwidget fits on the line?  Are the
   results reasonable?  I see that produce_xwidget_glyph does attempt
   to crop the xwidget to fit in the line, but then display_line
   should handle xwidget glyphs the same as it does with image glyphs,
   when it decides how to go about truncation/continuation.

7) xwidget.c:make-xwidget seems to support xwidgets only in a buffer.
   What about strings?  If strings aren't going to be supported, then
   the 'object' member of the iterator stack entry for xwidgets is not
   needed.

8) Do we really need to expose xwgir-require-namespace?  Can't
   something like that be done automatically under the hood?

9) xwgir-xwidget-call-method needs the method as a string.  Wouldn't
   it be better to use a symbol here?  Strings are more expensive to
   compare, e.g. if some code needs to manage methods.

10) Several places in xwidget.c use Lisp string data without first
    verifying it's a string.  Examples include
    xwidget-webkit-execute-script and xwidget-webkit-goto-uri.

11) The doc strings of functions exposed to Lisp that are defined on
    xwidget.c are not yet finished.

12) A question about configuration: are xwidgets only supported in a
    GTK3 compiled Emacs, or also in other builds?

13) A minor stylistic nit: the code style is somewhat different from
    the GNU Coding Standard: no space between the function name and
    the left parentheses that follows, opening brace of a block at
    the end of a line rather than on the next line, comments that
    don't end with a period, etc.

14) Finally, there are a lot of places in the code with FIXME's,
    TODO's, fragments that are commented out, debugging printf's, and
    other left-overs that I suggest to clean up before the merge.

Thanks again for working on this.

Re: [Emacs-diffs] xwidget updated (1d8b8a2 -> 5f46725)

[joakim]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: joakim@verona.se
>> Date: Sat, 27 Dec 2014 16:48:44 +0100
>> Cc: emacs-devel@gnu.org
>> 
>> Stefan Monnier <address@hidden> writes:
>> 
>> > Hi Joakim,
>> >
>> > BTW, what's the status of this branch (w.r.t to its mergeability into master)?
>> 
>> Short answer: I'm trying to figure that out now.
>> 
>> Longer answer:
>> 
>> I think its pretty okay. Theres a problem with automatic
>> resizing of webkit that sometimes get so big emacs crashe I would like
>> to fix though.
>> 
>> I'm not sure how to do the actual merge. I think cherry picking or
>> something would be better than a plain merge.
>> 
>> Also, since I'm pretty blind to the flaws the code has by now, it would
>> be nice with some maintainer criticism.
>
> Thank you for your work.  Please find a few comments and questions
> below, all based solely on reading the source.

I'm extatic that you found time to review it! Thanks Eli!

Giving a thorough reply will take time, so I just include replies I
could give quickly below.

>
> 1) In dispextern.h:'struct it' you made this addition to the iterator
>    structure:
>
>   @@ -500,6 +504,9 @@ struct glyph
>        /* Image ID for image glyphs (type == IMAGE_GLYPH).  */
>        int img_id;
>
>   +#ifdef HAVE_XWIDGETS
>   +    struct xwidget* xwidget;
>   +#endif
>        /* Sub-structure for type == STRETCH_GLYPH.  */
>        struct
>        {
>
>   This might be a problem, because several places in the display
>   engine make local copies of the 'struct it' object, which will
>   duplicate the pointer you added, so you will have 2 or more pointers
>   to the same object.  If one of the copies of the pointer is used to
>   modify the 'struct xwidget' object, or free it, the other copies
>   will be affected, which the code doesn't expect.  Note that images,
>   for example, store only their numerical ID in the iterator
>   structure, not a direct pointer to an image.
>
>   Also, you added a similar pointer to the iterator stack entry:
>
>   @@ -2379,6 +2396,13 @@ struct it
> 	 struct {
> 	  Lisp_Object object;
> 	 } stretch;
>   +#ifdef HAVE_XWIDGETS
>   +      /* method == GET_FROM_XWIDGET */
>   +      struct {
>   +       Lisp_Object object;
>   +        struct xwidget* xwidget;
>   +      } xwidget;
>   +#endif
>        } u;
>
>    But that pointer seems to be unused, so I guess it should be
>    deleted.
>
> 2) In dispnew.c:update_window you added a call to
>    xwidget_end_redisplay.  I think this call should be made before we
>    call update_window_end_hook, because when that call is made, the
>    redisplay interface implementation assumes the window is already up
>    to date, whereas xwidget_end_redisplay still manipulates portions
>    of the display (AFAIU).
>
> 3) A few places (for example, xdisp.c:handle_single_display_spec)
>    process xwidget display elements even on non-GUI frames -- does
>    that mean xwidget.c will be compiled even in --without-x
>    configurations of Emacs?  If not, you need to condition that code
>    on HAVE_WINDOW_SYSTEM, like we do with images, for example.

No the code shouldn't be compiled  if we dont
HAVE_WINDOW_SYSTEM. Thanks for the catch!

> 4) xdisp.c:produce_xwidget_glyph seems to need some cleanup: it's
>    basically a copy of produce_image_glyph, and at least some of the
>    code there is not needed with xwidgets, I think.
>    OTOH, if indeed xwidgets are very similar to images, perhaps we
>    should have only one method that handles both.
>
> 5) xdisp.c:produce_xwidget_glyph needs to account for bidirectional
>    display in the same way produce_image_glyph does: swao the left and
>    right box edges, and populate the bidi members of struct glyph.

I havent thought about bidi at all. Do you have a simple test case?

> 6) Did you test what happens with xwidgets when the lines are
>    truncated, and only part of the xwidget fits on the line?  Are the
>    results reasonable?  I see that produce_xwidget_glyph does attempt
>    to crop the xwidget to fit in the line, but then display_line
>    should handle xwidget glyphs the same as it does with image glyphs,
>    when it decides how to go about truncation/continuation.

Truncation works same as for an image, which I think is reasonable. Or
did you mean something else?

A quick way to test it:
- firstt create a brower xwidget, point it to fsf
m-x xwidget-browse-url RET http://www.fsf.org RET
- then switch to fundamental mode. turn off read-only. insert some chars
before the xwidget. the xwidget will scroll right, and get truncated
when it hits the frame border.

I dont understand your comment about display_line.




> 7) xwidget.c:make-xwidget seems to support xwidgets only in a buffer.
>    What about strings?  If strings aren't going to be supported, then
>    the 'object' member of the iterator stack entry for xwidgets is not
>    needed.

Hmm okay.

> 8) Do we really need to expose xwgir-require-namespace?  Can't
>    something like that be done automatically under the hood?

The xwgir stuff is eventually supposed to work like an FFI. So something
like it will be needed. 

> 9) xwgir-xwidget-call-method needs the method as a string.  Wouldn't
>    it be better to use a symbol here?  Strings are more expensive to
>    compare, e.g. if some code needs to manage methods.

Okay.

> 10) Several places in xwidget.c use Lisp string data without first
>     verifying it's a string.  Examples include
>     xwidget-webkit-execute-script and xwidget-webkit-goto-uri.

Yes, I'm lazy, sorry.

>
> 11) The doc strings of functions exposed to Lisp that are defined on
>     xwidget.c are not yet finished.

Yes.

> 12) A question about configuration: are xwidgets only supported in a
>     GTK3 compiled Emacs, or also in other builds?

xwidgets were originally developed on GTK2, then ported to GTK3. The
code only works on GTK3 now, so theres lots of potential cleanup.

AFAICS theres no real obstacle for getting xwidgets to work on whatever
windowing system. Off screen rendering, and some other things would be
needed, but I think most modern toolkits support that.

OTOH, I think it would perhaps be easier to just use GTK3 on the target
build.

Eli, how difficult do you suppose it would be to get a GTK3 emacs
running on Windows?


> 13) A minor stylistic nit: the code style is somewhat different from
>     the GNU Coding Standard: no space between the function name and
>     the left parentheses that follows, opening brace of a block at
>     the end of a line rather than on the next line, comments that
>     don't end with a period, etc.

Okay.

> 14) Finally, there are a lot of places in the code with FIXME's,
>     TODO's, fragments that are commented out, debugging printf's, and
>     other left-overs that I suggest to clean up before the merge.
>

Yes.

> Thanks again for working on this.

And thanks again for the review!

-- 
Joakim Verona

Re: [Emacs-diffs] xwidget updated (1d8b8a2 -> 5f46725)

[Ulrich Mueller]

>>>>> On Mon, 29 Dec 2014, joakim  wrote:

>> 3) A few places (for example, xdisp.c:handle_single_display_spec)
>> process xwidget display elements even on non-GUI frames -- does
>> that mean xwidget.c will be compiled even in --without-x
>> configurations of Emacs?  If not, you need to condition that code
>> on HAVE_WINDOW_SYSTEM, like we do with images, for example.

> No the code shouldn't be compiled  if we dont
> HAVE_WINDOW_SYSTEM. Thanks for the catch!

Currently compilation in the xwidget branch also fails when configure
was executed with the --without-xwidgets option:

   In toplevel form:
   xwidget-test.el:33:7:Error: Symbol's function definition is void: get-buffer-xwidgets
   Makefile:273: recipe for target 'xwidget-test.elc' failed

Maybe byte-compilation of this file (or of all xwidget*.el) should be
made conditional on the HAVE_XWIDGETS flag?

>> 12) A question about configuration: are xwidgets only supported in
>> a GTK3 compiled Emacs, or also in other builds?

> xwidgets were originally developed on GTK2, then ported to GTK3. The
> code only works on GTK3 now, so theres lots of potential cleanup.

> AFAICS theres no real obstacle for getting xwidgets to work on
> whatever windowing system. Off screen rendering, and some other
> things would be needed, but I think most modern toolkits support
> that.

> OTOH, I think it would perhaps be easier to just use GTK3 on the
> target build.

Is there any chance to get the code working for lucid or motif? (The
gtk2 and gtk3 builds still suffer from crashes when an X connection is
closed in a multi-tty setup.)

Ulrich

Re: [Emacs-diffs] xwidget updated (1d8b8a2 -> 5f46725)

[Eli Zaretskii]

> Date: Mon, 29 Dec 2014 14:46:04 +0100
> Cc: Eli Zaretskii <eliz@gnu.org>, monnier@iro.umontreal.ca,
>         emacs-devel@gnu.org
> From: Ulrich Mueller <ulm@gentoo.org>
> 
> Currently compilation in the xwidget branch also fails when configure
> was executed with the --without-xwidgets option:
> 
>    In toplevel form:
>    xwidget-test.el:33:7:Error: Symbol's function definition is void: get-buffer-xwidgets
>    Makefile:273: recipe for target 'xwidget-test.elc' failed
> 
> Maybe byte-compilation of this file (or of all xwidget*.el) should be
> made conditional on the HAVE_XWIDGETS flag?

No, I think the *.el files should be fixed to not cause errors even if
HAVE_XWIDGETS is not defined.

Re: [Emacs-diffs] xwidget updated (1d8b8a2 -> 5f46725)

[joakim]

Ulrich Mueller <ulm@gentoo.org> writes:

>>>>>> On Mon, 29 Dec 2014, joakim  wrote:
>
>>> 3) A few places (for example, xdisp.c:handle_single_display_spec)
>>> process xwidget display elements even on non-GUI frames -- does
>>> that mean xwidget.c will be compiled even in --without-x
>>> configurations of Emacs?  If not, you need to condition that code
>>> on HAVE_WINDOW_SYSTEM, like we do with images, for example.
>
>> No the code shouldn't be compiled  if we dont
>> HAVE_WINDOW_SYSTEM. Thanks for the catch!
>
> Currently compilation in the xwidget branch also fails when configure
> was executed with the --without-xwidgets option:
>
>    In toplevel form:
>    xwidget-test.el:33:7:Error: Symbol's function definition is void: get-buffer-xwidgets
>    Makefile:273: recipe for target 'xwidget-test.elc' failed
>
> Maybe byte-compilation of this file (or of all xwidget*.el) should be
> made conditional on the HAVE_XWIDGETS flag?
>
>>> 12) A question about configuration: are xwidgets only supported in
>>> a GTK3 compiled Emacs, or also in other builds?
>
>> xwidgets were originally developed on GTK2, then ported to GTK3. The
>> code only works on GTK3 now, so theres lots of potential cleanup.
>
>> AFAICS theres no real obstacle for getting xwidgets to work on
>> whatever windowing system. Off screen rendering, and some other
>> things would be needed, but I think most modern toolkits support
>> that.
>
>> OTOH, I think it would perhaps be easier to just use GTK3 on the
>> target build.
>
> Is there any chance to get the code working for lucid or motif? (The
> gtk2 and gtk3 builds still suffer from crashes when an X connection is
> closed in a multi-tty setup.)

I dont know those toolkits very well and I dont know if they support the
required feature of offscreen rendering. At any rate it would be a lot
of work to implement support for another toolkit.


> Ulrich

-- 
Joakim Verona

Re: [Emacs-diffs] xwidget updated (1d8b8a2 -> 5f46725)

[Eli Zaretskii]

> From: joakim@verona.se
> Cc: monnier@iro.umontreal.ca,  emacs-devel@gnu.org
> Date: Mon, 29 Dec 2014 10:48:00 +0100
> 
> I'm extatic that you found time to review it! Thanks Eli!

That's the least we could do to help you finish the job.

> > 5) xdisp.c:produce_xwidget_glyph needs to account for bidirectional
> >    display in the same way produce_image_glyph does: swao the left and
> >    right box edges, and populate the bidi members of struct glyph.

> I havent thought about bidi at all.

Just look at what produce_image_glyph does, it should tell you enough.

> Do you have a simple test case [for bidi]?

Type a few Arabic or Hebrew characters, then insert a widget, then
type some more Arabic or Hebrew.  Try this both in a buffer that has
bidi-paragraph-direction set to nil and in a buffer that has it set to
left-to-right or right-to-left.

> > 6) Did you test what happens with xwidgets when the lines are
> >    truncated, and only part of the xwidget fits on the line?  Are the
> >    results reasonable?  I see that produce_xwidget_glyph does attempt
> >    to crop the xwidget to fit in the line, but then display_line
> >    should handle xwidget glyphs the same as it does with image glyphs,
> >    when it decides how to go about truncation/continuation.
> 
> Truncation works same as for an image, which I think is reasonable. Or
> did you mean something else?

No, this is what I meant.

> I dont understand your comment about display_line.

My comment has to deal with truncation marks, when the lines are
truncated and the widget gets truncated as result.  Around line 20690
in xdisp.c (this is from master; the number could be different on your
branch), you will see this code fragment:

      /* If we truncate lines, we are done when the last displayed
	 glyphs reach past the right margin of the window.  */
      if (it->line_wrap == TRUNCATE
	  && ((FRAME_WINDOW_P (it->f)
	       /* Images are preprocessed in produce_image_glyph such
		  that they are cropped at the right edge of the
		  window, so an image glyph will always end exactly at
		  last_visible_x, even if there's no right fringe.  */
	       && ((row->reversed_p
		    ? WINDOW_LEFT_FRINGE_WIDTH (it->w)
		    : WINDOW_RIGHT_FRINGE_WIDTH (it->w))
		   || it->what == IT_IMAGE))
	      ? (it->current_x >= it->last_visible_x)
	      : (it->current_x > it->last_visible_x)))
	{
	  /* Maybe add truncation glyphs.  */

It currently treats specially only image glyphs, for the reasons
explained in the comment.  Based on your response and on what I saw in
the code, the xwidget glyphs should be handled in the same manner
there.

To see this code in action, turn on truncate-lines, disable the
fringes, and see if you get the truncation glyph when the line ends in
an xwidget (you shouldn't).

> Eli, how difficult do you suppose it would be to get a GTK3 emacs
> running on Windows?

I don't know.  A Windows port of GTK3 does exist (see
http://www.gtk.org/download/win32_tutorial.php), but the GTK related
code in Emacs was never tried on Windows, so Someone(TM) will have to
get their hands dirty before it will work.  I'm guessing that the
event loop will give that Someone the most grief, unless the GTK folks
already solved that.

Alternatively, Someone Else(TM) could port your xwidget code to the
corresponding Windows features, but it will take a Windows GUI expert
to do that, and I don't think we have such a person on board.

> And thanks again for the review!

You are welcome.

Re: [Emacs-diffs] xwidget updated (1d8b8a2 -> 5f46725)

[joakim]

Eli Zaretskii <eliz@gnu.org> writes:

>
>> > 5) xdisp.c:produce_xwidget_glyph needs to account for bidirectional
>> >    display in the same way produce_image_glyph does: swao the left and
>> >    right box edges, and populate the bidi members of struct glyph.
>
>> I havent thought about bidi at all.
>
> Just look at what produce_image_glyph does, it should tell you enough.
>
>> Do you have a simple test case [for bidi]?
>
> Type a few Arabic or Hebrew characters, then insert a widget, then
> type some more Arabic or Hebrew.  Try this both in a buffer that has
> bidi-paragraph-direction set to nil and in a buffer that has it set to
> left-to-right or right-to-left.

I tried this briefly by opening the hello file and pasting an arabic
hello string a couple of times.

Stuff happens, but maybe not the correct stuff. It is hard for me to tell.

Do you have a more precise test case?


-- 
Joakim Verona

Re: [Emacs-diffs] xwidget updated (1d8b8a2 -> 5f46725)

[Eli Zaretskii]

> From: joakim@verona.se
> Cc: monnier@iro.umontreal.ca,  emacs-devel@gnu.org
> Date: Fri, 09 Jan 2015 21:12:39 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >
> >> > 5) xdisp.c:produce_xwidget_glyph needs to account for bidirectional
> >> >    display in the same way produce_image_glyph does: swao the left and
> >> >    right box edges, and populate the bidi members of struct glyph.
> >
> >> I havent thought about bidi at all.
> >
> > Just look at what produce_image_glyph does, it should tell you enough.
> >
> >> Do you have a simple test case [for bidi]?
> >
> > Type a few Arabic or Hebrew characters, then insert a widget, then
> > type some more Arabic or Hebrew.  Try this both in a buffer that has
> > bidi-paragraph-direction set to nil and in a buffer that has it set to
> > left-to-right or right-to-left.
> 
> I tried this briefly by opening the hello file and pasting an arabic
> hello string a couple of times.
> 
> Stuff happens, but maybe not the correct stuff. It is hard for me to tell.

Describe the "stuff that happens", or show a snapshot, and perhaps I
will be able to tell if its correct.

Re: [Emacs-diffs] xwidget updated (1d8b8a2 -> 5f46725)

[joakim]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: joakim@verona.se
>> Cc: monnier@iro.umontreal.ca,  emacs-devel@gnu.org
>> Date: Fri, 09 Jan 2015 21:12:39 +0100
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >
>> >> > 5) xdisp.c:produce_xwidget_glyph needs to account for bidirectional
>> >> >    display in the same way produce_image_glyph does: swao the left and
>> >> >    right box edges, and populate the bidi members of struct glyph.
>> >
>> >> I havent thought about bidi at all.
>> >
>> > Just look at what produce_image_glyph does, it should tell you enough.
>> >
>> >> Do you have a simple test case [for bidi]?
>> >
>> > Type a few Arabic or Hebrew characters, then insert a widget, then
>> > type some more Arabic or Hebrew.  Try this both in a buffer that has
>> > bidi-paragraph-direction set to nil and in a buffer that has it set to
>> > left-to-right or right-to-left.
>> 
>> I tried this briefly by opening the hello file and pasting an arabic
>> hello string a couple of times.
>> 
>> Stuff happens, but maybe not the correct stuff. It is hard for me to tell.
>
> Describe the "stuff that happens", or show a snapshot, and perhaps I
> will be able to tell if its correct.

Ok, so I added some test code that looks like:

(defmacro xwidget-demo (name &rest body)
  `(defun ,(intern (concat "xwidget-demo-" name)) ()
     (interactive)
     (switch-to-buffer ,(format "*xwidget-demo-%s*" name))
     (text-mode);;otherwise no local keymap
     (insert "Some random text for xwidgets to be inserted in for demo purposes.\n")
     ,@body))

(xwidget-demo "a-button-bidi"
              (xwidget-insert (point-min) 'Button  "button" 60  50)
              (set (make-local-variable 'bidi-paragraph-direction) 'right-to-left)
              (define-key (current-local-map) [xwidget-event] 'xwidget-handler-demo-basic))

and I get a buffer with some text that behaves in R2L mode, and a gtk
button embedded in the text.

The button doesnt seem to move with the text as it should, it stays
glued to the right window edge for some reason.

I tried adding some missing bidi code in produce_xwidget_glyph() but the
result is the same, so I'm obviously missing something. Any ideas?



-- 
Joakim Verona

Re: [Emacs-diffs] xwidget updated (1d8b8a2 -> 5f46725)

[Eli Zaretskii]

> From: joakim@verona.se
> Cc: monnier@iro.umontreal.ca,  emacs-devel@gnu.org
> Date: Fri, 16 Jan 2015 21:50:20 +0100
> 
> (defmacro xwidget-demo (name &rest body)
>   `(defun ,(intern (concat "xwidget-demo-" name)) ()
>      (interactive)
>      (switch-to-buffer ,(format "*xwidget-demo-%s*" name))
>      (text-mode);;otherwise no local keymap
>      (insert "Some random text for xwidgets to be inserted in for demo purposes.\n")
>      ,@body))
> 
> (xwidget-demo "a-button-bidi"
>               (xwidget-insert (point-min) 'Button  "button" 60  50)
>               (set (make-local-variable 'bidi-paragraph-direction) 'right-to-left)
>               (define-key (current-local-map) [xwidget-event] 'xwidget-handler-demo-basic))
> 
> and I get a buffer with some text that behaves in R2L mode, and a gtk
> button embedded in the text.
> 
> The button doesnt seem to move with the text as it should, it stays
> glued to the right window edge for some reason.
> 
> I tried adding some missing bidi code in produce_xwidget_glyph() but the
> result is the same, so I'm obviously missing something. Any ideas?

I'm not sure I'm following, sorry.  Can you show a screenshot, and
explain what did you mean by "move with the text as it should"?

Also, what text did you insert?  If my reading of the defmacro and its
call is correct, you just put a single button alone on its line, in
which case it should behave like a single character, and should be
displayed at the right edge of the window when paragraph direction is
right-to-left.

Re: [Emacs-diffs] xwidget updated (1d8b8a2 -> 5f46725)

[joakim]

[-- Attachment #1: Type: text/plain, Size: 2649 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: joakim@verona.se
>> Cc: monnier@iro.umontreal.ca,  emacs-devel@gnu.org
>> Date: Fri, 16 Jan 2015 21:50:20 +0100
>> 
>> (defmacro xwidget-demo (name &rest body)
>>   `(defun ,(intern (concat "xwidget-demo-" name)) ()
>>      (interactive)
>>      (switch-to-buffer ,(format "*xwidget-demo-%s*" name))
>>      (text-mode);;otherwise no local keymap
>>      (insert "Some random text for xwidgets to be inserted in for demo purposes.\n")
>>      ,@body))
>> 
>> (xwidget-demo "a-button-bidi"
>>               (xwidget-insert (point-min) 'Button  "button" 60  50)
>>               (set (make-local-variable 'bidi-paragraph-direction) 'right-to-left)
>>               (define-key (current-local-map) [xwidget-event] 'xwidget-handler-demo-basic))
>> 
>> and I get a buffer with some text that behaves in R2L mode, and a gtk
>> button embedded in the text.
>> 
>> The button doesnt seem to move with the text as it should, it stays
>> glued to the right window edge for some reason.
>> 
>> I tried adding some missing bidi code in produce_xwidget_glyph() but the
>> result is the same, so I'm obviously missing something. Any ideas?
>
> I'm not sure I'm following, sorry.  Can you show a screenshot, and
> explain what did you mean by "move with the text as it should"?
>
> Also, what text did you insert?  If my reading of the defmacro and its
> call is correct, you just put a single button alone on its line, in
> which case it should behave like a single character, and should be
> displayed at the right edge of the window when paragraph direction is
> right-to-left.

Maybe this is a bit clearer.

Here in the L2R case, I insert a button in the middle of the text. If I
type more text, the buttone moves along with the text towards the right
edge. This is the expected behaviour.
(xwidget-demo "a-button"
              (xwidget-insert (+ 15 (point-min)) 'Button  "button" 60  50)
              (define-key (current-local-map) [xwidget-event] 'xwidget-handler-demo-basic))

In the next case the button is also in the middle of the text at
first. However, when the direction is changed to right-to-left, the text
sticks to the right edge as expected, but the button jumps to the edge
rather than staying within the text. If I type some text, the text move
right to left, but the button just stays there at the right edge. 

(xwidget-demo "a-button-bidi"
              (xwidget-insert (+ 15 (point-min)) 'Button  "button" 60  50)
              (set (make-local-variable 'bidi-paragraph-direction) 'right-to-left)
              (define-key (current-local-map) [xwidget-event] 'xwidget-handler-demo-basic))


[-- Attachment #2: xw-bidi1.png --]
[-- Type: image/png, Size: 29917 bytes --]

[-- Attachment #3: Type: text/plain, Size: 19 bytes --]


-- 
Joakim Verona

Re: [Emacs-diffs] xwidget updated (1d8b8a2 -> 5f46725)

[Eli Zaretskii]

> From: joakim@verona.se
> Cc: monnier@iro.umontreal.ca,  emacs-devel@gnu.org
> Date: Fri, 16 Jan 2015 22:16:57 +0100
> 
> Here in the L2R case, I insert a button in the middle of the text. If I
> type more text, the buttone moves along with the text towards the right
> edge. This is the expected behaviour.
> (xwidget-demo "a-button"
>               (xwidget-insert (+ 15 (point-min)) 'Button  "button" 60  50)
>               (define-key (current-local-map) [xwidget-event] 'xwidget-handler-demo-basic))
> 
> In the next case the button is also in the middle of the text at
> first. However, when the direction is changed to right-to-left, the text
> sticks to the right edge as expected, but the button jumps to the edge
> rather than staying within the text. If I type some text, the text move
> right to left, but the button just stays there at the right edge. 

That's indeed a sign of some problem.  The code that places the widget
on the screen on the Xlib level is something specific to your changes,
am I right?  I believe that's where the problem is: somehow that code
doesn't work correctly in right-to-left display lines (a.k.a. "glyph
rows").  Can you show the code which computes the coordinates where to
place the widget?

To make sure this code is indeed the problem, I suggest to look at the
glyph row generated by the device-independent part of the display
engine, and make sure the glyph row is correct.  Here's how:

 $ cd src
 $ gdb ./emacs
 (gdb) break Fredraw_display
 (gdb) r -Q

Now recreate the problem with button display in R2L line, make sure
the cursor is in that line, and type "M-x redraw-display RET".  This
will cause GDB to kick in.  Then do:

 (gdb) break set_cursor_from_row
 (gdb) continue

Usually, the first time the breakpoint in set_cursor_from_row is hit,
it's because Emacs redisplays the echo area, which is not where we
want to look.  Type "bt" to see if that's the case: if it is, you
should see display_echo_area in the backtrace, in which case type
"continue" and wait for the next time the breakpoint breaks.  This
time, "bt" should show something like this:

  14252     struct glyph *glyph = row->glyphs[TEXT_AREA];
  (gdb) bt 10
  #0  set_cursor_from_row (w=0x17e4918 <dumped_data+2497336>, row=0xfc46e8,
      matrix=0xfbd418, delta=0, delta_bytes=0, dy=0, dvpos=0) at xdisp.c:14252
  #1  0x010663b0 in display_line (it=0x82beb0) at xdisp.c:20828
  #2  0x0105720f in try_window (window=25053469, pos=..., flags=1)
      at xdisp.c:16928
  #3  0x01053f90 in redisplay_window (window=25053469, just_this_one_p=false)
      at xdisp.c:16401
  #4  0x0104c567 in redisplay_window_0 (window=25053469) at xdisp.c:14226

Now issue this command:

 (gdb) pgrow

and post the full output.

The command "pgrow" is defined by src/.gdbinit.  Latest versions of
GDB will refuse to read that file unless you tell GDB that it's "safe"
to read it.  If you didn't make such an arrangement, the easiest thing
to do is simply read the file manually:

 (gdb) source .gdbinit

You'll have to do that in case GDB says "pgrow" is not a known
command.

Finally, it could be that "pgrow" doesn't yet know how to display the
xwidget glyph, in which case you will have to modify the pgx function
in .gdbinit to do that.

Re: [Emacs-diffs] xwidget updated (1d8b8a2 -> 5f46725)

[joakim]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: joakim@verona.se
>> Cc: monnier@iro.umontreal.ca,  emacs-devel@gnu.org
>> Date: Fri, 16 Jan 2015 22:16:57 +0100
>> 
>> Here in the L2R case, I insert a button in the middle of the text. If I
>> type more text, the buttone moves along with the text towards the right
>> edge. This is the expected behaviour.
>> (xwidget-demo "a-button"
>>               (xwidget-insert (+ 15 (point-min)) 'Button  "button" 60  50)
>>               (define-key (current-local-map) [xwidget-event] 'xwidget-handler-demo-basic))
>> 
>> In the next case the button is also in the middle of the text at
>> first. However, when the direction is changed to right-to-left, the text
>> sticks to the right edge as expected, but the button jumps to the edge
>> rather than staying within the text. If I type some text, the text move
>> right to left, but the button just stays there at the right edge. 
>
> That's indeed a sign of some problem.  The code that places the widget
> on the screen on the Xlib level is something specific to your changes,
> am I right?  I believe that's where the problem is: somehow that code
> doesn't work correctly in right-to-left display lines (a.k.a. "glyph
> rows").  Can you show the code which computes the coordinates where to
> place the widget?

The code is supposed to work like it does for images. The display engine
calculates coordinates.

Then the xwidget code further tweaks the coordinates, and place the gtk
components on-screen.

AFAICS in this particular case the (xwidget modified) display engine
isnt calculating the coordinates properly, because text is flowing
around the gtk button. It is not being placed above text.

I think the xwidget code is basically older than the merge of bidi to
trunk, and I never did the modifications necessary for bidi, and I think
I'm missing some important bidi bits.

Anyway, I will try your method below and see if I can figure something out.

> To make sure this code is indeed the problem, I suggest to look at the
> glyph row generated by the device-independent part of the display
> engine, and make sure the glyph row is correct.  Here's how:
>
>  $ cd src
>  $ gdb ./emacs
>  (gdb) break Fredraw_display
>  (gdb) r -Q
>
> Now recreate the problem with button display in R2L line, make sure
> the cursor is in that line, and type "M-x redraw-display RET".  This
> will cause GDB to kick in.  Then do:
>
>  (gdb) break set_cursor_from_row
>  (gdb) continue
>
> Usually, the first time the breakpoint in set_cursor_from_row is hit,
> it's because Emacs redisplays the echo area, which is not where we
> want to look.  Type "bt" to see if that's the case: if it is, you
> should see display_echo_area in the backtrace, in which case type
> "continue" and wait for the next time the breakpoint breaks.  This
> time, "bt" should show something like this:
>
>   14252     struct glyph *glyph = row->glyphs[TEXT_AREA];
>   (gdb) bt 10
>   #0  set_cursor_from_row (w=0x17e4918 <dumped_data+2497336>, row=0xfc46e8,
>       matrix=0xfbd418, delta=0, delta_bytes=0, dy=0, dvpos=0) at xdisp.c:14252
>   #1  0x010663b0 in display_line (it=0x82beb0) at xdisp.c:20828
>   #2  0x0105720f in try_window (window=25053469, pos=..., flags=1)
>       at xdisp.c:16928
>   #3  0x01053f90 in redisplay_window (window=25053469, just_this_one_p=false)
>       at xdisp.c:16401
>   #4  0x0104c567 in redisplay_window_0 (window=25053469) at xdisp.c:14226
>
> Now issue this command:
>
>  (gdb) pgrow
>
> and post the full output.
>
> The command "pgrow" is defined by src/.gdbinit.  Latest versions of
> GDB will refuse to read that file unless you tell GDB that it's "safe"
> to read it.  If you didn't make such an arrangement, the easiest thing
> to do is simply read the file manually:
>
>  (gdb) source .gdbinit
>
> You'll have to do that in case GDB says "pgrow" is not a known
> command.
>
> Finally, it could be that "pgrow" doesn't yet know how to display the
> xwidget glyph, in which case you will have to modify the pgx function
> in .gdbinit to do that.

-- 
Joakim Verona

Re: [Emacs-diffs] xwidget updated (1d8b8a2 -> 5f46725)

[joakim]

joakim@verona.se writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> From: joakim@verona.se
>>> Cc: monnier@iro.umontreal.ca,  emacs-devel@gnu.org
>>> Date: Fri, 16 Jan 2015 22:16:57 +0100
>>> 
>>> Here in the L2R case, I insert a button in the middle of the text. If I
>>> type more text, the buttone moves along with the text towards the right
>>> edge. This is the expected behaviour.
>>> (xwidget-demo "a-button"
>>>               (xwidget-insert (+ 15 (point-min)) 'Button  "button" 60  50)
>>>               (define-key (current-local-map) [xwidget-event] 'xwidget-handler-demo-basic))
>>> 
>>> In the next case the button is also in the middle of the text at
>>> first. However, when the direction is changed to right-to-left, the text
>>> sticks to the right edge as expected, but the button jumps to the edge
>>> rather than staying within the text. If I type some text, the text move
>>> right to left, but the button just stays there at the right edge. 
>>
>> That's indeed a sign of some problem.  The code that places the widget
>> on the screen on the Xlib level is something specific to your changes,
>> am I right?  I believe that's where the problem is: somehow that code
>> doesn't work correctly in right-to-left display lines (a.k.a. "glyph
>> rows").  Can you show the code which computes the coordinates where to
>> place the widget?
>
> The code is supposed to work like it does for images. The display engine
> calculates coordinates.

Now I tried the same test, but with an image. It behaves the same way!

So this would seem to indicate a number of possibilities:

- My test is somewhow flawed. Perhaps  (set (make-local-variable
  'bidi-paragraph-direction) 'right-to-left) isnt supposed to affect
  images and xwidgets etc.

- my xwidget code is so flawed that it affects the image code where it
  shouldnt. (the code paths are supposed to be separate though)

- this is how its supposed to work, and everything is allright.


> Then the xwidget code further tweaks the coordinates, and place the gtk
> components on-screen.
>
> AFAICS in this particular case the (xwidget modified) display engine
> isnt calculating the coordinates properly, because text is flowing
> around the gtk button. It is not being placed above text.
>
> I think the xwidget code is basically older than the merge of bidi to
> trunk, and I never did the modifications necessary for bidi, and I think
> I'm missing some important bidi bits.
>
> Anyway, I will try your method below and see if I can figure something out.
>
>> To make sure this code is indeed the problem, I suggest to look at the
>> glyph row generated by the device-independent part of the display
>> engine, and make sure the glyph row is correct.  Here's how:
>>
>>  $ cd src
>>  $ gdb ./emacs
>>  (gdb) break Fredraw_display
>>  (gdb) r -Q
>>
>> Now recreate the problem with button display in R2L line, make sure
>> the cursor is in that line, and type "M-x redraw-display RET".  This
>> will cause GDB to kick in.  Then do:
>>
>>  (gdb) break set_cursor_from_row
>>  (gdb) continue
>>
>> Usually, the first time the breakpoint in set_cursor_from_row is hit,
>> it's because Emacs redisplays the echo area, which is not where we
>> want to look.  Type "bt" to see if that's the case: if it is, you
>> should see display_echo_area in the backtrace, in which case type
>> "continue" and wait for the next time the breakpoint breaks.  This
>> time, "bt" should show something like this:
>>
>>   14252     struct glyph *glyph = row->glyphs[TEXT_AREA];
>>   (gdb) bt 10
>>   #0  set_cursor_from_row (w=0x17e4918 <dumped_data+2497336>, row=0xfc46e8,
>>       matrix=0xfbd418, delta=0, delta_bytes=0, dy=0, dvpos=0) at xdisp.c:14252
>>   #1  0x010663b0 in display_line (it=0x82beb0) at xdisp.c:20828
>>   #2  0x0105720f in try_window (window=25053469, pos=..., flags=1)
>>       at xdisp.c:16928
>>   #3  0x01053f90 in redisplay_window (window=25053469, just_this_one_p=false)
>>       at xdisp.c:16401
>>   #4  0x0104c567 in redisplay_window_0 (window=25053469) at xdisp.c:14226
>>
>> Now issue this command:
>>
>>  (gdb) pgrow
>>
>> and post the full output.
>>
>> The command "pgrow" is defined by src/.gdbinit.  Latest versions of
>> GDB will refuse to read that file unless you tell GDB that it's "safe"
>> to read it.  If you didn't make such an arrangement, the easiest thing
>> to do is simply read the file manually:
>>
>>  (gdb) source .gdbinit
>>
>> You'll have to do that in case GDB says "pgrow" is not a known
>> command.
>>
>> Finally, it could be that "pgrow" doesn't yet know how to display the
>> xwidget glyph, in which case you will have to modify the pgx function
>> in .gdbinit to do that.

-- 
Joakim Verona

Re: [Emacs-diffs] xwidget updated (1d8b8a2 -> 5f46725)

[Eli Zaretskii]

> From: joakim@verona.se
> Cc: monnier@iro.umontreal.ca,  emacs-devel@gnu.org
> Date: Sat, 17 Jan 2015 18:40:48 +0100
> 
> Now I tried the same test, but with an image. It behaves the same way!

Hmm... yes, I see that, too.

> So this would seem to indicate a number of possibilities:
> 
> - My test is somewhow flawed. Perhaps  (set (make-local-variable
>   'bidi-paragraph-direction) 'right-to-left) isnt supposed to affect
>   images and xwidgets etc.
> 
> - my xwidget code is so flawed that it affects the image code where it
>   shouldnt. (the code paths are supposed to be separate though)
> 
> - this is how its supposed to work, and everything is allright.

No, it's a bug, and I will fix it.

Thanks.

Re: [Emacs-diffs] xwidget updated (1d8b8a2 -> 5f46725)

[joakim]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: joakim@verona.se
>> Cc: monnier@iro.umontreal.ca,  emacs-devel@gnu.org
>> Date: Sat, 17 Jan 2015 18:40:48 +0100
>> 
>> Now I tried the same test, but with an image. It behaves the same way!
>
> Hmm... yes, I see that, too.
>
>> So this would seem to indicate a number of possibilities:
>> 
>> - My test is somewhow flawed. Perhaps  (set (make-local-variable
>>   'bidi-paragraph-direction) 'right-to-left) isnt supposed to affect
>>   images and xwidgets etc.
>> 
>> - my xwidget code is so flawed that it affects the image code where it
>>   shouldnt. (the code paths are supposed to be separate though)
>> 
>> - this is how its supposed to work, and everything is allright.
>
> No, it's a bug, and I will fix it.
>
> Thanks.

Ah, I'm glad we discovered something :)

-- 
Joakim Verona

Re: [Emacs-diffs] xwidget updated (1d8b8a2 -> 5f46725)

[Eli Zaretskii]

> Date: Sat, 17 Jan 2015 20:04:07 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: monnier@iro.umontreal.ca, emacs-devel@gnu.org
> 
> > From: joakim@verona.se
> > Cc: monnier@iro.umontreal.ca,  emacs-devel@gnu.org
> > Date: Sat, 17 Jan 2015 18:40:48 +0100
> > 
> > Now I tried the same test, but with an image. It behaves the same way!
> 
> Hmm... yes, I see that, too.
> 
> > So this would seem to indicate a number of possibilities:
> > 
> > - My test is somewhow flawed. Perhaps  (set (make-local-variable
> >   'bidi-paragraph-direction) 'right-to-left) isnt supposed to affect
> >   images and xwidgets etc.
> > 
> > - my xwidget code is so flawed that it affects the image code where it
> >   shouldnt. (the code paths are supposed to be separate though)
> > 
> > - this is how its supposed to work, and everything is allright.
> 
> No, it's a bug, and I will fix it.

OK, fixed on the emacs-24 branch with the attached patch.  You need to
do something similar with xwidget glyphs.

I also attach a test file.  Visit the file, modify the file names of
the icons to point to your repository, then eval-region on the 4
put-text-property lines.  You should see the images in the middle of
the text, both in L2R and R2L lines.  You can use this as a starting
point for doing the same with xwidgets.

Here's the patch:

diff --git a/src/xdisp.c b/src/xdisp.c
index a1cc286..b1125d3 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -25428,6 +25428,15 @@ and buffer to use as the context for the formatting (defaults
       enum glyph_row_area area = it->area;
 
       glyph = it->glyph_row->glyphs[area] + it->glyph_row->used[area];
+      if (it->glyph_row->reversed_p)
+	{
+	  struct glyph *g;
+
+	  /* Make room for the new glyph.  */
+	  for (g = glyph - 1; g >= it->glyph_row->glyphs[it->area]; g--)
+	    g[1] = *g;
+	  glyph = it->glyph_row->glyphs[it->area];
+	}
       if (glyph < it->glyph_row->glyphs[area + 1])
 	{
 	  glyph->charpos = CHARPOS (it->position);


And here's the test file:

============================== cut here ==============================
aaaaa  bbbbbb
 aaaaa  xxxxxx
aaaaa  yyyyyyyyy

אאאאא  בבבבב

(put-text-property 8 9 'display '(image :type xpm :file "/path/to/emacs/trunk/etc/images/bookmark_add.xpm"))

(put-text-property 23 24 'display '(image :type xpm :file "/path/to/emacs/trunk/etc/images/refresh.xpm"))

(put-text-property 37 38 'display '(image :type xpm :file "/path/to/emacs/trunk/etc/images/data-save.xpm"))

(put-text-property 54 55 'display '(image :type xpm :file "/path/to/emacs/trunk/etc/images/data-save.xpm"))

Re: [Emacs-diffs] xwidget updated (1d8b8a2 -> 5f46725)

[joakim]

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Sat, 17 Jan 2015 20:04:07 +0200
>> From: Eli Zaretskii <eliz@gnu.org>
>> Cc: monnier@iro.umontreal.ca, emacs-devel@gnu.org
>> 
>> > From: joakim@verona.se
>> > Cc: monnier@iro.umontreal.ca,  emacs-devel@gnu.org
>> > Date: Sat, 17 Jan 2015 18:40:48 +0100
>> > 
>> > Now I tried the same test, but with an image. It behaves the same way!
>> 
>> Hmm... yes, I see that, too.
>> 
>> > So this would seem to indicate a number of possibilities:
>> > 
>> > - My test is somewhow flawed. Perhaps  (set (make-local-variable
>> >   'bidi-paragraph-direction) 'right-to-left) isnt supposed to affect
>> >   images and xwidgets etc.
>> > 
>> > - my xwidget code is so flawed that it affects the image code where it
>> >   shouldnt. (the code paths are supposed to be separate though)
>> > 
>> > - this is how its supposed to work, and everything is allright.
>> 
>> No, it's a bug, and I will fix it.
>
> OK, fixed on the emacs-24 branch with the attached patch.  You need to
> do something similar with xwidget glyphs.
>
> I also attach a test file.  Visit the file, modify the file names of
> the icons to point to your repository, then eval-region on the 4
> put-text-property lines.  You should see the images in the middle of
> the text, both in L2R and R2L lines.  You can use this as a starting
> point for doing the same with xwidgets.


Ok, this patch works, so I consider bidi done for xwidgets now. Thanks!

>
> Here's the patch:
>
> diff --git a/src/xdisp.c b/src/xdisp.c
> index a1cc286..b1125d3 100644
> --- a/src/xdisp.c
> +++ b/src/xdisp.c
> @@ -25428,6 +25428,15 @@ and buffer to use as the context for the formatting (defaults
>        enum glyph_row_area area = it->area;
>  
>        glyph = it->glyph_row->glyphs[area] + it->glyph_row->used[area];
> +      if (it->glyph_row->reversed_p)
> +	{
> +	  struct glyph *g;
> +
> +	  /* Make room for the new glyph.  */
> +	  for (g = glyph - 1; g >= it->glyph_row->glyphs[it->area]; g--)
> +	    g[1] = *g;
> +	  glyph = it->glyph_row->glyphs[it->area];
> +	}
>        if (glyph < it->glyph_row->glyphs[area + 1])
>  	{
>  	  glyph->charpos = CHARPOS (it->position);
>
>
> And here's the test file:
>
> ============================== cut here ==============================
> aaaaa  bbbbbb
>  aaaaa  xxxxxx
> aaaaa  yyyyyyyyy
>
> אאאאא  בבבבב
>
> (put-text-property 8 9 'display '(image :type xpm :file "/path/to/emacs/trunk/etc/images/bookmark_add.xpm"))
>
> (put-text-property 23 24 'display '(image :type xpm :file "/path/to/emacs/trunk/etc/images/refresh.xpm"))
>
> (put-text-property 37 38 'display '(image :type xpm :file "/path/to/emacs/trunk/etc/images/data-save.xpm"))
>
> (put-text-property 54 55 'display '(image :type xpm :file "/path/to/emacs/trunk/etc/images/data-save.xpm"))

-- 
Joakim Verona