bug#21054: 25.0.50; Can't edit SES documents: Not at cell

bug#21054: 25.0.50; Can't edit SES documents: Not at cell

[Óscar Fuentes]

emacs -Q

Visit $EMACS_SRC/etc/ses-example.ses

On some cell, type RETURN to edit its contents.

The minibuffer says: Not at cell.

So it is impossible to edit existing SES documents. Editing just-created
documents works.



In GNU Emacs 25.0.50.1 (x86_64-unknown-linux-gnu, X toolkit)
 of 2015-07-08 on qcore
Repository revision: 7da7a9774cbb4d8991bd002c1c31bf49b27fb521
Windowing system distributor `The X.Org Foundation', version 11.0.11701000
System Description:	Ubuntu 15.04

Configured using:
 `configure --without-toolkit-scroll-bars --with-x-toolkit=lucid'

Configured features:
XAW3D XPM JPEG TIFF GIF PNG SOUND GSETTINGS NOTIFY LIBXML2 FREETYPE XFT
ZLIB LUCID X11

Important settings:
  value of $LANG: es_ES.UTF-8
  locale-coding-system: utf-8-unix

bug#21054: Acknowledgement (25.0.50; Can't edit SES documents: Not at cell)

[Óscar Fuentes]

Bisecting gives this as the culprit:

commit 84e0b7dad6f1a8e53261f9b96f5a9080fea681a4
Author: Stefan Monnier <monnier@iro.umontreal.ca>
Date:   Mon Apr 13 15:51:15 2015 -0400

bug#21054: 25.0.50; Can't edit SES documents: Not at cell

[Óscar Fuentes]

Óscar Fuentes <ofv@wanadoo.es> writes:

> Bisecting gives this as the culprit:
>
> commit 84e0b7dad6f1a8e53261f9b96f5a9080fea681a4
> Author: Stefan Monnier <monnier@iro.umontreal.ca>
> Date:   Mon Apr 13 15:51:15 2015 -0400

Stefan:

Is it ok to revert the SES-related changes on this revision? SES is
unusable for 4 months now.

bug#21054: 25.0.50; Can't edit SES documents: Not at cell

[Stefan Monnier]

>> Bisecting gives this as the culprit:
>> commit 84e0b7dad6f1a8e53261f9b96f5a9080fea681a4
>> Author: Stefan Monnier <monnier@iro.umontreal.ca>
>> Date:   Mon Apr 13 15:51:15 2015 -0400

> Stefan:

> Is it ok to revert the SES-related changes on this revision?

No, we should fix the actual problem,

> SES is unusable for 4 months now.

Yes, this sucks, I'm sorry,


        Stefan

bug#21054: Fixed

[Óscar Fuentes]

This was fixed by some undetermined change.

bug#21054: Reopen

[Óscar Fuentes]

reopen

After a proper build (full rebuild from a pristine checkout; something
is wrong with the Emacs build system) the problem is still there.

bug#21054: Reopen

[Eli Zaretskii]

> From: Óscar Fuentes <ofv@wanadoo.es>
> Date: Wed, 18 Nov 2015 05:00:15 +0100
> 
> After a proper build (full rebuild from a pristine checkout; something
> is wrong with the Emacs build system) the problem is still there.

The changeset you identified as the one to blame is huge, even if we
look only at changes in ses.el.  Would it be possible for you to try
to locate the code that specifically caused this misbehavior?  Or
maybe you already know that?

Armed with that knowledge, we could see how to fix this annoying
problem, which IMO should block Emacs 25.1 release, unless we fix it
soon.

Thanks.

bug#21054: 25.0.50; Can't edit SES documents: Not at cell

[Óscar Fuentes]

Eli Zaretskii <eliz@gnu.org> writes:

> The changeset you identified as the one to blame is huge, even if we
> look only at changes in ses.el.  Would it be possible for you to try
> to locate the code that specifically caused this misbehavior?

I tried for a bit and located the specific call chain that throws the
error, but not why it is thrown.

> Or maybe you already know that?
>
> Armed with that knowledge, we could see how to fix this annoying
> problem, which IMO should block Emacs 25.1 release, unless we fix it
> soon.

This bug renders ses.el unusable, so it is a release blocker indeed. The
right approach would be to revert the omnibus change that introduced the
problem as soon as it was reported, but that was rejected by Stephan and
now possibly other changes rely on the parts covered by the problematic
one.

Anyways, I have the intention of delving into this issue as soon as
possible (before the year's end) and it is very likely that I find a
"fix", but I have no enough knowledge about ses.el to guarantee its
soundness, so if anyone has a proper understanding of what's going on,
please go ahead and fix it. (The fact that Stephan didn't fix the bug
himself is an indication that it is far from being a trivial one.)

bug#21054: 25.0.50; Can't edit SES documents: Not at cell

[Óscar Fuentes]

Óscar Fuentes <ofv@wanadoo.es> writes:

> ... Stephan ...

Stefan, sorry.

For the record, the problem starts on ses-edit-cell calling
ses-check-curcell, which throws the "Not at cell" error. It seems that
ses--curcell is always nil at that call path. So this is a "fix" that
ensures that ses--curcell has a value that correponds to the actual
position of the cursor:


diff --git a/lisp/ses.el b/lisp/ses.el
index ec1359b..1631af0 100644
--- a/lisp/ses.el
+++ b/lisp/ses.el
@@ -2355,6 +2355,7 @@ ses-edit-cell
   (interactive
    (progn
      (barf-if-buffer-read-only)
+     (ses-set-curcell)
      (ses-check-curcell)
      (let* ((rowcol  (ses-sym-rowcol ses--curcell))
 	    (row     (car rowcol))


As mentioned on my previous message, I have no idea about the
correctness of this change. Moreover, some quick checks with other
functions that call ses-check-curcell show that quite a few operations
are broken too. Since the set of SES features that I use is very small,
at this point I don't know how many of those bugs are regressions and,
specifically, how many of them were introduced by the same change that
caused this bug report. Comparing with an older Emacs version will
clarify those doubts, but right now I'm under the impression that ses.el
is badly broken.

bug#21054: Reopen

[Stephen Leake]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Óscar Fuentes <ofv@wanadoo.es>
>> Date: Wed, 18 Nov 2015 05:00:15 +0100
>> 
>> After a proper build (full rebuild from a pristine checkout; something
>> is wrong with the Emacs build system) the problem is still there.
>
> The changeset you identified as the one to blame is huge, even if we
> look only at changes in ses.el.

That change introduced the function `ses--curcell'. It appears that the
variable `ses--curcell' is supposed to be the cached value of the
function `ses--curcell', but that is failing in this case.

`ses--curcell-overlay' is another variable that must be set for the
current cell; it adds the underline of the current cell. I noticed that
the underline is correct in this use case, so I looked for places where
`ses--curcell-overlay' is set but `ses--curcell' is not. I only found
one, in `ses-setup'. This patch fixes the problem for me:

diff --git a/lisp/ses.el b/lisp/ses.el
index ec1359b..03aca0a 100644
--- a/lisp/ses.el
+++ b/lisp/ses.el
@@ -1905,6 +1905,8 @@ Narrows the buffer to show only the print area.  Gives it `read-only' and
                           (forward-char)
                           (point))))
             (put-text-property pos end 'cursor-intangible sym))))))
+  (goto-char (point-min))
+  (ses-set-curcell)
   ;; Create the underlining overlay.  It's impossible for (point) to be 2,
   ;; because column A must be at least 1 column wide.
   (setq ses--curcell-overlay (make-overlay (1+ (point-min)) (1+ (point-min))))

-- 
-- Stephe

bug#21054: Reopen

[Stephen Leake]

Stephen Leake <stephen_leake@stephe-leake.org> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> From: Óscar Fuentes <ofv@wanadoo.es>
>>> Date: Wed, 18 Nov 2015 05:00:15 +0100
>>> 
>>> After a proper build (full rebuild from a pristine checkout; something
>>> is wrong with the Emacs build system) the problem is still there.
>>
>> The changeset you identified as the one to blame is huge, even if we
>> look only at changes in ses.el.
>
> That change introduced the function `ses--curcell'. It appears that the
> variable `ses--curcell' is supposed to be the cached value of the
> function `ses--curcell', but that is failing in this case.
>
> `ses--curcell-overlay' is another variable that must be set for the
> current cell; it adds the underline of the current cell. I noticed that
> the underline is correct in this use case, so I looked for places where
> `ses--curcell-overlay' is set but `ses--curcell' is not. I only found
> one, in `ses-setup'. This patch fixes the problem for me:
>
> diff --git a/lisp/ses.el b/lisp/ses.el
> index ec1359b..03aca0a 100644
> --- a/lisp/ses.el
> +++ b/lisp/ses.el
> @@ -1905,6 +1905,8 @@ Narrows the buffer to show only the print area.  Gives it `read-only' and
>                            (forward-char)
>                            (point))))
>              (put-text-property pos end 'cursor-intangible sym))))))
> +  (goto-char (point-min))
> +  (ses-set-curcell)
>    ;; Create the underlining overlay.  It's impossible for (point) to be 2,
>    ;; because column A must be at least 1 column wide.
>    (setq ses--curcell-overlay (make-overlay (1+ (point-min)) (1+ (point-min))))

I spoke too soon; this allows hitting <return> on cell A1. But hitting
<return> on cell C8 shows the contents of A1. So I missed something.

-- 
-- Stephe

bug#21054: Reopen

[Stephen Leake]

Stephen Leake <stephen_leake@stephe-leake.org> writes:

> Stephen Leake <stephen_leake@stephe-leake.org> writes:

> I spoke too soon; this allows hitting <return> on cell A1. But hitting
> <return> on cell C8 shows the contents of A1. So I missed something.

Found it; the change deleted (ses-set-curcell) from ses-command-hook, I
assume because it was in a section labeled "update mode line". But it
also updates ses--curcell, so I put it back, and things seem to work:

diff --git a/lisp/ses.el b/lisp/ses.el
index ec1359b..564d2a5 100644
--- a/lisp/ses.el
+++ b/lisp/ses.el
@@ -1905,6 +1905,8 @@ Narrows the buffer to show only the print area.  Gives it `read-only' and
                           (forward-char)
                           (point))))
             (put-text-property pos end 'cursor-intangible sym))))))
+  (goto-char (point-min))
+  (ses-set-curcell))
   ;; Create the underlining overlay.  It's impossible for (point) to be 2,
   ;; because column A must be at least 1 column wide.
   (setq ses--curcell-overlay (make-overlay (1+ (point-min)) (1+ (point-min))))
@@ -2063,7 +2065,11 @@ narrows the buffer now."
 	  ;; read the local variables at the end of the file.  Now it's safe to
 	  ;; do the narrowing.
 	  (narrow-to-region (point-min) ses--data-marker)
-	  (setq ses--deferred-narrow nil)))
+	  (setq ses--deferred-narrow nil))
+
+        ;; Update the current cell
+        (ses-set-curcell))
+
     ;; Prevent errors in this post-command-hook from silently erasing the hook!
     (error
      (unless executing-kbd-macro

-- 
-- Stephe

bug#21054: Reopen

[Óscar Fuentes]

Stephen Leake <stephen_leake@stephe-leake.org> writes:

> Stephen Leake <stephen_leake@stephe-leake.org> writes:
>
>> Stephen Leake <stephen_leake@stephe-leake.org> writes:
>
>> I spoke too soon; this allows hitting <return> on cell A1. But hitting
>> <return> on cell C8 shows the contents of A1. So I missed something.
>
> Found it; the change deleted (ses-set-curcell) from ses-command-hook, I
> assume because it was in a section labeled "update mode line". But it
> also updates ses--curcell, so I put it back, and things seem to work:

This is an interesting insight, thanks.

> diff --git a/lisp/ses.el b/lisp/ses.el
> index ec1359b..564d2a5 100644
> --- a/lisp/ses.el
> +++ b/lisp/ses.el
> @@ -1905,6 +1905,8 @@ Narrows the buffer to show only the print area.  Gives it `read-only' and
>                            (forward-char)
>                            (point))))
>              (put-text-property pos end 'cursor-intangible sym))))))
> +  (goto-char (point-min))
> +  (ses-set-curcell))

There is an extra paren here.

Besides, SES is still broken. Try

emacs -Q etc/ses-example.ses

put the cursor on A13, for instance, then press the right cursor arrow
to move the cursor to B13 and press ENTER. The prompt starts editing the
contents of A13. Interestingly, if you cancel the minibuffer prompt with
C-g and press ENTER again, B13 is edited.

Apart from that, cursor movement is erratic: try to move the cursor to
the line after the last row of the spreadsheet, note how it is not
possible and it goes to one line below. Now press the up cursor arrow
and see how the cursor goes to the rightmost cell of the last row. This
problem may be unrelated to the bug you tried to fix. For all that I
know, it might be present before the change that caused this bug report,
I need to check.

bug#21054: 25.0.50; Can't edit SES documents: Not at cell

[Óscar Fuentes]

Fixed on emacs-25 branch by commit
b1a8509030a8656a6fd3e8bb64ae38d85cd889ee and on master by
e8702794d46ae032803bf54ffbd71ebde215179c, both by Vincent Belaïche.

Those changes are functionally the same but text-wise are not equal.

The fix on emacs-25 seems definitive while the change on master is
labeled as a "quick temporary hack", so the text on master shall be
changed to match the text on emacs-25, otherwise a conflict will arise
on the next merge.

bug#21054: 25.0.50; Can't edit SES documents: Not at cell

[Vincent Belaïche]

I just did the change to master branch, as asked for by Óscar.

  Vincent.