bug#28080: 25.2; `outline-invisible-p' should be specific to `invisible' property of `outline.el'

bug#28080: 25.2; `outline-invisible-p' should be specific to `invisible' property of `outline.el'

[Drew Adams]

This is the definition of `outline-invisible-p' in `outline.el':

(defsubst outline-invisible-p (&optional pos)
  "Non-nil if the character after POS is invisible.
If POS is nil, use `point' instead."
  (get-char-property (or pos (point)) 'invisible))

That's wrong.  Outline should recognize and respect, for its purposes,
only the invisibility that it, itself, imposes.

Here is a correct definition, AFAICT.  My own use of this indicates that
it DTRT.  In particular, I believe that it handles all of the invisible
text that Outline itself imposes, so that `outline.el' loses nothing by
using it.

(defun outline-invisible-p (&optional pos)
  "Non-nil if the character after point is invisible for outline purposes."
  (eq (get-char-property (or pos (point)) 'invisible) 'outline))

(It also should not be a `defsubst'.  1985 has come and gone.)

This fix is important for letting `outline.el' play with other code that
uses text invisibility for other reasons.  For example, I use it with
Info mode in a TOC buffer.  (That lets you show/hide parts of the TOC,
move parts around, etc., while still keeping hidden *Note:: etc.)

In GNU Emacs 25.2.1 (x86_64-w64-mingw32)
 of 2017-04-24
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --without-dbus --without-compress-install 'CFLAGS=-O2
 -static -g3''

bug#28080: 25.2; `outline-invisible-p' should be specific to `invisible' property of `outline.el'

[npostavs]

forcemerge 24073 28080
quit

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

> Here is a correct definition, AFAICT.  My own use of this indicates that
> it DTRT.  In particular, I believe that it handles all of the invisible
> text that Outline itself imposes, so that `outline.el' loses nothing by
> using it.
>
> (defun outline-invisible-p (&optional pos)
>   "Non-nil if the character after point is invisible for outline purposes."
>   (eq (get-char-property (or pos (point)) 'invisible) 'outline))

Yup, this is the definition in master.

> (It also should not be a `defsubst'.  1985 has come and gone.)

Except it is still a defsubst.  It is called within some loops, so
perhaps we should at least see some numbers before changing that?

bug#28080: 25.2; `outline-invisible-p' should be specific to `invisible' property of `outline.el'

[Drew Adams]

> Yup, this is the definition in master.

Great.
 
> > (It also should not be a `defsubst'.  1985 has come and gone.)
> 
> Except it is still a defsubst.  It is called within some loops, so
> perhaps we should at least see some numbers before changing that?

Doesn't matter what the numbers are, IMHO.  `defsubst' should be
dropped altogether now.  It was annoying 30 years ago, and it's
still annoying - for no real benefit. ;-)

bug#28080: 25.2; `outline-invisible-p' should be specific to `invisible' property of `outline.el'

[Drew Adams]

> > > (It also should not be a `defsubst'.  1985 has come and gone.)
> >
> > Except it is still a defsubst.  It is called within some loops, so
> > perhaps we should at least see some numbers before changing that?
> 
> Doesn't matter what the numbers are, IMHO.  `defsubst' should be
> dropped altogether now.  It was annoying 30 years ago, and it's
> still annoying - for no real benefit. ;-)

(2017 has also come and gone.)  This is still a defsubst.
Could we please get it promoted to a defun now?  Thx.

bug#28080: 25.2; `outline-invisible-p' should be specific to `invisible' property of `outline.el'

[Stefan Kangas]

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

>> > > (It also should not be a `defsubst'.  1985 has come and gone.)
>> >
>> > Except it is still a defsubst.  It is called within some loops, so
>> > perhaps we should at least see some numbers before changing that?
>>
>> Doesn't matter what the numbers are, IMHO.  `defsubst' should be
>> dropped altogether now.  It was annoying 30 years ago, and it's
>> still annoying - for no real benefit. ;-)
>
> (2017 has also come and gone.)  This is still a defsubst.
> Could we please get it promoted to a defun now?  Thx.

Noam has a point though: it would be nice to see some numbers before we
change it.