bug#33664: 26.1; Document vars and functions in `cursor-sensor.el'

bug#33664: 26.1; Document vars and functions in `cursor-sensor.el'

[Drew Adams]

Please document the variables and functions in `cursor-sensor.el'.  It
would be a step backward for Emacs to document less and less.  Calling
something an "internal" function is not, in itself, a sufficient reason
not to give it a doc string.

In particular, please add a doc string for variable
`cursor-sensor-inhibit'.

I was reading the `isearch.el' code and saw this: (push 'isearch
cursor-sensor-inhibit).  Had to start searching `cursor-sensor.el' to
try to understand what this variable is for.  Even that didn't really
help, since there is nothing in `cursor-sensor.el' that would lead one
to understand that the variable is used somehow as a list of values.

Even grepping for that var didn't help with such understanding, as there
are no uses of it outside of isearch.el.

Isearch apparently goes to the trouble of allowing for the var to be
used as a list (similar to `buffer-invisibility-spec', perhaps), but
there is no comment, doc, or spec describing this behavior/use in
`cursor-sensor.el'.


In GNU Emacs 26.1 (build 1, x86_64-w64-mingw32)
 of 2018-05-30
Repository revision: 07f8f9bc5a51f5aa94eb099f3e15fbe0c20ea1ea
Windowing system distributor `Microsoft Corp.', version 10.0.16299
Configured using:
 `configure --without-dbus --host=x86_64-w64-mingw32
 --without-compress-install 'CFLAGS=-O2 -static -g3''

bug#33664: 26.1; Document vars and functions in `cursor-sensor.el'

[Drew Adams]

To be clear: I don't really care about the "internal"
functions here.  I would like some doc for or
explanation of variable `cursor-sensor-inhibit',
particularly if the use of it by Isearch is part of
the design/intention.

(It also wouldn't hurt to add a doc string for
non-"internal" functions `cursor-sensor-tangible-pos'
and `cursor-sensor-move-to-tangible'.)

bug#33664: 26.1; Document vars and functions in `cursor-sensor.el'

[Drew Adams]

BTW, seems like function `cursor-sensor--intangible-p'
should not be "internal".  (Should be renamed to
`cursor-sensor-intangible-p' and given a doc string.)

bug#33664: 26.1; Document vars and functions in `cursor-sensor.el'

[Eli Zaretskii]

> Date: Fri, 7 Dec 2018 15:15:37 +0000 (UTC)
> From: Drew Adams <drew.adams@oracle.com>
> 
> Please document the variables and functions in `cursor-sensor.el'.  It
> would be a step backward for Emacs to document less and less.  Calling
> something an "internal" function is not, in itself, a sufficient reason
> not to give it a doc string.
> 
> In particular, please add a doc string for variable
> `cursor-sensor-inhibit'.
> 
> I was reading the `isearch.el' code and saw this: (push 'isearch
> cursor-sensor-inhibit).  Had to start searching `cursor-sensor.el' to
> try to understand what this variable is for.  Even that didn't really
> help, since there is nothing in `cursor-sensor.el' that would lead one
> to understand that the variable is used somehow as a list of values.
> 
> Even grepping for that var didn't help with such understanding, as there
> are no uses of it outside of isearch.el.
> 
> Isearch apparently goes to the trouble of allowing for the var to be
> used as a list (similar to `buffer-invisibility-spec', perhaps), but
> there is no comment, doc, or spec describing this behavior/use in
> `cursor-sensor.el'.

AFAIK, about the only person who could do something about this doesn't
read bug reports.

bug#33664: 26.1; Document vars and functions in `cursor-sensor.el'

[Drew Adams]

> AFAIK, about the only person who could do something about this doesn't
> read bug reports.

Maybe you mean might instead of could?  Anyone who has
the will could, I suppose.

But maybe the use of the variable as a list value is
only an Isearch thing (that's the only place it's used,
so far), and is not something intended/prescribed by
`cursor-sensor.el'.

Juri reads bug reports, and he has worked on Isearch
(dunno whether he worked on this part of it).  Maybe
the design/use of the var for Isearch could be
documented in a comment in `isearch.el', at least.

But if its use as a list is part of the general
design/intention then that should be made clear in
`cursor-sensor.el', I think.

bug#33664: 26.1; Document vars and functions in `cursor-sensor.el'

[Eli Zaretskii]

> Date: Fri, 7 Dec 2018 15:57:58 +0000 (UTC)
> From: Drew Adams <drew.adams@oracle.com>
> Cc: 33664@debbugs.gnu.org
> 
> > AFAIK, about the only person who could do something about this doesn't
> > read bug reports.
> 
> Maybe you mean might instead of could?  Anyone who has
> the will could, I suppose.

Patches are welcome, as always.

bug#33664: 26.1; Document vars and functions in `cursor-sensor.el'

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > AFAIK, about the only person who could do something about this doesn't
  > read bug reports.

Who is it?  Maybe we could write to per directly.

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#33664: 26.1; Document vars and functions in `cursor-sensor.el'

[Juri Linkov]

> Juri reads bug reports, and he has worked on Isearch
> (dunno whether he worked on this part of it).  Maybe
> the design/use of the var for Isearch could be
> documented in a comment in `isearch.el', at least.

I could help but I know nothing about cursor-sensor.
Please explain the purpose of cursor-sensor, then
I'll write a comment in `isearch.el'.

bug#33664: 26.1; Document vars and functions in `cursor-sensor.el'

[Drew Adams]

> > Juri reads bug reports, and he has worked on Isearch
> > (dunno whether he worked on this part of it).  Maybe
> > the design/use of the var for Isearch could be
> > documented in a comment in `isearch.el', at least.
> 
> I could help but I know nothing about cursor-sensor.
> Please explain the purpose of cursor-sensor, then
> I'll write a comment in `isearch.el'.

I too know nothing about it.

I was hoping that it was perhaps you who added its
use in Isearch.

It is apparently a replacement for using text
property `intangible', which its comments say
is too low-level.  It activates the intangibility
of text when you turn on `cursor-intangible-mode'.

The point of the bug report is to get a little
more doc for it.

Isearch's use of it seems to be to just make
sure that intangible text is not recognized
as such, i.e., it is treated as normal,
tangible text by Isearch.  At least that's
my reading of the code.

Isearch seems to politely ensure that it does
not interfere with other uses of variable
`cursor-sensor-inhibit'  It does so not by
recording the value before changing it, and
then restoring that value when finished, but
adding a non-nil value to it as a list element.

That seems like a good (and interesting)
approach.  But there is nothing in the definition,
doc, or comments for that var that would lead one
to understand that that's a good (or recommended)
way to use it, let alone the expected way.

bug#33664: 26.1; Document vars and functions in `cursor-sensor.el'

[Stefan Monnier]

I installed the patch below.


        Stefan


diff --git a/doc/lispref/text.texi b/doc/lispref/text.texi
index 14cc4fba45..78d51c6dbb 100644
--- a/doc/lispref/text.texi
+++ b/doc/lispref/text.texi
@@ -3398,6 +3398,9 @@ Special Properties
 is moved away from any position that has a non-@code{nil}
 @code{cursor-intangible} property, just before redisplay happens.
 
+When the variable @code{cursor-sensor-inhibit} is non-@code{nil}, the
+@code{cursor-intangible} property is ignored.
+
 @item field
 @kindex field @r{(text property)}
 Consecutive characters with the same @code{field} property constitute a
@@ -3579,6 +3582,9 @@ Special Properties
 property or leaving it.  The functions are called only when the minor
 mode @code{cursor-sensor-mode} is turned on.
 
+When the variable @code{cursor-sensor-inhibit} is non-@code{nil}, the
+@code{cursor-sensor-functions} property is ignored.
+
 @item composition
 @kindex composition @r{(text property)}
 This text property is used to display a sequence of characters as a
diff --git a/lisp/emacs-lisp/cursor-sensor.el b/lisp/emacs-lisp/cursor-sensor.el
index 7e3088dd91..21c48f830f 100644
--- a/lisp/emacs-lisp/cursor-sensor.el
+++ b/lisp/emacs-lisp/cursor-sensor.el
@@ -22,17 +22,29 @@
 
 ;;; Commentary:
 
-;; This package implements the `cursor-intangible' property, which is
-;; meant to replace the old `intangible' property.  To use it, just enable the
-;; `cursor-intangible-mode', after which this package will move point away from
-;; any position that has a non-nil `cursor-intangible' property.  This is only
-;; done just before redisplay happens, contrary to the old `intangible'
-;; property which was done at a much lower level.
+;; This package implements the `cursor-intangible' and
+;; `cursor-sensor-functions' properties, which are meant to replace
+;; the old `intangible', `point-entered', and `point-left' properties.
+
+;; To use `cursor-intangible', just enable the
+;; `cursor-intangible-mode' minor mode, after which this package will
+;; move point away from any position that has a non-nil
+;; `cursor-intangible' property.  This is only done just before
+;; redisplay happens, contrary to the old `intangible' property which
+;; was done at a much lower level.
+
+;; To use `cursor-sensor-functions', enable the `cursor-sensor-mode'
+;; minor mode, after which the `cursor-sensor-functions' will be
+;; called just before redisplay happens, according to the movement of
+;; the cursor since the last redisplay.
 
 ;;; Code:
 
 ;;;###autoload
-(defvar cursor-sensor-inhibit nil)
+(defvar cursor-sensor-inhibit nil
+  "When non-nil, suspend `cursor-sensor-mode' and `cursor-intangible-mode'.
+By convention, this is a list of symbols where each symbol stands for the
+\"cause\" of the suspension.")
 
 (defun cursor-sensor--intangible-p (pos)
   (let ((p (get-pos-property pos 'cursor-intangible)))

bug#33664: 26.1; Document vars and functions in `cursor-sensor.el'

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Date: Sun, 09 Dec 2018 09:01:02 -0500
> Cc: 33664@debbugs.gnu.org
> 
> I installed the patch below.

Thanks!

bug#33664: 26.1; Document vars and functions in `cursor-sensor.el'

[Drew Adams]

> > I installed the patch below.
> 
> Thanks!

+1.  The bug can be closed, IMO.

But one thing that I'd like to see/know, is some
idea of what "at a much lower level" means here:

 This is only done just before redisplay happens,
 contrary to the old `intangible' property which
 was done at a much lower level.

Specifically, I'd like to know some behavior
differences - examples of when `intangible' text
property is handled and what the resulting
behavior difference is.  Even examples of problems
that using that text property can cause would help.

Putting such additional info in the Commentary
would be useful, I think.  It would help motivate
the new mode and make clear whether and why/when
you might still want to use the text property.
Or if the answer to that is a solid no and never
then it would still help to better understand.

bug#33664: 26.1; Document vars and functions in `cursor-sensor.el'

[Drew Adams]

> +1.  The bug can be closed, IMO.

Oh, but one other thing:

We're still saying that it's enough that
the variable be Boolean.  OK - that's true,
and that's perhaps all that matters to
`cursor-sensor.el'.

But what about Isearch's use of it?  There
seems to be a disconnect of sorts, wrt
intention.

Should the variable be documented more in
line with the Isearch use of it: encourage
pushing to a list value instead of just
setting to some other non-nil value?

bug#33664: 26.1; Document vars and functions in `cursor-sensor.el'

[Stefan Monnier]

> Specifically, I'd like to know some behavior
> differences - examples of when `intangible' text

With `intangible` you can have a situation where a chunk of code does

    (goto-char POS)
    BLAH
    BLAH
    BLAH

and it misbehaves because BLAH BLAH BLAH expects to run with point at POS,
but some other package added an `intangible` property so the goto-char
actually ended up putting point somewhere else.

IOW `intangible` breaks all kinds of invariants like

    (point) == (progn (forward-char N) (forward-char -N) (point))
    (+ N (point)) == (progn (forward-char N) (point))

[ Admittedly these invariants aren't true when you bump into EOB but
  a lot of code is prepared to deal with odd cases at EOB but is not
  prepared for such surprises happening virtually anywhere.  ]


        Stefan

bug#33664: 26.1; Document vars and functions in `cursor-sensor.el'

[Drew Adams]

> > Specifically, I'd like to know some behavior
> > differences - examples of when `intangible' text
> 
> With `intangible` you can have a situation where a chunk of code does
> 
>     (goto-char POS)
>     BLAH
>     BLAH
>     BLAH
> 
> and it misbehaves because BLAH BLAH BLAH expects to run with point at
> POS,
> but some other package added an `intangible` property so the goto-char
> actually ended up putting point somewhere else.
> 
> IOW `intangible` breaks all kinds of invariants like
> 
>     (point) == (progn (forward-char N) (forward-char -N) (point))
>     (+ N (point)) == (progn (forward-char N) (point))
> 
> [ Admittedly these invariants aren't true when you bump into EOB but
>   a lot of code is prepared to deal with odd cases at EOB but is not
>   prepared for such surprises happening virtually anywhere.  ]

Thank you.  I think it would be good if you would
add this info (exactly what you wrote is fine) to
the Commentary.

Besides the difference in positions, aren't there
other important differences that come from the
`intangible' property changing Emacs behavior at
a low level rather than in pre-display?  Is it
possible that someone might take advantage of that
earlier, or lower-level, reaction?

It seems (to me, so far) like maybe the behavior
difference you described should not be cause for
deprecating text property `intangible'.  It might
be cause for recommending to generally use
`cursor-intangible-mode' instead.  It sounds like
both should probably be offered, with a description
of the differences.

Just one, hardly informed, opinion.

bug#33664: 26.1; Document vars and functions in `cursor-sensor.el'

[Stefan Monnier]

> Thank you.  I think it would be good if you would
> add this info (exactly what you wrote is fine) to
> the Commentary.

I pushed something like that (see below).

> It seems (to me, so far) like maybe the behavior
> difference you described should not be cause for
> deprecating text property `intangible'.

If you can find a case where the old `intangible` property provides the
behavior that's needed, feel free to send a bug-report to
de-deprecate it.


        Stefan


;;;; Motivation

;; The old properties were very problematic in practice because they
;; operate at a much lower level and hence affect all motion
;; *functions* like goto-char, forward-char, ... hence breaking
;; invariants like:
;;
;;    (forward-char N) == (progn (forward-char N1) (forward-char (- N N1)))
;;    (point) == (progn (forward-char N) (forward-char -N) (point))
;;    (+ N (point)) == (progn (forward-char N) (point))
;;
;; The problems would usually show up due to interaction between
;; unrelated code working in the same buffer, where one code used those
;; properties and the other (unknowingly) assumed those aren't used.
;; In practice a *lot* of code assumes there's no such funny business.
;;
;; Worse: all(?) packages using those properties don't actually want those
;; properties to affect motion at such a low-level, they only want to
;; affect the overall effect of commands, but not the effect of every
;; single point-motion that a given command happened to use internally.

bug#33664: 26.1; Document vars and functions in `cursor-sensor.el'

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

Thank you.  We needed you for this job.

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#33664: 26.1; Document vars and functions in `cursor-sensor.el'

[Lars Ingebrigtsen]

Drew Adams <drew.adams@oracle.com> writes:

>> +1.  The bug can be closed, IMO.
>
> Oh, but one other thing:
>
> We're still saying that it's enough that
> the variable be Boolean.  OK - that's true,
> and that's perhaps all that matters to
> `cursor-sensor.el'.
>
> But what about Isearch's use of it?  There
> seems to be a disconnect of sorts, wrt
> intention.
>
> Should the variable be documented more in
> line with the Isearch use of it: encourage
> pushing to a list value instead of just
> setting to some other non-nil value?

This was part of the patch:

+(defvar cursor-sensor-inhibit nil
+  "When non-nil, suspend `cursor-sensor-mode' and `cursor-intangible-mode'.
+By convention, this is a list of symbols where each symbol stands for the
+\"cause\" of the suspension.")

So I think this has been documented now, unless I misunderstand what you
mean, and I'm closing this bug report.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no