Re: Emacs bug #23794; sort-line behavior regressed from prior Emacs versions

Re: Emacs bug #23794; sort-line behavior regressed from prior Emacs versions

[Robert Weiner]

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

I would like this to be considered for marking as a blocking issue on the
next release, so that the patch may be applied and users do not experience
sort-line problems..  A patch was provided which should be compatible with
any tests available for sort-lines.  The patch makes Emacs behave as it did
before Emacs outlines moved to overlays for outlining and away from \r
characters to denote hidden lines, that is sort-lines works on visible
lines not invisible ones.  Please let me know your decision after
discussion.  Thanks, Bob

To test the problem, create a simple outline file of:
-----

* Head2
   Body2

* Head1
  Body1

-----

[-- Attachment #2: Type: text/html, Size: 1112 bytes --]

Re: Emacs bug #23794; sort-line behavior regressed from prior Emacs versions

[Eli Zaretskii]

> From: Robert Weiner <rsw@gnu.org>
> Date: Sat, 18 Jun 2016 13:01:54 -0400
> 
> I would like this to be considered for marking as a blocking issue on the next release, so that the patch may be
> applied and users do not experience sort-line problems.. A patch was provided which should be compatible
> with any tests available for sort-lines. The patch makes Emacs behave as it did before Emacs outlines moved
> to overlays for outlining and away from \r characters to denote hidden lines, that is sort-lines works on visible
> lines not invisible ones. Please let me know your decision after discussion. Thanks, Bob

Sorry, but I disagree.  A new feature can never block a release.

Re: Emacs bug #23794; sort-line behavior regressed from prior Emacs versions

[Eli Zaretskii]

> Date: Sat, 18 Jun 2016 20:26:52 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: emacs-devel@gnu.org
> 
> > From: Robert Weiner <rsw@gnu.org>
> > Date: Sat, 18 Jun 2016 13:01:54 -0400
> > 
> > I would like this to be considered for marking as a blocking issue on the next release, so that the patch may be
> > applied and users do not experience sort-line problems.. A patch was provided which should be compatible
> > with any tests available for sort-lines. The patch makes Emacs behave as it did before Emacs outlines moved
> > to overlays for outlining and away from \r characters to denote hidden lines, that is sort-lines works on visible
> > lines not invisible ones. Please let me know your decision after discussion. Thanks, Bob
> 
> Sorry, but I disagree.  A new feature can never block a release.

I see that you consider this a regression.  But the change you refer
to was in Emacs 22.1, released 9 years ago, so we've lived with this
long enough to consider your proposal a new feature.  I also think
that your proposed change goes farther than just restoring that old
behavior, because outline and its derivatives are not the only modes
that use invisible text.

Re: Emacs bug #23794; sort-line behavior regressed from prior Emacs versions

[Robert Weiner]

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

On Sat, Jun 18, 2016 at 1:42 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>
> I see that you consider this a regression.  But the change you refer
> to was in Emacs 22.1, released 9 years ago, so we've lived with this
> long enough to consider your proposal a new feature.


Well, we just fixed a 20-year-old documentation bug on markers from some
prior discussion, but I see why you might want to view it that way.  If the
behavior is a bug, no matter how long-standing, it should certainly be
given some standing above a regular new feature request.  Maybe RMS could
comment on whether sort-lines by design should sort only visible lines,
e.g. when some are hidden in outlines, or should sort all lines regardless
of visibility.

  I also think
> that your proposed change goes farther than just restoring that old
> behavior, because outline and its derivatives are not the only modes
> that use invisible text.
>

Can you provide one example where this patch changes the behavior of line
sorting in a negative way?  That would help to understand your thinking.  I
have provided the example of where it improves it.

This should be simple to resolve by just getting to the bottom of what the
function is designed to do in the case of invisible lines.

Bob

[-- Attachment #2: Type: text/html, Size: 1796 bytes --]

Re: Emacs bug #23794; sort-line behavior regressed from prior Emacs versions

[Eli Zaretskii]

> From: Robert Weiner <rsw@gnu.org>
> Date: Sat, 18 Jun 2016 13:50:23 -0400
> Cc: emacs-devel <emacs-devel@gnu.org>, Richard Stallman <rms@gnu.org>
> 
>  I see that you consider this a regression. But the change you refer
>  to was in Emacs 22.1, released 9 years ago, so we've lived with this
>  long enough to consider your proposal a new feature.
> 
> Well, we just fixed a 20-year-old documentation bug on markers from some prior discussion, but I see why
> you might want to view it that way. If the behavior is a bug, no matter how long-standing, it should certainly be
> given some standing above a regular new feature request.

I don't think it's a bug, in the sense that the old behavior was
intended.  I think the old behavior was a side effect of the
implementation, so when the implementation changed, the behavior
changed with it.

IOW, I don't think it was ever the design goal to have sorting
disregard invisible text.

>  I also think
>  that your proposed change goes farther than just restoring that old
>  behavior, because outline and its derivatives are not the only modes
>  that use invisible text.
> 
> Can you provide one example where this patch changes the behavior of line sorting in a negative way?

Why do you need examples?  Isn't it clear that ignoring invisible text
will affect much more than just outline modes?  It's clear to me.

Re: Emacs bug #23794; sort-line behavior regressed from prior Emacs versions

[Stefan Monnier]

> IOW, I don't think it was ever the design goal to have sorting
> disregard invisible text.

While I agree in general, the reason why the text is made invisible
should be taken into account: in the case of outline-mode, it seems
rather unlikely that the user would like to sort lines where some lines
are headers and others aren't, since it will completely mess up
the structure.

So by including the subsequent invisible text into each visible line,
sort-line can become usable again in outline-mode (since it then sorts
(sub)sections rather than lines).


        Stefan

Re: Emacs bug #23794; sort-line behavior regressed from prior Emacs versions

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Sat, 18 Jun 2016 14:22:18 -0400
> 
> While I agree in general, the reason why the text is made invisible
> should be taken into account: in the case of outline-mode, it seems
> rather unlikely that the user would like to sort lines where some lines
> are headers and others aren't, since it will completely mess up
> the structure.
> 
> So by including the subsequent invisible text into each visible line,
> sort-line can become usable again in outline-mode (since it then sorts
> (sub)sections rather than lines).

As I wrote elsewhere, I don't object to outline-only solution.  The
proposed solution would have a much wider effect.

Re: Emacs bug #23794; sort-line behavior regressed from prior Emacs versions

[Robert Weiner]

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

On Sat, Jun 18, 2016 at 2:28 PM, Eli Zaretskii <eliz@gnu.org> wrote:

> As I wrote elsewhere, I don't object to outline-only solution.
>

Of course we could write another function and make it do what we wanted for
a limited set of callers but the reason we are discussing it and trying to
have sort-lines changed is to help all callers of sort-lines.  My
suggestion is that if anyone has a use case where this change would hurt a
caller of sort-lines that they list it here so that it can be part of the
discussion.

There is also the second part of the patch which makes forward-visible-line
be callable the same way as forward-line, which will also help callers of
those functions.


>   The
> proposed solution would have a much wider effect.
>

As intended.

Bob

[-- Attachment #2: Type: text/html, Size: 1402 bytes --]

Re: Emacs bug #23794; sort-line behavior regressed from prior Emacs versions

[Robert Weiner]

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

On Sat, Jun 18, 2016 at 2:15 PM, Eli Zaretskii <eliz@gnu.org> wrote:

> I don't think it's a bug, in the sense that the old behavior was
> intended.  I think the old behavior was a side effect of the
> implementation, so when the implementation changed, the behavior
> changed with it.
>

If RMS confirms that is the case, then I will retract my request that this
be a blocking bug for the next release but I have a feeling the old
behavior was intended.

>
> IOW, I don't think it was ever the design goal to have sorting
> disregard invisible text.
>

I think you are misstating what happens a bit or should use some other
phrasing.  It is not that invisible text is ignored, it is simply that
invisible line endings are ignored when computing lines and the text of
invisible lines is paired with any prior visible line as part of it.  Thus,
the text characters within the invisible part are still considered when
sorting but they are not sorted separately from the visible parts.

Bob

[-- Attachment #2: Type: text/html, Size: 1619 bytes --]

RE: Emacs bug #23794; sort-line behavior regressed from prior Emacs versions

[Drew Adams]

Is it not possible to have sort-lines _optionally_ ignore
(or not ignore) invisible whatevers?

Are you discussing picking one or the other behavior only as
the default behavior, or are you discussing picking it as the
only behavior?

Re: Emacs bug #23794; sort-line behavior regressed from prior Emacs versions

[Robert Weiner]

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

On Sat, Jun 18, 2016 at 5:28 PM, Drew Adams <drew.adams@oracle.com> wrote:

> Is it not possible to have sort-lines _optionally_ ignore
> (or not ignore) invisible whatevers?
>
> Are you discussing picking one or the other behavior only as
> the default behavior, or are you discussing picking it as the
> only behavior?
>

The patch makes ignoring invisible end of line characters the default as it
was years ago when sort-lines worked for outline entries.
It would certainly make sense to have an option that toggled this behavior
that worked across the sort functions.  Eventually both cases will likely
be accommodated but the discussion is focused on what if anything to do in
the immediate term.   I am happy to make a patch with such an option if
that would settle things faster but there would still be the issue of the
default behavior.

Bob

[-- Attachment #2: Type: text/html, Size: 1233 bytes --]

Re: Emacs bug #23794; sort-line behavior regressed from prior Emacs versions

[Eli Zaretskii]

> Date: Sat, 18 Jun 2016 14:28:49 -0700 (PDT)
> From: Drew Adams <drew.adams@oracle.com>
> Cc: Richard Stallman <rms@gnu.org>, emacs-devel <emacs-devel@gnu.org>
> 
> Is it not possible to have sort-lines _optionally_ ignore
> (or not ignore) invisible whatevers?
> 
> Are you discussing picking one or the other behavior only as
> the default behavior, or are you discussing picking it as the
> only behavior?

The proposal was to make it the only behavior.  I'm quite sure such
backward incompatible changes are out of the question.

Re: Emacs bug #23794; sort-line behavior regressed from prior Emacs versions

[Robert Weiner]

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

On Sat, Jun 18, 2016 at 10:39 PM, Eli Zaretskii <eliz@gnu.org> wrote:

> The proposal was to make it the only behavior.
>

That was how the patch was written but it could easily be changed if the
core team will just say what would be acceptable.  Do you want a new option
variable that can be used by multiple functions to pay attention to or
ignore invisible newlines with a default that would be the same as things
work now?  Or would you like just an optional flag for sort-lines that
outline modes could use to minimize any change?  Then we can move on to
other things.

Bob

[-- Attachment #2: Type: text/html, Size: 989 bytes --]

Re: Emacs bug #23794; sort-line behavior regressed from prior Emacs versions

[Eli Zaretskii]

> From: Robert Weiner <rsw@gnu.org>
> Date: Sun, 19 Jun 2016 09:31:14 -0400
> Cc: Drew Adams <drew.adams@oracle.com>, Richard Stallman <rms@gnu.org>, 
> 	emacs-devel <emacs-devel@gnu.org>
> 
>  The proposal was to make it the only behavior.
> 
> That was how the patch was written but it could easily be changed if the core team will just say what would be
> acceptable. Do you want a new option variable that can be used by multiple functions to pay attention to or
> ignore invisible newlines with a default that would be the same as things work now? Or would you like just an
> optional flag for sort-lines that outline modes could use to minimize any change? Then we can move on to
> other things.

As I wrote, I'd very much prefer a solution that only affects
outline-mode and its descendants.  In those modes, ignoring invisible
lines could be the default behavior.  I think we should also provide a
defcustom to make sort-lines behave in outline modes as it does today,
because, although I agree that the current behavior makes less sense
in those modes, it's nonetheless a valid use case that we should not
disallow completely.

As for modes that are not descendants of outline-mode, the default
should IMO stay as it is now.  Whether we want an option to make
sort-lines disregard invisible text in those other modes is something
I have no opinion about, and won't object if patches to that effect
are submitted.

Thanks.

Re: Emacs bug #23794; sort-line behavior regressed from prior Emacs versions

[Robert Weiner]

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

On Sun, Jun 19, 2016 at 11:18 AM, Eli Zaretskii <eliz@gnu.org> wrote:

> As I wrote, I'd very much prefer a solution that only affects
> outline-mode and its descendants.  In those modes, ignoring invisible
> lines could be the default behavior.  I think we should also provide a
> defcustom to make sort-lines behave in outline modes as it does today,
> because, although I agree that the current behavior makes less sense
> in those modes, it's nonetheless a valid use case that we should not
> disallow completely.
>
> As for modes that are not descendants of outline-mode, the default
> should IMO stay as it is now.  Whether we want an option to make
> sort-lines disregard invisible text in those other modes is something
> I have no opinion about, and won't object if patches to that effect
> are submitted.
>

This is a much clearer statement of your thinking.  Thanks, it is helpful.
It sounds like you are suggesting that sort-lines have an internal
conditional check for whether an outline mode is active in the buffer to be
sorted (rather than suggesting a separate outline-sort-lines function),
correct?

So I will try for a patch that leaves the default behavior of sort-lines
the same unless an outline-mode or descendant is active in which case the
default will be to group invisible lines with visible ones.  There will be
a way to override the default behavior with the other behavior (not
grouping invisible lines with visible ones and treating them as separate
lines for sorting).  How does that sound?

I have already fixed this issue for Hyperbole, so I hereby retract the
request that it be made a blocking issue for 25.1.

Bob

[-- Attachment #2: Type: text/html, Size: 2099 bytes --]

Re: Emacs bug #23794; sort-line behavior regressed from prior Emacs versions

[Eli Zaretskii]

> From: Robert Weiner <rsw@gnu.org>
> Date: Sun, 19 Jun 2016 12:12:12 -0400
> Cc: Drew Adams <drew.adams@oracle.com>, Richard Stallman <rms@gnu.org>, 
> 	emacs-devel <emacs-devel@gnu.org>
> 
> On Sun, Jun 19, 2016 at 11:18 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>  As I wrote, I'd very much prefer a solution that only affects
>  outline-mode and its descendants. In those modes, ignoring invisible
>  lines could be the default behavior. I think we should also provide a
>  defcustom to make sort-lines behave in outline modes as it does today,
>  because, although I agree that the current behavior makes less sense
>  in those modes, it's nonetheless a valid use case that we should not
>  disallow completely.
> 
>  As for modes that are not descendants of outline-mode, the default
>  should IMO stay as it is now. Whether we want an option to make
>  sort-lines disregard invisible text in those other modes is something
>  I have no opinion about, and won't object if patches to that effect
>  are submitted.
> 
> This is a much clearer statement of your thinking. Thanks, it is helpful. It sounds like you are suggesting that
> sort-lines have an internal conditional check for whether an outline mode is active in the buffer to be sorted
> (rather than suggesting a separate outline-sort-lines function), correct?
> 
> So I will try for a patch that leaves the default behavior of sort-lines the same unless an outline-mode or
> descendant is active in which case the default will be to group invisible lines with visible ones. There will be a
> way to override the default behavior with the other behavior (not grouping invisible lines with visible ones and
> treating them as separate lines for sorting). How does that sound?

Didn't think that far, but is it really clean for sort-lines to have
special code for some major mode?  I thought a better way is to
override the default behavior by having sort-lines call functions
through funcall or somesuch, and then outline modes could set the
appropriate variable to the function of their liking?

Just thinking aloud.

bug#23794: Emacs bug #23794; sort-line behavior regressed from prior Emacs versions

[Robert Weiner]

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

On Sun, Jun 19, 2016 at 12:30 PM, Eli Zaretskii <eliz@gnu.org> wrote:

> Didn't think that far, but is it really clean for sort-lines to have
> special code for some major mode?  I thought a better way is to
> override the default behavior by having sort-lines call functions
> through funcall or somesuch, and then outline modes could set the
> appropriate variable to the function of their liking?
>

The problem with that approach is that each mode has to be aware of this
and add a setting, really not much different than each mode having its own
sort function or way of calling a sort function.  It is much more useful to
centralize the behavior within the sorting library, even if it adds some
conditional complexity to the code.  Here is the suggested patch to do it
this way.  -- Bob

*** sort-orig.el.gz 2016-06-19 12:42:12.000000000 -0400
--- sort.el.gz 2016-06-19 12:42:12.000000000 -0400
***************
*** 39,44 ****
--- 39,50 ----
    :type 'boolean)
  ;;;###autoload(put 'sort-fold-case 'safe-local-variable 'booleanp)

+ (defcustom sort-invisible-lines nil
+   "Non-nil if the buffer `sort-line' function should treat invisible
lines like visible ones."
+   :group 'sort
+   :type 'boolean)
+ ;;;###autoload(put 'sort-invisible-lines 'safe-local-variable 'booleanp)
+
  ;;;###autoload
  (defun sort-subr (reverse nextrecfun endrecfun
   &optional startkeyfun endkeyfun predicate)
***************
*** 210,216 ****
        (goto-char (point-min))
        (let ;; To make `end-of-line' and etc. to ignore fields.
   ((inhibit-field-text-motion t))
! (sort-subr reverse 'forward-line 'end-of-line)))))

  ;;;###autoload
  (defun sort-paragraphs (reverse beg end)
--- 216,228 ----
        (goto-char (point-min))
        (let ;; To make `end-of-line' and etc. to ignore fields.
   ((inhibit-field-text-motion t))
! (if (and (not sort-invisible-lines)
! (or (derived-mode-p 'outline-mode)
!     (and (boundp 'outline-minor-mode) outline-minor-mode)))
!    ;; In in an outline mode with sort-invisible-lines nil, sort
!    ;; only visible lines.
!    (sort-subr reverse 'forward-visible-line 'end-of-visible-line)
!  (sort-subr reverse 'forward-line 'end-of-line))))))

  ;;;###autoload
  (defun sort-paragraphs (reverse beg end)

*** simple-orig.el.gz 2016-06-18 11:29:58.000000000 -0400
--- simple.el.gz 2016-06-18 11:29:58.000000000 -0400
***************
*** 4909,4918 ****
  (kill-region (point)
       (progn (forward-visible-line arg) (point))))))

! (defun forward-visible-line (arg)
!   "Move forward by ARG lines, ignoring currently invisible newlines only.
  If ARG is negative, move backward -ARG lines.
  If ARG is zero, move to the beginning of the current line."
    (condition-case nil
        (if (> arg 0)
   (progn
--- 4909,4919 ----
  (kill-region (point)
       (progn (forward-visible-line arg) (point))))))

! (defun forward-visible-line (&optional arg)
!   "Move forward by optional ARG lines (default = 1), ignoring currently
invisible newlines only.
  If ARG is negative, move backward -ARG lines.
  If ARG is zero, move to the beginning of the current line."
+   (if (null arg) (setq arg 1))
    (condition-case nil
        (if (> arg 0)
   (progn

[-- Attachment #2: Type: text/html, Size: 7053 bytes --]

Re: Emacs bug #23794; sort-line behavior regressed from prior Emacs versions

[Eli Zaretskii]

> From: Robert Weiner <rsw@gnu.org>
> Date: Sun, 19 Jun 2016 12:51:08 -0400
> Cc: Drew Adams <drew.adams@oracle.com>, Richard Stallman <rms@gnu.org>, 
> 	emacs-devel <emacs-devel@gnu.org>, 23794@debbugs.gnu.org
> 
> On Sun, Jun 19, 2016 at 12:30 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>  Didn't think that far, but is it really clean for sort-lines to have
>  special code for some major mode? I thought a better way is to
>  override the default behavior by having sort-lines call functions
>  through funcall or somesuch, and then outline modes could set the
>  appropriate variable to the function of their liking?
> 
> The problem with that approach is that each mode has to be aware of this and add a setting, really not much
> different than each mode having its own sort function or way of calling a sort function. It is much more useful
> to centralize the behavior within the sorting library, even if it adds some conditional complexity to the code.

Not necessarily: if you set that up in outline-mode, all of its
descendants will inherit the setting for free.

> Here is the suggested patch to do it this way. -- Bob

Thanks, I hope others will comment on this.

Re: Emacs bug #23794; sort-line behavior regressed from prior Emacs versions

[Robert Weiner]

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

On Sun, Jun 19, 2016 at 12:55 PM, Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Robert Weiner <rsw@gnu.org>
> >
> > The problem with that approach is that each mode has to be aware of this
> and add a setting, really not much
> > different than each mode having its own sort function or way of calling
> a sort function. It is much more useful
> > to centralize the behavior within the sorting library, even if it adds
> some conditional complexity to the code.
>
> Not necessarily: if you set that up in outline-mode, all of its
> descendants will inherit the setting for free.
>

Ok.


>
> > Here is the suggested patch to do it this way. -- Bob
>

One other possibility that is fairly clean is to add sort-visible-*
functions to the sort library; then callers would just have to remember to
call the visible or the regular version of the function and there would be
no need for any new defcustoms.


> Thanks, I hope others will comment on this.
>

Yes.

[-- Attachment #2: Type: text/html, Size: 1698 bytes --]

RE: Emacs bug #23794; sort-line behavior regressed from prior Emacs versions

[Drew Adams]

Please don't send to both the bug list and emacs-devel, in general.

(And please consider using plain text, not HTML, mail.)

Re: bug#23794: Emacs bug #23794; sort-line behavior regressed from prior Emacs versions

[Stefan Monnier]

> The problem with that approach is that each mode has to be aware of this
> and add a setting, really not much different than each mode having its own
> sort function or way of calling a sort function.

Right.  To be an improvement, the setting should be one which makes
sense independently from the modes which sets it and independently from
the packages that uses the setting.  I.e. one needs to get at the core
reason *why* we want this behavior in this case.

I suggest something like
`invisible-text-belongs-to-the-preceding-visible-text' (which hence
implies that if you move one, you need to move the other along with it).
Maybe a more concise name is in order, tho ;-)


        Stefan

Re: Emacs bug #23794; sort-line behavior regressed from prior Emacs versions

[John Wiegley]

>>>>> Eli Zaretskii <eliz@gnu.org> writes:

> Didn't think that far, but is it really clean for sort-lines to have special
> code for some major mode? I thought a better way is to override the default
> behavior by having sort-lines call functions through funcall or somesuch,
> and then outline modes could set the appropriate variable to the function of
> their liking?
> 
> Just thinking aloud.

I would prefer this approach as well. Rather than coupling core functionality
to any particular mode, we should expose hooks that allow core functions to be
specialized.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2