bug#15925: 24.3.50; error when customizing whitespace-display-mappings

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

bug#15925: 24.3.50; error when customizing whitespace-display-mappings

Claudio Bley-3
1. M-x customize-option RET whitespace-display-mappings RET
2. click INS to insert an arbitrary char into one of the vectors
3. click the State button and select "Set for current session"

You should see this error / backtrace:

Debugger entered--Lisp error: (error "This field should contain a single character")
  signal(error ("This field should contain a single character"))
  error("%s" "This field should contain a single character")
  custom-variable-set((custom-variable :documentation-shown t
  :custom-state modified :tag "Whitespace Display Mappings" :value
  whitespace-display-mappings :custom-form edit :custom-magic
  [...]
  call-interactively(widget-button-click nil nil)
  command-execute(widget-button-click)


In wid-edit.el the character widget is defined as

(define-widget 'character 'editable-field
  "A character."
  :tag "Character"
  :value 0
  :size 1
  :format "%{%t%}: %v\n"
  :valid-regexp "\\`.\\'"
  :error "This field should contain a single character"
  [...]

Note, that the regexp does not match a single newline character, which
happens to be the problem here, as the default value of
`whitespace-display-mappings' is

((space-mark 32 [183] [46])
 (space-mark 160 [164] [95])
 (newline-mark 10 [36 10])  ;; <- ?\n here
 (tab-mark 9 [187 9] [92 9]))

I think the regexp should be changed to "\\`\(.\|\n\)\\'" to allow a
single newline also.

Claudio
--
Claudio



Reply | Threaded
Open this post in threaded view
|

bug#15925: 24.3.50; error when customizing whitespace-display-mappings

Glenn Morris-3
Claudio Bley wrote:

> I think the regexp should be changed to "\\`\(.\|\n\)\\'" to allow a
> single newline also.

We already tried that once; it caused more problems than it solved:
http://debbugs.gnu.org/2689



Reply | Threaded
Open this post in threaded view
|

bug#15925: 24.3.50; error when customizing whitespace-display-mappings

Claudio Bley-3
At Tue, 19 Nov 2013 03:24:02 -0500,
Glenn Morris wrote:
>
> Claudio Bley wrote:
>
> > I think the regexp should be changed to "\\`\(.\|\n\)\\'" to allow a
> > single newline also.
>
> We already tried that once; it caused more problems than it solved:
> http://debbugs.gnu.org/2689

I see. So lets get this solved.

At first, changing the regexp is just the right thing to do and
in itself, IMO, does not break anything.

Alas, it does not solve the problem at hand because
`widget-specify-field' behaves /badly/ when the field ends with a
newline.

So, actually, this hunk of your patch trying to work around
this behaviour broke other things.

How about using a special :value-create function for character fields?

Furthermore, the presentation of the character values is bad,
usability-wise, because for nonprintable chars you cannot see what
value the field actually has.

How about displaying those chars in the buffer in another format?
E.g. use escape sequences like \n \r et cetera?

I (rather hackish) way to fix this would be to always insert a ?\n
as part of the value of a character field:

(defun widget-field-char-create (widget)
  "Create an editable character field."
  (let ((value (widget-get widget :value))
        (from (point))
        ;; This is changed to a real overlay in `widget-setup'.  We
        ;; need the end points to behave differently until
        ;; `widget-setup' is called.
        (overlay (cons (make-marker) (make-marker))))
    (widget-put widget :field-overlay overlay)
    (insert value)
    (insert ?\n) ;; EEEK
    (unless (memq widget widget-field-list)
      (setq widget-field-new (cons widget widget-field-new)))
    (move-marker (cdr overlay) (point))
    (set-marker-insertion-type (cdr overlay) nil)
    (insert ?\n)
    (move-marker (car overlay) from)
    (set-marker-insertion-type (car overlay) t)))

What do you think?
--
Claudio



Reply | Threaded
Open this post in threaded view
|

bug#15925: 24.3.50; error when customizing whitespace-display-mappings

Stefan Monnier
> How about displaying those chars in the buffer in another format?
> E.g. use escape sequences like \n \r et cetera?

Agreed.  It could use the same syntax as used after the ? in the Elisp
source code.  I.e. the conversion would be (read-from-string (concat
"?" field)) in one direction and (substring (prin1-char char) 1) in the other.


        Stefan



Reply | Threaded
Open this post in threaded view
|

bug#15925: 24.3.50; error when customizing whitespace-display-mappings

Lars Ingebrigtsen
In reply to this post by Claudio Bley-3
Mauro Aranda <[hidden email]> writes:

> I attach a patch that should fix this (but that does not change the
> way the characters are displayed) and that doesn't introduce the
> breakage of Bug#3136 (for which I added a test).

Thanks; applied to Emacs 28.

The bits that remains to be fixed here is the display of the TAB/newline
(etc.) characters?

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



Reply | Threaded
Open this post in threaded view
|

bug#15925: 24.3.50; error when customizing whitespace-display-mappings

Mauro Aranda
Lars Ingebrigtsen <[hidden email]> writes:

> Mauro Aranda <[hidden email]> writes:
>
>> I attach a patch that should fix this (but that does not change the
>> way the characters are displayed) and that doesn't introduce the
>> breakage of Bug#3136 (for which I added a test).
>
> Thanks; applied to Emacs 28.

Thank you.

> The bits that remains to be fixed here is the display of the TAB/newline
> (etc.) characters?

Yes.  I think there were two suggestions, which if I'm not mistaken can
be summarized as:
- Display newline as \n, tab as \t, etc.
- Display newline as C-j (or ?\\C-j, or \\C-j, etc), tab as C-i, etc.

To those suggestions, I add one of mine:
- Display newline as ^J, tab as ^I, etc.

That is what Isearch does, and I think it would not be much of a
trouble to implement that.  Furthermore, we already display other
non-printable characters like that, so my suggestion would just
change newline and tab, I think.
Reply | Threaded
Open this post in threaded view
|

bug#15925: 24.3.50; error when customizing whitespace-display-mappings

Lars Ingebrigtsen
Mauro Aranda <[hidden email]> writes:

> Yes.  I think there were two suggestions, which if I'm not mistaken can
> be summarized as:
> - Display newline as \n, tab as \t, etc.
> - Display newline as C-j (or ?\\C-j, or \\C-j, etc), tab as C-i, etc.
>
> To those suggestions, I add one of mine:
> - Display newline as ^J, tab as ^I, etc.
>
> That is what Isearch does, and I think it would not be much of a
> trouble to implement that.  Furthermore, we already display other
> non-printable characters like that, so my suggestion would just
> change newline and tab, I think.

Isearch is a bit special here (being interactive)...

My guess is that more people understand that \n is newline than people
remember that ^J is newline.  (The same goes for \t/^I, but probably to
a lesser degree.)

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



Reply | Threaded
Open this post in threaded view
|

bug#15925: 24.3.50; error when customizing whitespace-display-mappings

Mauro Aranda
Lars Ingebrigtsen <[hidden email]> writes:

> Mauro Aranda <[hidden email]> writes:
>
>> Yes.  I think there were two suggestions, which if I'm not mistaken can
>> be summarized as:
>> - Display newline as \n, tab as \t, etc.
>> - Display newline as C-j (or ?\\C-j, or \\C-j, etc), tab as C-i, etc.
>>
>> To those suggestions, I add one of mine:
>> - Display newline as ^J, tab as ^I, etc.
>>
>> That is what Isearch does, and I think it would not be much of a
>> trouble to implement that.  Furthermore, we already display other
>> non-printable characters like that, so my suggestion would just
>> change newline and tab, I think.
>
> Isearch is a bit special here (being interactive)...
>
> My guess is that more people understand that \n is newline than people
> remember that ^J is newline.  (The same goes for \t/^I, but probably to
> a lesser degree.)

Those are good points.  A couple of questions:
- Do we change the display for the space character as well?

We could display it as ?\s, or leave it as " ", which makes the
character widget appear empty, when it's not.

- What do we do with the other escape sequences, like ?\r and ?\f?

Right now, we display those as ^M and ^L respectively.  If we keep this
representation, maybe somebody will feel there is some inconsistency,
because some characters we display as ^M, while others as \n.  Perhaps
is not a big deal, though.
Reply | Threaded
Open this post in threaded view
|

bug#15925: 24.3.50; error when customizing whitespace-display-mappings

Lars Ingebrigtsen
Mauro Aranda <[hidden email]> writes:

> Those are good points.  A couple of questions:
> - Do we change the display for the space character as well?
>
> We could display it as ?\s, or leave it as " ", which makes the
> character widget appear empty, when it's not.
>
> - What do we do with the other escape sequences, like ?\r and ?\f?
>
> Right now, we display those as ^M and ^L respectively.  If we keep this
> representation, maybe somebody will feel there is some inconsistency,
> because some characters we display as ^M, while others as \n.  Perhaps
> is not a big deal, though.

We should definitely strive for consistency here...  \r and \f for ^M
and ^L is fine by me (although I guess more people are familiar with ^L
than \f).

As for space...  Hm.  It's unfortunate that it's displayed just as a
blank, so displaying it as \s would definitely make sense.  But it also
sounds a bit confusing.  :-/  Could we ... use a different background
colour on the space to indicate that it's there?  I guess that's not all
that easy to interpret, either...

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



Reply | Threaded
Open this post in threaded view
|

bug#15925: 24.3.50; error when customizing whitespace-display-mappings

Eli Zaretskii
> From: Lars Ingebrigtsen <[hidden email]>
> Date: Fri, 25 Sep 2020 12:00:07 +0200
> Cc: Glenn Morris <[hidden email]>, [hidden email],
>  [hidden email]
>
> > - What do we do with the other escape sequences, like ?\r and ?\f?
> >
> > Right now, we display those as ^M and ^L respectively.  If we keep this
> > representation, maybe somebody will feel there is some inconsistency,
> > because some characters we display as ^M, while others as \n.  Perhaps
> > is not a big deal, though.
>
> We should definitely strive for consistency here...  \r and \f for ^M
> and ^L is fine by me (although I guess more people are familiar with ^L
> than \f).

I very much hope we leave the ^M and ^L display alone.  This is what
we did since day one (and yes, \n is a special case), and I'd rather
we didn't change that just in the name of consistency.



Reply | Threaded
Open this post in threaded view
|

bug#15925: 24.3.50; error when customizing whitespace-display-mappings

Mauro Aranda
In reply to this post by Lars Ingebrigtsen
How does the attached patch look? I didn't include changes for the rest
of the escape sequences, because of Eli's objections.

The reason I used the notify function is because it is much simpler than
rebinding insertion commands and having to copy some of the
functionality.  And I don't think the change in the calls to the notify
function for editable-field widgets (and descendants) will be much of a
problem.  I looked for functions that assume EVENT is nil and found
none, nor in-tree or in third-party packages.

0001-Display-some-character-widget-values-in-a-more-user-.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#15925: 24.3.50; error when customizing whitespace-display-mappings

Lars Ingebrigtsen
Mauro Aranda <[hidden email]> writes:

> How does the attached patch look? I didn't include changes for the rest
> of the escape sequences, because of Eli's objections.

Looks good.  The only thing that's confusing now is that if you delete
the char, it displays as \s?

M-x customize-variable RET whitespace-display-mappings RET



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



Reply | Threaded
Open this post in threaded view
|

bug#15925: 24.3.50; error when customizing whitespace-display-mappings

Lars Ingebrigtsen
In reply to this post by Mauro Aranda
(Oops.  Sent the previous mail too early.)

Mauro Aranda <[hidden email]> writes:

> How does the attached patch look? I didn't include changes for the rest
> of the escape sequences, because of Eli's objections.

Looks good.  The only thing that's confusing now is that if you delete
the char, it displays as \s?

M-x customize-variable RET whitespace-display-mappings RET



Hit DEL:




So there's no difference in the display between a missing character and
a space character...

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

attachment0 (5K) Download Attachment
attachment1 (14K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#15925: 24.3.50; error when customizing whitespace-display-mappings

Mauro Aranda
Lars Ingebrigtsen <[hidden email]> writes:

> (Oops.  Sent the previous mail too early.)
>
> Mauro Aranda <[hidden email]> writes:
>
>> How does the attached patch look? I didn't include changes for the rest
>> of the escape sequences, because of Eli's objections.
>
> Looks good.  The only thing that's confusing now is that if you delete
> the char, it displays as \s?
>
> M-x customize-variable RET whitespace-display-mappings RET
>
>
>
>
> Hit DEL:
>
>
>
>
>
> So there's no difference in the display between a missing character and
> a space character...

I did that on purpose, and this comment tries to explain that:
;; The character widget is not really empty:
;; its value is a single space character.
;; We need to propertize it again, if it became empty for a while.

Try the following in current master (i.e., without my patch applied):

(defcustom foo-option ?. "..."
  :type 'character
  :group 'emacs)

Eval, and confirm that foo-option value is ?.

M-x customize-option RET foo-option RET

Delete the ".", and then click State and Set for Current Session.
You'll see that foo-option was set...but to what? You said there is a
missing character, but really there's not such thing: there is a space
there, and the Widget library will return ?\s for the value of that
widget.

If you do:
M-: foo-option RET
you can confirm foo-option was set to ?\s

That's why I thought it would be less confusing to show the \s at all
times, and say "No, you can't have an empty character widget".  Having a
truly empty character widget wouldn't be of much help, since the
:validate function will complain anyway because the widget doesn't have
a single character.
Reply | Threaded
Open this post in threaded view
|

bug#15925: 24.3.50; error when customizing whitespace-display-mappings

Lars Ingebrigtsen
Mauro Aranda <[hidden email]> writes:

> That's why I thought it would be less confusing to show the \s at all
> times, and say "No, you can't have an empty character widget".  Having a
> truly empty character widget wouldn't be of much help, since the
> :validate function will complain anyway because the widget doesn't have
> a single character.

Ah, I see.  Thanks for the explanation.  Then your patch makes perfect
sense here, and I've applied it to Emacs 28.

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