Re: [O] Backward incompatible outline-invisible-p change in emacs master for Org

[Kaushal Modi]

On Thu, Jun 15, 2017 at 8:40 AM Bastien Guerry  wrote:

> Kaushal Modi  writes:
>
> > Before I commit this to master, I was thinking that this patch looks
> > safe enough for the maint branch. If there are no objections, I can
> > commit this to maint and merge to master.
>
> Sure, please go ahead.


Thanks. Done.
-- 

Kaushal Modi

Re: [O] Backward incompatible outline-invisible-p change in emacs master for Org

[Bastien Guerry]

Kaushal Modi  writes:

> Before I commit this to master, I was thinking that this patch looks
> safe enough for the maint branch. If there are no objections, I can
> commit this to maint and merge to master.

Sure, please go ahead.

Thanks!

-- 
 Bastien

Re: [O] Backward incompatible outline-invisible-p change in emacs master for Org

[Kaushal Modi]

On Thu, Jun 15, 2017, 2:19 AM Bastien Guerry  wrote:

> Kaushal, thanks for the fix, please go
> ahead and commit it.
>

Before I commit this to master, I was thinking that this patch looks safe
enough for the maint branch. If there are no objections, I can commit this
to maint and merge to master.

The patch adds a new function org-invisible-p whose definition is the same
as outline-invisible-p from emacs 25.2 and refactors all instances of
outline-invisible-p with org-invisible-p. So the change will be transparent
for people using emacs stable releases.

But as a plus, people using emacs master + org maint will get this bug fix
too.

> --

Kaushal Modi

Re: [O] Backward incompatible outline-invisible-p change in emacs master for Org

[Bastien Guerry]

Hi Paul,

thanks for the follow-up -- Kaushal, thanks for the fix, please go
ahead and commit it.

Best,

-- 
 Bastien

Re: [O] Backward incompatible outline-invisible-p change in emacs master for Org

[Paul Rankin]

On Thu, 15 Jun 2017, at 03:18 AM, Kaushal Modi wrote:
> On Wed, Jun 14, 2017 at 1:02 PM Bastien Guerry  wrote:
> 
> > The question is: why this patch in the first place?  Paul authored it
> > and I committed it, so I should know--but I don't.  Maybe Paul know or
> > you know better?
> >
> 
> I later found the reason for that commit here:
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24073
> 
> Here is the emacs -Q recipe that Paul posted on that debbugs report:
> 
> 1. emacs -Q
> 2. insert ";;; heading"
> 3. M-: (outline-on-heading-p)
> => t
> 4. C-a
> 5. M-: (put-text-property (point) (1+ (point)) 'invisible 'foo)
> 6. M-; (outline-on-heading-p)
> => nil
> 
> Expected results:
> 
> (outline-on-heading-p)
> => t
> 
> Actual results:
> 
> (outline-on-heading-p)
> => nil
> 
> This shows the relation between outline-on-heading-p and
> outline-invisible-p:
> 
> (defun outline-on-heading-p ( invisible-ok)
>   "Return t if point is on a (visible) heading line.
> If INVISIBLE-OK is non-nil, an invisible heading line is ok too."
>   (save-excursion
> (beginning-of-line)
> (and (bolp) (or invisible-ok (not (outline-invisible-p)))
> (looking-at outline-regexp
> 
> Basically the expectation is that a outline heading be not marked as
> "invisible" by any 'foo invisible property. Outline headings should be
> marked invisible by only 'outline invisible property.
> 
> Also as the function is prefixed with "outline-", that kind of makes sense.
> 
> On the other hand, in org, we need a function that returns non-nil for
> *any* invisible property. So that commit breaks org's expectation.
> 
> This looks fine,
> 
> 
> Thanks.
> 
> 
> > but I'd rather revert the faulty Emacs commit if
> > it is not necessary.
> >
> > Until Paul enlighten us, I'll have a deeper look.
> >
> 
> After reviewing debbugs 24073, the commit looks correct for emacs master
> and outline package. WDYT?

Glad the mystery looks like it's solved.

Btw I looped in the orgmode list back here 
http://lists.gnu.org/archive/html/emacs-orgmode/2016-09/msg00029.html

Thanks.

--
www.paulwrankin.com

Re: [O] Backward incompatible outline-invisible-p change in emacs master for Org

[Kaushal Modi]

On Wed, Jun 14, 2017 at 1:02 PM Bastien Guerry  wrote:

> The question is: why this patch in the first place?  Paul authored it
> and I committed it, so I should know--but I don't.  Maybe Paul know or
> you know better?
>

I later found the reason for that commit here:
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24073

Here is the emacs -Q recipe that Paul posted on that debbugs report:

1. emacs -Q
2. insert ";;; heading"
3. M-: (outline-on-heading-p)
=> t
4. C-a
5. M-: (put-text-property (point) (1+ (point)) 'invisible 'foo)
6. M-; (outline-on-heading-p)
=> nil

Expected results:

(outline-on-heading-p)
=> t

Actual results:

(outline-on-heading-p)
=> nil

This shows the relation between outline-on-heading-p and
outline-invisible-p:

(defun outline-on-heading-p ( invisible-ok)
  "Return t if point is on a (visible) heading line.
If INVISIBLE-OK is non-nil, an invisible heading line is ok too."
  (save-excursion
(beginning-of-line)
(and (bolp) (or invisible-ok (not (outline-invisible-p)))
(looking-at outline-regexp

Basically the expectation is that a outline heading be not marked as
"invisible" by any 'foo invisible property. Outline headings should be
marked invisible by only 'outline invisible property.

Also as the function is prefixed with "outline-", that kind of makes sense.

On the other hand, in org, we need a function that returns non-nil for
*any* invisible property. So that commit breaks org's expectation.

This looks fine,


Thanks.


> but I'd rather revert the faulty Emacs commit if
> it is not necessary.
>
> Until Paul enlighten us, I'll have a deeper look.
>

After reviewing debbugs 24073, the commit looks correct for emacs master
and outline package. WDYT?
-- 

Kaushal Modi

Re: [O] Backward incompatible outline-invisible-p change in emacs master for Org

[Bastien Guerry]

Hi Kaushal,

thanks for putting me in the loop.

Kaushal Modi  writes:

> I also find an org-invisible-p2 function, but it's not clear why it
> does a (backward-char  1) before doing invisibility check.. and that
> too eventually relies on outline-visible-p. (Turns out this function
> has a long history: http://orgmode.org/cgit.cgi/org-mode.git/commit/?
> id=93a4128a6fe47b4e05c0a7cb3ec14878b41d6000 )

The question is: why this patch in the first place?  Paul authored it
and I committed it, so I should know--but I don't.  Maybe Paul know or
you know better?

> Here's a patch for the above proposed solution.

This looks fine, but I'd rather revert the faulty Emacs commit if
it is not necessary.

Until Paul enlighten us, I'll have a deeper look.

Thanks,

-- 
 Bastien

Re: [O] Backward incompatible outline-invisible-p change in emacs master for Org (Was: 6 failing tests on master branch)

[Kaushal Modi]

On Wed, Jun 14, 2017 at 10:56 AM Kaushal Modi 
wrote:

> One solution would be to have a new function "org-invisible-p" that
> restores the old definition of outline-invisible-p. Interestingly I find
> that an "org-invisible-p" did exist back then, but it was replaced with
> outline-invisible-p in 2011 (
> http://orgmode.org/cgit.cgi/org-mode.git/commit/?id=84d7165d74a5061413168af435d61453be217933
> ). Looks like that might need reverting.
>

Here's a patch for the above proposed solution.

With that applied, 'make test' now passes cleanly on emacs master. Running
'make single' also looks good.
-- 

Kaushal Modi


0001-Fix-breakage-due-to-outline-invisible-p-defn-change-.patch
Description: Binary data