bug#47784: 27.2: ses-header-row [PATCH INCLUDED]

bug#47784: 27.2: ses-header-row [PATCH INCLUDED]

[Boruch Baum]

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

Two patch files are attached.

+ ses-header-row.patch

  + Function `ses-set-header-row' was determining the current row based
    upon variable `ses--curcell', but that variable is NIL until one
    begins an operation on a cell (eg. keybindings '=', '"'), so
    navigating to a row was insufficient to select that row, and further
    generated an ERROR because the code was not expecting a NIL value for
    variable `ses--curcell'

  + Using function `user-error' instead of `error' doesn't spawn a
    debug session and backtrace buffer when variable
    `toggle-debug-on-error' is set.

+ ses-user-error.patch

  + Quasi-randomly replaces some calls to function `error' with
    `user-error'.

  + There could be many other desirable points of replacement, but these
    were some obvious ones, and without knowing whether the patch would
    be accepted at all, it's what I invested.


--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

[-- Attachment #2: ses-header-row.patch --]
[-- Type: text/x-diff, Size: 650 bytes --]

diff --git a/ses.el b/ses.el
index a2e6033..06a3d51 100644
--- a/ses.el
+++ b/ses.el
@@ -3457,7 +3457,9 @@ With a \\[universal-argument] prefix arg, prompt the user.
 The top row is row 1.  Selecting row 0 displays the default header row."
   (interactive
    (list (if (numberp current-prefix-arg) current-prefix-arg
-	   (let ((currow (1+ (car (ses-sym-rowcol ses--curcell)))))
+	   (let* ((curcell (or (ses--cell-at-pos (point))
+                               (user-error "Invalid header-row")))
+         	  (currow (1+ (car (ses-sym-rowcol curcell)))))
 	     (if current-prefix-arg
 		 (read-number "Header row: " currow)
 	       currow)))))

[-- Attachment #3: ses-user-error.patch --]
[-- Type: text/x-diff, Size: 1267 bytes --]

diff --git a/ses.el b/ses.el
index 06a3d51..f7893de 100644
--- a/ses.el
+++ b/ses.el
@@ -2252,7 +2252,7 @@ Based on the current set of columns and `window-hscroll' position."
 				      names)))))
 		 (if
 		     (string= s "")
-		     (error "Invalid cell name")
+		     (user-error "Invalid cell name")
 		   (list (intern s)))))
   (let ((rowcol (ses-sym-rowcol sym)))
     (or rowcol (error "Invalid cell name"))
@@ -3380,7 +3380,7 @@ buffer."
           ((derived-mode-p 'ses-mode) ses--local-printer-hashmap)
           ((minibufferp) ses--completion-table)
           ((derived-mode-p 'help-mode) nil)
-          (t (error "Not in a SES buffer")))))
+          (t (user-error "Not in a SES buffer")))))
   (when local-printer-hashmap
     (let ((ses--list-orig-buffer (or ses--list-orig-buffer (current-buffer))))
       (help-setup-xref
@@ -3414,7 +3414,7 @@ buffer."
           ((derived-mode-p 'ses-mode) ses--named-cell-hashmap)
           ((minibufferp) ses--completion-table)
           ((derived-mode-p 'help-mode) nil)
-          (t (error "Not in a SES buffer")))))
+          (t (user-error "Not in a SES buffer")))))
   (when named-cell-hashmap
     (let ((ses--list-orig-buffer (or ses--list-orig-buffer (current-buffer))))
       (help-setup-xref

bug#47784: 27.2: ses-header-row [PATCH INCLUDED]

[Eli Zaretskii]

> Date: Wed, 14 Apr 2021 23:31:47 -0400
> From: Boruch Baum <boruch_baum@gmx.com>
> 
> Two patch files are attached.
> 
> + ses-header-row.patch
> 
>   + Function `ses-set-header-row' was determining the current row based
>     upon variable `ses--curcell', but that variable is NIL until one
>     begins an operation on a cell (eg. keybindings '=', '"'), so
>     navigating to a row was insufficient to select that row, and further
>     generated an ERROR because the code was not expecting a NIL value for
>     variable `ses--curcell'
> 
>   + Using function `user-error' instead of `error' doesn't spawn a
>     debug session and backtrace buffer when variable
>     `toggle-debug-on-error' is set.
> 
> + ses-user-error.patch
> 
>   + Quasi-randomly replaces some calls to function `error' with
>     `user-error'.
> 
>   + There could be many other desirable points of replacement, but these
>     were some obvious ones, and without knowing whether the patch would
>     be accepted at all, it's what I invested.

Thanks, but I don't think I understand the rationale for the second
patch.  What is the problem with signaling an error in those two
places?

bug#47784: 27.2: ses-header-row [PATCH INCLUDED]

[Boruch Baum]

On 2021-04-17 22:30, Eli Zaretskii wrote:
> > Date: Wed, 14 Apr 2021 23:31:47 -0400
> > From: Boruch Baum <boruch_baum@gmx.com>

> > ...

> > + ses-user-error.patch
> >
> >   + Quasi-randomly replaces some calls to function `error' with
> >     `user-error'.
> >
> >   + There could be many other desirable points of replacement, but these
> >     were some obvious ones, and without knowing whether the patch would
> >     be accepted at all, it's what I invested.
>
> Thanks, but I don't think I understand the rationale for the second
> patch.  What is the problem with signaling an error in those two
> places?

They avoid spawning a backtrace buffer when toggle-debug-on-error is set
and the error is a user input error caught by a validation test within
an `interactive' function call.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

bug#47784: 27.2: ses-header-row [PATCH INCLUDED]

[Eli Zaretskii]

> Date: Mon, 19 Apr 2021 05:24:30 -0400
> From: Boruch Baum <boruch_baum@gmx.com>
> Cc: 47784@debbugs.gnu.org
> 
> > Thanks, but I don't think I understand the rationale for the second
> > patch.  What is the problem with signaling an error in those two
> > places?
> 
> They avoid spawning a backtrace buffer when toggle-debug-on-error is set
> and the error is a user input error caught by a validation test within
> an `interactive' function call.

I understand that, but the same is true for every call to 'error'
whenever debug-on-error is set.  Since by default debug-on-error is
NOT set, I don't think I agree with that part of the change.  (Why do
you even have that variable set non-nil?)

bug#47784: 27.2: ses-header-row [PATCH INCLUDED]

[Boruch Baum]

On 2021-04-19 16:03, Eli Zaretskii wrote:
> > Date: Mon, 19 Apr 2021 05:24:30 -0400
> > From: Boruch Baum <boruch_baum@gmx.com>
> > Cc: 47784@debbugs.gnu.org
> >
> > > Thanks, but I don't think I understand the rationale for the second
> > > patch.  What is the problem with signaling an error in those two
> > > places?
> >
> > They avoid spawning a backtrace buffer when toggle-debug-on-error is set
> > and the error is a user input error caught by a validation test within
> > an `interactive' function call.
>
> I understand that, but the same is true for every call to 'error'
> whenever debug-on-error is set.

Not at all. The specific metrics are:

  1) validation of user-input
  2) interactive call

Decide what your opinion is the purpose of function `user-error'
separate from `error' and act accordingly. Whatever metric you use,
you're bound to find cases of `error' being misused when `user-error'
should be used.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

bug#47784: 27.2: ses-header-row [PATCH INCLUDED]

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> > Thanks, but I don't think I understand the rationale for the second
>> > patch.  What is the problem with signaling an error in those two
>> > places?
>>
>> They avoid spawning a backtrace buffer when toggle-debug-on-error is set
>> and the error is a user input error caught by a validation test within
>> an `interactive' function call.
>
> I understand that, but the same is true for every call to 'error'
> whenever debug-on-error is set.  Since by default debug-on-error is
> NOT set, I don't think I agree with that part of the change.  (Why do
> you even have that variable set non-nil?)

AFAIU, `user-error' is to be used when, according to its docstring:

    This is like ‘error’ except that a user error (or "pilot error") comes
    from an incorrect manipulation by the user, not from an actual problem.

So if that is the case here, then it should be `user-error's and
otherwise it should be plain `error's.  Right?

bug#47784: 27.2: ses-header-row [PATCH INCLUDED]

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Tue, 20 Apr 2021 19:02:26 -0500
> Cc: Boruch Baum <boruch_baum@gmx.com>, 47784@debbugs.gnu.org
> 
> >> They avoid spawning a backtrace buffer when toggle-debug-on-error is set
> >> and the error is a user input error caught by a validation test within
> >> an `interactive' function call.
> >
> > I understand that, but the same is true for every call to 'error'
> > whenever debug-on-error is set.  Since by default debug-on-error is
> > NOT set, I don't think I agree with that part of the change.  (Why do
> > you even have that variable set non-nil?)
> 
> AFAIU, `user-error' is to be used when, according to its docstring:
> 
>     This is like ‘error’ except that a user error (or "pilot error") comes
>     from an incorrect manipulation by the user, not from an actual problem.
> 
> So if that is the case here, then it should be `user-error's and
> otherwise it should be plain `error's.  Right?

How do you establish whether a particular problem is due to "incorrect
manipulation by the user"?  Since almost everything in Emacs comes
from the user, one way or another, every error is due to "incorrect
manipulation" of some kind.  OTOH, commands can also be invoked from
Lisp, non-interactively, in which case the user may have nothing to do
with the problem, at least not directly.

With that in mind, please look at the code where Boruch suggested to
make these changes, and tell me what you think about them.

Boruch's rationale was a different one, it stems from running with
debug-on-error set non-nil.  But that is a very unusual thing to do,
and is bound to produce annoying backtraces all over the place.  So
I'm not sure I share the opinion that these changes should be made.  I
would like to hear other opinions, though.

bug#47784: 27.2: ses-header-row [PATCH INCLUDED]

[Lars Ingebrigtsen]

Boruch Baum <boruch_baum@gmx.com> writes:

> Two patch files are attached.
>
> + ses-header-row.patch
>
>   + Function `ses-set-header-row' was determining the current row based
>     upon variable `ses--curcell', but that variable is NIL until one
>     begins an operation on a cell (eg. keybindings '=', '"'), so
>     navigating to a row was insufficient to select that row, and further
>     generated an ERROR because the code was not expecting a NIL value for
>     variable `ses--curcell'
>
>   + Using function `user-error' instead of `error' doesn't spawn a
>     debug session and backtrace buffer when variable
>     `toggle-debug-on-error' is set.
>
> + ses-user-error.patch
>
>   + Quasi-randomly replaces some calls to function `error' with
>     `user-error'.

Looks good to me, so I've applied both patches to Emacs 28.  Eli had
some questions about the usage of user-error instead of error in the
second patch, but as far as I can tell, using `user-error' is indeed the
right thing here, because the errors in question seem more user
initiated.

I've got two tiny comments about the patch.  First of all, the
whitespace seemed odd:

lisp/ses.el:3462: space before tab in indent.
+         	  (currow (1+ (car (ses-sym-rowcol curcell)))))

So I fixed that up, but I've seen that in other patches -- could you
have a look and see what's causing this mixture of tab and spaces?

The other comment is that it would be nice to have ChangeLog-style
commit messages -- use `C-x 4 a' to format one before sending patches.
Less work when applying the patch.  :-)

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

bug#47784: 27.2: ses-header-row [PATCH INCLUDED]

[Boruch Baum]

On 2021-05-05 15:08, Lars Ingebrigtsen wrote:
> I've got two tiny comments about the patch.  First of all, the
> whitespace seemed odd:
> ...
> So I fixed that up, but I've seen that in other patches -- could you
> have a look and see what's causing this mixture of tab and spaces?

I could use a suggestion to automate that workflow: I never use tabs,
but often find myself editing files that have them. As soon as it gets
annoying, I remember to perform untabify on the buffer, but there's
probably an accepted automated method to respect the tab/space
preferences of everyone.

> The other comment is that it would be nice to have ChangeLog-style
> commit messages -- use `C-x 4 a' to format one before sending patches.
> Less work when applying the patch.  :-)

I use `C-x 4 a' a lot, but for Changelog files. How can it be used for
commit messages? Are you suggesting a temporary fake Changelog file? I
see the output now in the online gnu cgit repository, so that's do-able
manually. Are you saying that there's an automated way using function
`add-change-log-entry-other-window'?

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

bug#47784: 27.2: ses-header-row [PATCH INCLUDED]

[Eli Zaretskii]

> Date: Wed, 5 May 2021 12:40:34 -0400
> From: Boruch Baum <boruch_baum@gmx.com>
> Cc: 47784@debbugs.gnu.org
> 
> I use `C-x 4 a' a lot, but for Changelog files. How can it be used for
> commit messages? Are you suggesting a temporary fake Changelog file?

"C-x 4 a" will automatically produce a ChangeLog-formatted buffer,
from which you could copy the log message when you commit.

bug#47784: 27.2: ses-header-row [PATCH INCLUDED]

[Boruch Baum]

On 2021-05-05 22:15, Eli Zaretskii wrote:
> > Date: Wed, 5 May 2021 12:40:34 -0400
> > From: Boruch Baum <boruch_baum@gmx.com>
> > Cc: 47784@debbugs.gnu.org
> >
> > I use `C-x 4 a' a lot, but for Changelog files. How can it be used for
> > commit messages? Are you suggesting a temporary fake Changelog file?
>
> "C-x 4 a" will automatically produce a ChangeLog-formatted buffer,
> from which you could copy the log message when you commit.

Is this some new and 'backward-incompatible' behavior? In the past, I
remember it always creating a Changelog file...

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

bug#47784: 27.2: ses-header-row [PATCH INCLUDED]

[Eli Zaretskii]

> Date: Wed, 5 May 2021 15:40:46 -0400
> From: Boruch Baum <boruch_baum@gmx.com>
> Cc: larsi@gnus.org, 47784@debbugs.gnu.org
> 
> > "C-x 4 a" will automatically produce a ChangeLog-formatted buffer,
> > from which you could copy the log message when you commit.
> 
> Is this some new and 'backward-incompatible' behavior?

It's new, but not backward-incompatible, I think.  AFAIR it only
happens when the buffer visits a versioned file.

bug#47784: 27.2: ses-header-row [PATCH INCLUDED]

[Robert Pluim]

>>>>> On Wed, 05 May 2021 22:46:35 +0300, Eli Zaretskii <eliz@gnu.org> said:

    >> Date: Wed, 5 May 2021 15:40:46 -0400
    >> From: Boruch Baum <boruch_baum@gmx.com>
    >> Cc: larsi@gnus.org, 47784@debbugs.gnu.org
    >> 
    >> > "C-x 4 a" will automatically produce a ChangeLog-formatted buffer,
    >> > from which you could copy the log message when you commit.
    >> 
    >> Is this some new and 'backward-incompatible' behavior?

    Eli> It's new, but not backward-incompatible, I think.  AFAIR it only
    Eli> happens when the buffer visits a versioned file.

From NEWS.27:

    *** New command 'log-edit-generate-changelog-from-diff', bound to 'C-c C-w'.
    This generates ChangeLog entries from the VC fileset diff.

    *** Recording ChangeLog entries doesn't require an actual file.
    If a ChangeLog file doesn't exist, and if the new user option
    'add-log-dont-create-changelog-file' is non-nil (which is the
    default), commands such as 'C-x 4 a' will add log entries to a
    suitable named temporary buffer.  (An existing ChangeLog file will
    still be used if it exists.)  Set the user option to nil to get the
    previous behavior of always creating a buffer that visits a ChangeLog
    file.

If you use vc, then 'vc-checkin' will copy the log message from the
ChangeLog to the commit log edit buffer for you (and that I think itʼs
been doing for a long time).

Robert
-- 

bug#47784: 27.2: ses-header-row [PATCH INCLUDED]

[Boruch Baum]

On 2021-05-06 10:10, Robert Pluim wrote:
> From NEWS.27:
> ...

Thanks, Robert! Very helpful pointers. And now that debian has finally
bumped me to version 27.1, I really should read that NEWS file ...

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

bug#47784: 27.2: ses-header-row [PATCH INCLUDED]

[Lars Ingebrigtsen]

Boruch Baum <boruch_baum@gmx.com> writes:

>> So I fixed that up, but I've seen that in other patches -- could you
>> have a look and see what's causing this mixture of tab and spaces?
>
> I could use a suggestion to automate that workflow: I never use tabs,
> but often find myself editing files that have them. As soon as it gets
> annoying, I remember to perform untabify on the buffer, but there's
> probably an accepted automated method to respect the tab/space
> preferences of everyone.

With the default Emacs settings, you should never end up with a space
before a tab -- have you perhaps customised what the TAB key does here?
In general, the default TAB function tries to respect the indentation
style.  (But it'll never insert a space before a tab.)

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

bug#47784: 27.2: ses-header-row [PATCH INCLUDED]

[Boruch Baum]

On 2021-05-06 10:17, Lars Ingebrigtsen wrote:
> Boruch Baum <boruch_baum@gmx.com> writes:
>
> >> So I fixed that up, but I've seen that in other patches -- could you
> >> have a look and see what's causing this mixture of tab and spaces?
> >
> > I could use a suggestion to automate that workflow: I never use tabs,
> > but often find myself editing files that have them. As soon as it gets
> > annoying, I remember to perform untabify on the buffer, but there's
> > probably an accepted automated method to respect the tab/space
> > preferences of everyone.
>
> With the default Emacs settings, you should never end up with a space
> before a tab -- have you perhaps customised what the TAB key does here?
> In general, the default TAB function tries to respect the indentation
> style.  (But it'll never insert a space before a tab.)

But I don't ever want a tab, ever, in any document that I work on, ever.
If tabs are needed in order to be compatible with upstream then I'd want
emacs to untabify on read and tabify on export.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

bug#47784: 27.2: ses-header-row [PATCH INCLUDED]

[Lars Ingebrigtsen]

Boruch Baum <boruch_baum@gmx.com> writes:

> But I don't ever want a tab, ever, in any document that I work on, ever.

That's counter to how Emacs sources are (and should remain).  We use a
mix of tabs and spaces (but the spaces are after the tabs).

> If tabs are needed in order to be compatible with upstream then I'd want
> emacs to untabify on read and tabify on export.

I don't think there's any such functionality in Emacs, but it shouldn't
be difficult for you to hack up something like that to fix up your
patches.  It would be easier if you just didn't change your tab/space
settings when you work on Emacs sources, though.

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