unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
* [Emacs-diffs] master fd8f724: * src/xdisp.c (overlay_arrows_changed_p): Fix last change.
@ 2017-02-28 16:10 Eli Zaretskii
  2017-02-28 17:47 ` Stefan Monnier
  0 siblings, 1 reply; 16+ messages in thread
From: Eli Zaretskii @ 2017-02-28 16:10 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

Stefan, I don't understand the new semantics of
overlay_arrows_changed_p, and even less so after your latest updates.
The commentary which documents that function's contract is now
incorrect, but I don't know how to fix it, because the return value
changes based on conditions that make no sense to the caller.

I'd prefer to change the function to return true if overlay arrows or
overlay arrow string have changed, regardless of whether the argument
to the function is true or false and regardless of whether it sets the
buffer's redisplay flag.  Then the previous contract will still be
correct, with just an addition of a side effect when the argument is
true.  Any reasons not to do that?



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Emacs-diffs] master fd8f724: * src/xdisp.c (overlay_arrows_changed_p): Fix last change.
  2017-02-28 16:10 [Emacs-diffs] master fd8f724: * src/xdisp.c (overlay_arrows_changed_p): Fix last change Eli Zaretskii
@ 2017-02-28 17:47 ` Stefan Monnier
  2017-02-28 18:09   ` Eli Zaretskii
  0 siblings, 1 reply; 16+ messages in thread
From: Stefan Monnier @ 2017-02-28 17:47 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: emacs-devel

> Stefan, I don't understand the new semantics of
> overlay_arrows_changed_p, and even less so after your latest updates.

Does the patch below clarify the situation and clean up the behavior enough?
I'm still wondering about

                /* FIXME: Don't we have a problem, using such a global
                 * "last-position" if the variable is buffer-local?  */


-- Stefan


diff --git a/src/xdisp.c b/src/xdisp.c
index 4e87001abf..1f8878408b 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -13338,12 +13338,15 @@ overlay_arrow_in_current_buffer_p (void)
 
 
 /* Return true if any overlay_arrows have moved or overlay-arrow-string
-   has changed.  */
+   has changed.
+   If SET_REDISPLAY is true, additionally, set the `redisplay' bit in those
+   buffers that are affected.  */
 
 static bool
 overlay_arrows_changed_p (bool set_redisplay)
 {
   Lisp_Object vlist;
+  bool changed = false;
 
   for (vlist = Voverlay_arrow_variable_list;
        CONSP (vlist);
@@ -13370,12 +13373,13 @@ overlay_arrows_changed_p (bool set_redisplay)
             {
               if (buf)
 	        bset_redisplay (buf);
+              changed = true;
             }
 	  else
 	    return true;
 	}
     }
-  return false;
+  return changed;
 }
 
 /* Mark overlay arrows to be updated on next redisplay.  */
@@ -13397,6 +13401,8 @@ update_overlay_arrows (int up_to_date)
       if (up_to_date > 0)
 	{
 	  Lisp_Object val = find_symbol_value (var);
+          if (!MARKERP (val))
+	    continue;
 	  Fput (var, Qlast_arrow_position,
 		COERCE_MARKER (val));
 	  Fput (var, Qlast_arrow_string,



^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [Emacs-diffs] master fd8f724: * src/xdisp.c (overlay_arrows_changed_p): Fix last change.
  2017-02-28 17:47 ` Stefan Monnier
@ 2017-02-28 18:09   ` Eli Zaretskii
  2017-02-28 18:58     ` Stefan Monnier
  0 siblings, 1 reply; 16+ messages in thread
From: Eli Zaretskii @ 2017-02-28 18:09 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: emacs-devel@gnu.org
> Date: Tue, 28 Feb 2017 12:47:05 -0500
> 
> > Stefan, I don't understand the new semantics of
> > overlay_arrows_changed_p, and even less so after your latest updates.
> 
> Does the patch below clarify the situation and clean up the behavior enough?

Yes, thanks.

> I'm still wondering about
> 
>                 /* FIXME: Don't we have a problem, using such a global
>                  * "last-position" if the variable is buffer-local?  */

I'm not sure I understand the concern.  The position is a marker, so
it specifies the buffer as well.



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Emacs-diffs] master fd8f724: * src/xdisp.c (overlay_arrows_changed_p): Fix last change.
  2017-02-28 18:09   ` Eli Zaretskii
@ 2017-02-28 18:58     ` Stefan Monnier
  2017-02-28 19:11       ` Eli Zaretskii
  0 siblings, 1 reply; 16+ messages in thread
From: Stefan Monnier @ 2017-02-28 18:58 UTC (permalink / raw)
  To: emacs-devel

>> /* FIXME: Don't we have a problem, using such a global
>> * "last-position" if the variable is buffer-local?  */

> I'm not sure I understand the concern.  The position is a marker, so
> it specifies the buffer as well.

We have a list of variables, and we store the last position of the
marker held in that variable in a symbol property.

If that variable is buffer-local, then that same variable can contain
several markers (upto one per buffer) yet it only has a single property
to keep the old value, that needs to be shared by all those
markers/buffers.


        Stefan




^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Emacs-diffs] master fd8f724: * src/xdisp.c (overlay_arrows_changed_p): Fix last change.
  2017-02-28 18:58     ` Stefan Monnier
@ 2017-02-28 19:11       ` Eli Zaretskii
  2017-02-28 20:02         ` Stefan Monnier
  0 siblings, 1 reply; 16+ messages in thread
From: Eli Zaretskii @ 2017-02-28 19:11 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Tue, 28 Feb 2017 13:58:04 -0500
> 
> We have a list of variables, and we store the last position of the
> marker held in that variable in a symbol property.
> 
> If that variable is buffer-local, then that same variable can contain
> several markers (upto one per buffer) yet it only has a single property
> to keep the old value, that needs to be shared by all those
> markers/buffers.

But we only care what happens with these between two successive
redisplay cycles.  What kind of scenario could possibly change that
variable in more than one buffer?



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Emacs-diffs] master fd8f724: * src/xdisp.c (overlay_arrows_changed_p): Fix last change.
  2017-02-28 19:11       ` Eli Zaretskii
@ 2017-02-28 20:02         ` Stefan Monnier
  2017-03-01 17:03           ` Eli Zaretskii
  0 siblings, 1 reply; 16+ messages in thread
From: Stefan Monnier @ 2017-02-28 20:02 UTC (permalink / raw)
  To: emacs-devel

> But we only care what happens with these between two successive
> redisplay cycles.  What kind of scenario could possibly change that
> variable in more than one buffer?

Here's a situation:

    (with-current-buffer A
      (setq-local overlay-arrow-position (copy-marker 10)))
    (with-current-buffer B
      (setq-local overlay-arrow-position (copy-marker 5)))

after redisplay, what should be the value of

    (get 'overlay-arrow-position 'last-arrow-position)

?  If it's 5 and we perform a redisplay, will buffer A be refreshed even
if no changes occurred in it?

The way I look at it, this code fundamentally assumes that the variables
on that list are not buffer-local (not only because of the use of
global symbol properties but also because it only considers
the value of those vars in the buffer which happens to be current).

Yet:

    % grep -l local.\*overlay-arrow-position **/*.el
    calc/calc.el
    gnus/gnus-sum.el
    mpc.el
    net/rcirc.el
    progmodes/compile.el
    progmodes/python.el
    %

And from my understanding of how this code works, we can both end up
redisplaying unnecessarily, as well as fail to redisplay when needed.


        Stefan




^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Emacs-diffs] master fd8f724: * src/xdisp.c (overlay_arrows_changed_p): Fix last change.
  2017-02-28 20:02         ` Stefan Monnier
@ 2017-03-01 17:03           ` Eli Zaretskii
  2017-03-02  5:27             ` Stefan Monnier
  0 siblings, 1 reply; 16+ messages in thread
From: Eli Zaretskii @ 2017-03-01 17:03 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Tue, 28 Feb 2017 15:02:35 -0500
> 
>     (with-current-buffer A
>       (setq-local overlay-arrow-position (copy-marker 10)))
>     (with-current-buffer B
>       (setq-local overlay-arrow-position (copy-marker 5)))
> 
> after redisplay, what should be the value of
> 
>     (get 'overlay-arrow-position 'last-arrow-position)
> 
> ?  If it's 5 and we perform a redisplay, will buffer A be refreshed even
> if no changes occurred in it?

I'd expect the value to be nil, actually, since the buffer from which
you evaluate the above is most probably neither A nor B.

In any case, if you expect a buffer shown in a non-selected window to
be redisplayed just because its overlay-arrow was modified, this never
worked in Emacs.  You can see that clearly by trying the reproducer
file at the end of this message: load it into "emacs -Q" and then type
F9 -- you will see that the overlay-arrow is not updated.  But as soon
as you type "M-x", which enforces a more thorough redisplay, the
updates become visible.  And your recent changes make this worse,
because previously it was enough to type F9 from a window displaying
one of the two buffers which have overlay-arrows, whereas now the
display is not updated in that case.

> The way I look at it, this code fundamentally assumes that the variables
> on that list are not buffer-local (not only because of the use of
> global symbol properties but also because it only considers
> the value of those vars in the buffer which happens to be current).

I think it assumes that at most one buffer changes its overlay-arrows
between two redisplay cycles.

To fix this thoroughly, we probably need to add
overlay-arrow-position, overlay-arrow-string, and overlay-arrow-bitmap
to the list of variables at the end of frame.el which trigger
redisplay of their respective buffers.  But that is not enough,
because redisplay also wants to know whether a buffer whose window is
about to be redisplayed has changed one of these, in order to disable
certain redisplay optimizations.  So maybe we should add a flag to the
buffer object where this could be recorded.

>     % grep -l local.\*overlay-arrow-position **/*.el
>     calc/calc.el
>     gnus/gnus-sum.el
>     mpc.el
>     net/rcirc.el
>     progmodes/compile.el
>     progmodes/python.el
>     %
> 
> And from my understanding of how this code works, we can both end up
> redisplaying unnecessarily, as well as fail to redisplay when needed.

Changes in overlay-arrows always triggered a thorough redisplay.

Here's the reproducer I promised:

(switch-to-buffer-other-window "A")
(insert "a\na\na\na\na\na\na\na\na\na\na\na\na\na\na\na\na\na\na\na\n")
(goto-char (point-min))
(split-window-vertically)
(switch-to-buffer-other-window "B")
(insert "b\nb\nb\nb\nb\nb\nb\nb\nb\nb\nb\nb\nb\nb\nb\nb\nb\nb\nb\nb\n")
(goto-char (point-min))
(switch-to-buffer-other-window "*scratch*")

(setq cur-line 1)
(defun my-move-arrow ()
  (interactive)
  (with-current-buffer "A"
    (set (make-local-variable 'overlay-arrow-position)
         (copy-marker (1+ (line-end-position cur-line)))))
  (with-current-buffer "B"
    (set (make-local-variable 'overlay-arrow-position)
         (copy-marker (1+ (line-end-position cur-line)))))
  (setq cur-line (1+ cur-line)))
(define-key global-map [f9] 'my-move-arrow)



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Emacs-diffs] master fd8f724: * src/xdisp.c (overlay_arrows_changed_p): Fix last change.
  2017-03-01 17:03           ` Eli Zaretskii
@ 2017-03-02  5:27             ` Stefan Monnier
  2017-03-02 16:02               ` Eli Zaretskii
  0 siblings, 1 reply; 16+ messages in thread
From: Stefan Monnier @ 2017-03-02  5:27 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: emacs-devel

> In any case, if you expect a buffer shown in a non-selected window to
> be redisplayed just because its overlay-arrow was modified, this never
> worked in Emacs.

Hmm.... I see.  Then I guess I really understand even less about this code
than I thought.

> I think it assumes that at most one buffer changes its overlay-arrows
> between two redisplay cycles.

Given that it was used to:

- cause all windows to be considered for refresh when any overlay-arrow moves
- disable the try_window_id optimisation, i.e. force every window's
  display matrices to be re-rendered.

My impression is that it was specifically designed to handle the case
where an overlay arrow is moved in another window than the currently
selected window.  And I think it does that more or less correctly as
long as none of the vars on the list are made buffer-local (I guess it
would still fail in to refresh things in the corner case where one of
the markers is moved from one buffer to another while keeping the same
numeric position).

> To fix this thoroughly, we probably need to add
> overlay-arrow-position, overlay-arrow-string, and overlay-arrow-bitmap
> to the list of variables at the end of frame.el which trigger
> redisplay of their respective buffers.

But that won't catch the case where those vars aren't modified and
instead, the marker's position is simply modified by `move-marker`.

Another option is to change overlay_arrows_changed_p so that it checks
every var to see if it's been made local to a buffer, and if so, loop
though all buffers to check if it changed in any of them (and save the
last-arrow-position per buffer).

> Changes in overlay-arrows always triggered a thorough redisplay.

That's true for changes to overlay-arrows stored in non-buffer-local
variables, yes.  And it was so thorough that it caused re-rendering of
every single window, regardless if it has/had an overlay-arrow in it.

The intent of my patch was to refine this so only those windows which
have an overlay-arrow that changed gets re-rendered.  I think it does
work correctly *if* none of the vars were made buffer-local.

Maybe instead of getting redisplay to pay closer attention to
overlay-arrow-variable-list so as to catch all the cases, we can replace
this with another mechanism which make the redisplay's job easier.
I can see two such options:
- add some kind of function to register a new marker as being an
  overlay-arrow.  I.e. instead of putting the marker into one of the
  variables in overlay-arrow-variable-list, you'd call that function
  (call it `register-overlay-arrow-marker`).  Then redisplay can keep an
  internal list of "overlay-arrow markers" and overlay_arrows_changed_p
  can loop through that list to see if one of them changed, without
  having to worry about buffer-local variables.
- use overlays instead of markers for overlay-arrows: all
  overlay modification functions already trigger the needed redisplay,
  so we could get rid of overlay_arrows_changed_p altogether.
  The downside is that overlay_arrow_at_row would have to look through
  all the overlays looking for one with the magic property, but assuming
  we keep a constraint like "overlay-arrow overlays must start at the
  beginning of the line where the arrow will be displayed", it should be
  cheap enough (i.e. a single call to something like overlays-at).


        Stefan



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Emacs-diffs] master fd8f724: * src/xdisp.c (overlay_arrows_changed_p): Fix last change.
  2017-03-02  5:27             ` Stefan Monnier
@ 2017-03-02 16:02               ` Eli Zaretskii
  2017-03-02 18:29                 ` Stefan Monnier
  0 siblings, 1 reply; 16+ messages in thread
From: Eli Zaretskii @ 2017-03-02 16:02 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: emacs-devel@gnu.org
> Date: Thu, 02 Mar 2017 00:27:30 -0500
> 
> > I think it assumes that at most one buffer changes its overlay-arrows
> > between two redisplay cycles.
> 
> Given that it was used to:
> 
> - cause all windows to be considered for refresh when any overlay-arrow moves
> - disable the try_window_id optimisation, i.e. force every window's
>   display matrices to be re-rendered.
> 
> My impression is that it was specifically designed to handle the case
> where an overlay arrow is moved in another window than the currently
> selected window.  And I think it does that more or less correctly as
> long as none of the vars on the list are made buffer-local (I guess it
> would still fail in to refresh things in the corner case where one of
> the markers is moved from one buffer to another while keeping the same
> numeric position).

I don't think this is all there is to it, because this stuff was
specifically designed to support buffer-local values of overlay-arrow
related variables.  You will find a long discussion in Mar 2005 which
led to this; that discussion ended by all the interested parties
declaring a victory, i.e. that it was then possible to have more than
one overlay-arrow by using several distinct buffer-local values.
(Months later the ELisp manual was updated to say that it's okay to
use buffer-local values for these variables.)

> > To fix this thoroughly, we probably need to add
> > overlay-arrow-position, overlay-arrow-string, and overlay-arrow-bitmap
> > to the list of variables at the end of frame.el which trigger
> > redisplay of their respective buffers.
> 
> But that won't catch the case where those vars aren't modified and
> instead, the marker's position is simply modified by `move-marker`.

Then the markers will have to be watched as well.

> The intent of my patch was to refine this so only those windows which
> have an overlay-arrow that changed gets re-rendered.  I think it does
> work correctly *if* none of the vars were made buffer-local.

But being able to make them buffer-local was an explicit goal of the
Mar 2005 changes...

> Maybe instead of getting redisplay to pay closer attention to
> overlay-arrow-variable-list so as to catch all the cases, we can replace
> this with another mechanism which make the redisplay's job easier.
> I can see two such options:
> - add some kind of function to register a new marker as being an
>   overlay-arrow.  I.e. instead of putting the marker into one of the
>   variables in overlay-arrow-variable-list, you'd call that function
>   (call it `register-overlay-arrow-marker`).  Then redisplay can keep an
>   internal list of "overlay-arrow markers" and overlay_arrows_changed_p
>   can loop through that list to see if one of them changed, without
>   having to worry about buffer-local variables.

We need to react not only to the markers, but also to
overlay-arrow-string and overlay-arrow-bitmap.

> - use overlays instead of markers for overlay-arrows: all
>   overlay modification functions already trigger the needed redisplay,
>   so we could get rid of overlay_arrows_changed_p altogether.
>   The downside is that overlay_arrow_at_row would have to look through
>   all the overlays looking for one with the magic property, but assuming
>   we keep a constraint like "overlay-arrow overlays must start at the
>   beginning of the line where the arrow will be displayed", it should be
>   cheap enough (i.e. a single call to something like overlays-at).

Isn't it easier to keep the record of changes in these variables in
the buffer object, like we do with BUF_MODIFF etc.?



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Emacs-diffs] master fd8f724: * src/xdisp.c (overlay_arrows_changed_p): Fix last change.
  2017-03-02 16:02               ` Eli Zaretskii
@ 2017-03-02 18:29                 ` Stefan Monnier
  2017-03-02 20:18                   ` Eli Zaretskii
  2017-03-03  8:13                   ` Eli Zaretskii
  0 siblings, 2 replies; 16+ messages in thread
From: Stefan Monnier @ 2017-03-02 18:29 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: emacs-devel

> I don't think this is all there is to it, because this stuff was
> specifically designed to support buffer-local values of overlay-arrow
> related variables.  You will find a long discussion in Mar 2005 which
> led to this; that discussion ended by all the interested parties
> declaring a victory, i.e. that it was then possible to have more than
> one overlay-arrow by using several distinct buffer-local values.
> (Months later the ELisp manual was updated to say that it's okay to
> use buffer-local values for these variables.)

I don't think the overlay_arrows_changed_p machinery was adapted
correspondingly, tho.  Actually, looking at diffs around 2005, I don't
see many related changes at all.  I haven't found that discussion yet,
but I wouldn't be surprised if that buffer-local approach already worked
(with the same caveat as we're discussing here), or that the change was
mostly to set_buffer before calling those functions, but that very
little, if any, changes were made to overlay_arrows_changed_p code.

>> The intent of my patch was to refine this so only those windows which
>> have an overlay-arrow that changed gets re-rendered.  I think it does
>> work correctly *if* none of the vars were made buffer-local.
> But being able to make them buffer-local was an explicit goal of the
> Mar 2005 changes...

The only commits I see which touch this part of the code around that date are:

  commit 025568d9fcdf993a33c53e30a94ae50be1d972a7
  Author: Richard M. Stallman <rms@gnu.org>
  Date:   Thu Sep 15 13:16:53 2005 +0000

    (overlay_arrow_at_row): Add HAVE_WINDOW_SYSTEM conditional.
    (display_mode_element): Instead of `lisp_string' and `this',
    record `offset' and increment that.
    `last_offset' replaces `last'.

  commit 25bcb3535cee413d750730e4894e7a39dbdc839a
  Author: Kim F. Storm <storm@cua.dk>
  Date:   Mon Apr 18 14:10:09 2005 +0000

    (overlay_arrow_string_or_property): Remove PBITMAP arg.
    Calls changed.  Don't check for overlay-arrow-bitmap property here.
    (overlay_arrow_at_row): Remove PBITMAP arg.  Instead, if left
    fringe is present, return Lisp integer for bitmap (or -1 for default).
    Fix value of overlay-arrow-bitmap property to be a symbol, use
    lookup_fringe_bitmap to parse it.
    (display_line): Change call to overlay_arrow_at_row.  Store integer
    return value as overlay bitmap in row rather than window.
    Only show overlay arrow if row displays text, or if no other overlay
    arrow is seen in window (if overlay marker is at point-max).

  commit c514efbbf12b7468322df6badaf88b365a4f52ff
  Author: Kim F. Storm <storm@cua.dk>
  Date:   Sun Oct 17 13:17:00 2004 +0000

    (overlay_arrow_at_row): Return overlay string rather
    than bitmap if there is not left fringe.
    (get_overlay_arrow_glyph_row): Also used on windows system.
    (display_line): Display overlay string if no left fringe.

None of them touches overlay_arrows_changed_p, really.

>> Maybe instead of getting redisplay to pay closer attention to
>> overlay-arrow-variable-list so as to catch all the cases, we can replace
>> this with another mechanism which make the redisplay's job easier.
>> I can see two such options:
>> - add some kind of function to register a new marker as being an
>> overlay-arrow.  I.e. instead of putting the marker into one of the
>> variables in overlay-arrow-variable-list, you'd call that function
>> (call it `register-overlay-arrow-marker`).  Then redisplay can keep an
>> internal list of "overlay-arrow markers" and overlay_arrows_changed_p
>> can loop through that list to see if one of them changed, without
>> having to worry about buffer-local variables.
> We need to react not only to the markers, but also to
> overlay-arrow-string and overlay-arrow-bitmap.

Yes, I was thinking of an API that looks like:

    (register-overlay-arrow-marker MARKER &optional STRING BITMAP)

>> - use overlays instead of markers for overlay-arrows: all
>> overlay modification functions already trigger the needed redisplay,
>> so we could get rid of overlay_arrows_changed_p altogether.
>> The downside is that overlay_arrow_at_row would have to look through
>> all the overlays looking for one with the magic property, but assuming
>> we keep a constraint like "overlay-arrow overlays must start at the
>> beginning of the line where the arrow will be displayed", it should be
>> cheap enough (i.e. a single call to something like overlays-at).

> Isn't it easier to keep the record of changes in these variables in
> the buffer object, like we do with BUF_MODIFF etc.?

I don't understand the question.  Which "these variables" are you
talking about?  Is this comment related to the above suggestion to use
overlays instead, or is it unrelated?  How do you plan on detecting
those changes in order to then record them?


        Stefan



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Emacs-diffs] master fd8f724: * src/xdisp.c (overlay_arrows_changed_p): Fix last change.
  2017-03-02 18:29                 ` Stefan Monnier
@ 2017-03-02 20:18                   ` Eli Zaretskii
  2017-03-02 22:32                     ` Stefan Monnier
  2017-03-03  8:13                   ` Eli Zaretskii
  1 sibling, 1 reply; 16+ messages in thread
From: Eli Zaretskii @ 2017-03-02 20:18 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: emacs-devel@gnu.org
> Date: Thu, 02 Mar 2017 13:29:23 -0500
> 
> > Isn't it easier to keep the record of changes in these variables in
> > the buffer object, like we do with BUF_MODIFF etc.?
> 
> I don't understand the question.  Which "these variables" are you
> talking about?

The 3 variables related to overlay-arrow whose changes need to trigger
redisplay of the respective buffers: overlay-arrow-position,
overlay-arrow-string, and overlay-arrow-bitmap.



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Emacs-diffs] master fd8f724: * src/xdisp.c (overlay_arrows_changed_p): Fix last change.
  2017-03-02 20:18                   ` Eli Zaretskii
@ 2017-03-02 22:32                     ` Stefan Monnier
  2017-03-03  8:11                       ` Eli Zaretskii
  0 siblings, 1 reply; 16+ messages in thread
From: Stefan Monnier @ 2017-03-02 22:32 UTC (permalink / raw)
  To: emacs-devel

>> > Isn't it easier to keep the record of changes in these variables in
>> > the buffer object, like we do with BUF_MODIFF etc.?
>> I don't understand the question.  Which "these variables" are you
>> talking about?
> The 3 variables related to overlay-arrow whose changes need to trigger
> redisplay of the respective buffers: overlay-arrow-position,
> overlay-arrow-string, and overlay-arrow-bitmap.

But that means we need to watch variable assignments, marker
modifications, and symbol property modifications.  Using overlays gets
us all this "for free" because we already watch all overlay modifications.

The issue of where to store the "record of change" is actually rather
minor (we currently store it in a global var and that's a bug, but
fixing it is fairly easy).


        Stefan




^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Emacs-diffs] master fd8f724: * src/xdisp.c (overlay_arrows_changed_p): Fix last change.
  2017-03-02 22:32                     ` Stefan Monnier
@ 2017-03-03  8:11                       ` Eli Zaretskii
  2017-03-06  2:53                         ` Stefan Monnier
  0 siblings, 1 reply; 16+ messages in thread
From: Eli Zaretskii @ 2017-03-03  8:11 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Thu, 02 Mar 2017 17:32:52 -0500
> 
> >> > Isn't it easier to keep the record of changes in these variables in
> >> > the buffer object, like we do with BUF_MODIFF etc.?
> >> I don't understand the question.  Which "these variables" are you
> >> talking about?
> > The 3 variables related to overlay-arrow whose changes need to trigger
> > redisplay of the respective buffers: overlay-arrow-position,
> > overlay-arrow-string, and overlay-arrow-bitmap.
> 
> But that means we need to watch variable assignments, marker
> modifications, and symbol property modifications.  Using overlays gets
> us all this "for free" because we already watch all overlay modifications.

Variables and markers, yes.  Symbol properties: I don't see why would
we need to watch them.

Anyway, it was just a thought.  It sounds like a conceptually simpler,
even if the implementation is somewhat hairy.



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Emacs-diffs] master fd8f724: * src/xdisp.c (overlay_arrows_changed_p): Fix last change.
  2017-03-02 18:29                 ` Stefan Monnier
  2017-03-02 20:18                   ` Eli Zaretskii
@ 2017-03-03  8:13                   ` Eli Zaretskii
  2017-03-06  4:17                     ` Stefan Monnier
  1 sibling, 1 reply; 16+ messages in thread
From: Eli Zaretskii @ 2017-03-03  8:13 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Thu, 02 Mar 2017 13:29:23 -0500
> Cc: emacs-devel@gnu.org
> 
> > I don't think this is all there is to it, because this stuff was
> > specifically designed to support buffer-local values of overlay-arrow
> > related variables.  You will find a long discussion in Mar 2005 which
> > led to this; that discussion ended by all the interested parties
> > declaring a victory, i.e. that it was then possible to have more than
> > one overlay-arrow by using several distinct buffer-local values.
> > (Months later the ELisp manual was updated to say that it's okay to
> > use buffer-local values for these variables.)
> 
> I don't think the overlay_arrows_changed_p machinery was adapted
> correspondingly, tho.  Actually, looking at diffs around 2005, I don't
> see many related changes at all.  I haven't found that discussion yet,

That discussion starts here:

  http://lists.gnu.org/archive/html/emacs-pretest-bug/2005-03/msg00380.html



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Emacs-diffs] master fd8f724: * src/xdisp.c (overlay_arrows_changed_p): Fix last change.
  2017-03-03  8:11                       ` Eli Zaretskii
@ 2017-03-06  2:53                         ` Stefan Monnier
  0 siblings, 0 replies; 16+ messages in thread
From: Stefan Monnier @ 2017-03-06  2:53 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: emacs-devel

>> > The 3 variables related to overlay-arrow whose changes need to trigger
>> > redisplay of the respective buffers: overlay-arrow-position,
>> > overlay-arrow-string, and overlay-arrow-bitmap.
>> But that means we need to watch variable assignments, marker
>> modifications, and symbol property modifications.  Using overlays gets
>> us all this "for free" because we already watch all overlay modifications.
> Variables and markers, yes.  Symbol properties: I don't see why would
> we need to watch them.

overlay-arrow-string and overlay-arrow-bitmap are a symbol properties.
(overlay-arrow-string is *also* a variable, but overlay-arrow-bitmap isn't).

> Anyway, it was just a thought.  It sounds like a conceptually simpler,
> even if the implementation is somewhat hairy.

To me the conceptually simpler option is to use overlays (at least, when
I first learned about overlay-arrows many years ago it took me a while
to understand them because I expected them to be represented by special
kinds of overlays).


        Stefan



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Emacs-diffs] master fd8f724: * src/xdisp.c (overlay_arrows_changed_p): Fix last change.
  2017-03-03  8:13                   ` Eli Zaretskii
@ 2017-03-06  4:17                     ` Stefan Monnier
  0 siblings, 0 replies; 16+ messages in thread
From: Stefan Monnier @ 2017-03-06  4:17 UTC (permalink / raw)
  To: emacs-devel

>> I don't think the overlay_arrows_changed_p machinery was adapted
>> correspondingly, tho.  Actually, looking at diffs around 2005, I don't
>> see many related changes at all.  I haven't found that discussion yet,
> That discussion starts here:
>   http://lists.gnu.org/archive/html/emacs-pretest-bug/2005-03/msg00380.html

That seems to confirm my suspicion.


        Stefan




^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2017-03-06  4:17 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-02-28 16:10 [Emacs-diffs] master fd8f724: * src/xdisp.c (overlay_arrows_changed_p): Fix last change Eli Zaretskii
2017-02-28 17:47 ` Stefan Monnier
2017-02-28 18:09   ` Eli Zaretskii
2017-02-28 18:58     ` Stefan Monnier
2017-02-28 19:11       ` Eli Zaretskii
2017-02-28 20:02         ` Stefan Monnier
2017-03-01 17:03           ` Eli Zaretskii
2017-03-02  5:27             ` Stefan Monnier
2017-03-02 16:02               ` Eli Zaretskii
2017-03-02 18:29                 ` Stefan Monnier
2017-03-02 20:18                   ` Eli Zaretskii
2017-03-02 22:32                     ` Stefan Monnier
2017-03-03  8:11                       ` Eli Zaretskii
2017-03-06  2:53                         ` Stefan Monnier
2017-03-03  8:13                   ` Eli Zaretskii
2017-03-06  4:17                     ` Stefan Monnier

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).