bug#61432: 28.2; [PATCH] viper-init: disable face support

bug#61432: 28.2; [PATCH] viper-init: disable face support

[Andrés Ramírez]

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

Hi.

The following patch disables face support for not changing the modeline
fonts when using viper.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: mypatch --]
[-- Type: text/x-patch, Size: 718 bytes --]

diff -u /tmp/viper-init.el.bak /tmp/viper-init.el
--- /tmp/viper-init.el.bak	2023-02-10 12:25:52.388368619 +0000
+++ /tmp/viper-init.el	2023-02-10 12:30:11.118394615 +0000
@@ -78,11 +78,16 @@
   :type 'boolean
   :group 'viper-highlighting)
 
+(defvar viper-disable-face-support nil
+  "disable the face support for not changing faces in the modeline. And stay as emacs -Q.")
+
 (defun viper-has-face-support-p ()
+  (if (and (boundp 'viper-disable-face-support) viper-disable-face-support)
+      nil
   (cond ((viper-window-display-p))
 	(viper-force-faces)
 	((viper-color-display-p))
-	(t (memq window-system '(pc)))))
+	(t (memq window-system '(pc))))))
 
 \f
 ;;; Macros

Diff finished.  Fri Feb 10 12:31:17 2023

[-- Attachment #3: Type: text/plain, Size: 14 bytes --]


Best Regards

bug#61432: 28.2; [PATCH] viper-init: disable face support

[Eli Zaretskii]

> From: Andrés Ramírez <rrandresf@hotmail.com>
> Date: Sat, 11 Feb 2023 14:22:44 +0000
> 
> The following patch disables face support for not changing the modeline
> fonts when using viper.
> 
> diff -u /tmp/viper-init.el.bak /tmp/viper-init.el
> --- /tmp/viper-init.el.bak	2023-02-10 12:25:52.388368619 +0000
> +++ /tmp/viper-init.el	2023-02-10 12:30:11.118394615 +0000
> @@ -78,11 +78,16 @@
>    :type 'boolean
>    :group 'viper-highlighting)
>  
> +(defvar viper-disable-face-support nil
> +  "disable the face support for not changing faces in the modeline. And stay as emacs -Q.")
> +
>  (defun viper-has-face-support-p ()
> +  (if (and (boundp 'viper-disable-face-support) viper-disable-face-support)
> +      nil
>    (cond ((viper-window-display-p))
>  	(viper-force-faces)
>  	((viper-color-display-p))
> -	(t (memq window-system '(pc)))))
> +	(t (memq window-system '(pc))))))

Can't you have the same effect if you customize the faces
viper-minibuffer-emacs-face and viper-minibuffer-vi-face to look the
same?

IOW, I don't think I understand the intent (nor the rationale, btw),
but then I'm not using viper, so I'm probably missing something.

Thanks.

bug#61432: 28.2; [PATCH] viper-init: disable face support

[andrés ramírez]

Hi. Eli.

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


[...]


    Eli> Can't you have the same effect if you customize the faces viper-minibuffer-emacs-face and
    Eli> viper-minibuffer-vi-face to look the same?

I have tried some modifications. like this one.
--8<---------------cut here---------------start------------->8---
(when (viper-has-face-support-p)
  (set-face-foreground 'viper-minibuffer-emacs "white") 
  (set-face-background 'viper-minibuffer-emacs "black"))
--8<---------------cut here---------------end--------------->8---

But We need to remember than viper is a very old package. Perhaps emacs
21 or before. At that time viper coder's thought modifying always the
faces for reflecting the viper state and also the viper emulation level
(something like level 1 to level 5. It was according to the colors). The
only thing they missed at that time was a variable for leaving
everything as it was before. The same behaviour You get when running
'emacs -Q'.

My issues with viper default behaviour are:
Most of my time is spend on emacs within xterm. But from times to times
I open PDF files with an emacs-lucid-frame at that time when running
'M-x' and showing the prompt faces have changed.

Also when I move to an fb-console and when doing 'M-x' (and again
inspecting the prompt or writting the command) sometimes what I type is
not noticed cos of the faces. So I need to M-x delete-frame and
'TERM=screen emacsclient -c -t' that behaviour is different than the one
You get when not using viper. So I have been thinking why the original
viper coder's have not created a variable to inhibit the faces
modifications.

For solving the issue when being on 'insert-mode' or not being on
'insert-mode' I use a trick used by the evil-mode users changing the
color of the modeline. So the main issue why the original viper coder's
did that have been avoided (having a visual help for knowing in which
state viper is currently in).

And Yes perhaps customization of faces could work. But when You need to do it
on several machines. So the simple solution would be having a variable
to inhibit that behaviour.

Finding the right face to modify would take time also. 

Hope that clarifies the use of this variable.

And the most of vipers users are not going to notice that variable.

Best Regards

bug#61432: 28.2; [PATCH] viper-init: disable face support

[Eli Zaretskii]

> From: andrés ramírez <rrandresf@hotmail.com>
> Cc: 61432@debbugs.gnu.org
> Date: Sat, 11 Feb 2023 16:10:23 +0000
> 
>     Eli> Can't you have the same effect if you customize the faces viper-minibuffer-emacs-face and
>     Eli> viper-minibuffer-vi-face to look the same?
> 
> I have tried some modifications. like this one.

The easiest one is this:

  (require 'viper-init)
  (setq viper-minibuffer-vi-face viper-minibuffer-emacs-face)

Did you try that?
> Hope that clarifies the use of this variable.
> 
> And the most of vipers users are not going to notice that variable.

My problem with this is twofold:

  . I don't like to have code in Emacs which "works around" our own code
  . viper-has-face-support-p is used for more than just the mode line

So I'd prefer a different solution.  And in this case, it sounds like
a possible solution already exists, see above.

bug#61432: 28.2; [PATCH] viper-init: disable face support

[andrés ramírez]

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

Hi. Eli.

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


[...]


    Eli> The easiest one is this:

    Eli>   (require 'viper-init) (setq viper-minibuffer-vi-face viper-minibuffer-emacs-face)

    Eli> Did you try that?

I have tried it now. It is not giving me the right behaviour.

If You want to try that behaviour run:
emacs -Q -nw and C-x C-f (watch the faces on the prompt)
emacs -Q -nw -l /tmp/test-viper-loading.el


[-- Attachment #2: myel --]
[-- Type: text/plain, Size: 206 bytes --]

(require 'viper-init)
(setq viper-minibuffer-vi-face viper-minibuffer-emacs-face)
(setq viper-mode t)      
(setq viper-inhibit-startup-message 't)      
(setq viper-expert-level '5)      
(require 'viper)

[-- Attachment #3: Type: text/plain, Size: 456 bytes --]



[...]


    Eli> My problem with this is twofold:

    Eli>   . I don't like to have code in Emacs which "works around" our own code
    Eli> . viper-has-face-support-p is used for more than just the mode line

Mmm. Then perhaps the patch should be changed to integrate better with
the rest.

    Eli> So I'd prefer a different solution.  And in this case, it sounds like a possible solution
    Eli> already exists, see above.

See above.

Best Regards

bug#61432: 28.2; [PATCH] viper-init: disable face support

[Eli Zaretskii]

> From: andrés ramírez <rrandresf@hotmail.com>
> Cc: 61432@debbugs.gnu.org
> Date: Sat, 11 Feb 2023 18:32:36 +0000
> 
>     Eli> The easiest one is this:
>     Eli>   (require 'viper-init) (setq viper-minibuffer-vi-face viper-minibuffer-emacs-face)
>     Eli> Did you try that?
> 
> I have tried it now. It is not giving me the right behaviour.
> 
> If You want to try that behaviour run:
> emacs -Q -nw and C-x C-f (watch the faces on the prompt)
> emacs -Q -nw -l /tmp/test-viper-loading.el

What is the "right behavior"?  Which face do you want to avoid seeing?
maybe it's viper-minibuffer-insert?  Then try this instead:

  (require 'viper-init)
  (setq viper-minibuffer-insert-face viper-minibuffer-emacs-face)
  (setq viper-mode t)      
  (setq viper-inhibit-startup-message 't)      
  (setq viper-expert-level '5)      
  (require 'viper)

Anyway, the above is just a hint: how you can arrange for the viper
faces do whatever you like.

bug#61432: 28.2; [PATCH] viper-init: disable face support

[andrés ramírez]

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

Hi. Eli.
>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

    Eli> The easiest one is this: (require 'viper-init) (setq viper-minibuffer-vi-face
    Eli> viper-minibuffer-emacs-face) Did you try that?

Yes. without success.

    Eli> What is the "right behavior"?  

The right behaviour is staying as If it were an 'emacs -Q'. minibuffer
faces should stay as by default on vanilla emacs.

    Eli> Which face do you want to avoid seeing?  

I would like minibuffer faces stay as vanilla emacs when activating viper.


[...]


    Eli> Anyway, the above is just a hint: how you can arrange for the viper faces do whatever you
    Eli> like.

Thanks. But I have not being able of having the faces on minibuffer not
changing after activating viper. 

What about a oneline patch?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: mypatch --]
[-- Type: text/x-patch, Size: 602 bytes --]

diff -u /tmp/viper/viper-cmd.el.bak /tmp/viper/viper-cmd.el
--- /tmp/viper/viper-cmd.el.bak	2023-02-12 17:45:05.775987386 +0000
+++ /tmp/viper/viper-cmd.el	2023-02-12 17:06:44.589089504 +0000
@@ -564,7 +564,7 @@
        ))
 
   ;; minibuffer faces
-  (if (viper-has-face-support-p)
+  (if (and (viper-has-face-support-p) (not (and (boundp 'viper-disable-face-support) viper-disable-face-support)))
       (setq viper-minibuffer-current-face
 	    (cond ((eq state 'emacs-state) viper-minibuffer-emacs-face)
 		  ((eq state 'vi-state) viper-minibuffer-vi-face)

Diff finished.  Sun Feb 12 17:45:44 2023

[-- Attachment #3: Type: text/plain, Size: 14 bytes --]


Best Regards

bug#61432: 28.2; [PATCH] viper-init: disable face support

[Eli Zaretskii]

> From: andrés ramírez <rrandresf@hotmail.com>
> Cc: 61432@debbugs.gnu.org
> Date: Sun, 12 Feb 2023 17:54:45 +0000
> 
>     Eli> What is the "right behavior"?  
> 
> The right behaviour is staying as If it were an 'emacs -Q'. minibuffer
> faces should stay as by default on vanilla emacs.

Then make all those faces be the same: default.

> What about a oneline patch?

You are suggesting a new user option?  That might be okay, but then
the patch should include a suitable defcustom.

bug#61432: 28.2; [PATCH] viper-init: disable face support

[andrés ramírez]

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

Hi. Eli.

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


[...]

    Eli> Then make all those faces be the same: default.

There would be more than oneline. And There is no guarantee it should
work. I have tried several variations and have not gotten the default
faces showed when launching 'emacs -Q'


    Eli> You are suggesting a new user option?  That might be okay, but then the patch should
    Eli> include a suitable defcustom.

Yes. I am suggesting a new user option. For not changing minibuffer
faces when using viper.

Attached the new version of the patch.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: mypatch --]
[-- Type: text/x-patch, Size: 995 bytes --]

diff -u /tmp/viper/viper-cmd.el.bak /tmp/viper/viper-cmd.el
--- /tmp/viper/viper-cmd.el.bak	2023-02-12 17:45:05.775987386 +0000
+++ /tmp/viper/viper-cmd.el	2023-02-12 18:48:17.986368416 +0000
@@ -564,7 +564,7 @@
        ))
 
   ;; minibuffer faces
-  (if (viper-has-face-support-p)
+  (if (and (viper-has-face-support-p) (not (and (boundp 'viper-disable-minibuffer-faces) viper-disable-minibuffer-faces)))
       (setq viper-minibuffer-current-face
 	    (cond ((eq state 'emacs-state) viper-minibuffer-emacs-face)
 		  ((eq state 'vi-state) viper-minibuffer-vi-face)
@@ -1923,6 +1923,12 @@
   :group 'viper-misc)
 
 
+(defcustom viper-disable-minibuffer-faces nil
+  "If non-nil, viper does change the minibuffer faces."
+  :type 'boolean
+  :group 'viper-misc)
+
+
 ;; Try to add a suitable suffix to files whose name ends with a `.'
 ;; Useful when the user hits RET on a non-completed file name.
 ;; Used as a minibuffer exit hook in read-file-name

Diff finished.  Sun Feb 12 18:48:25 2023

[-- Attachment #3: Type: text/plain, Size: 14 bytes --]


Best Regards

bug#61432: 28.2; [PATCH] viper-init: disable face support

[Robert Pluim]

>>>>> On Sun, 12 Feb 2023 18:53:05 +0000, andrés ramírez <rrandresf@hotmail.com> said:
    INKA>    ;; minibuffer faces
    INKA> -  (if (viper-has-face-support-p)
    INKA> +  (if (and (viper-has-face-support-p) (not (and (boundp 'viper-disable-minibuffer-faces) viper-disable-minibuffer-faces)))
    INKA>        (setq viper-minibuffer-current-face
    INKA>  	    (cond ((eq state 'emacs-state) viper-minibuffer-emacs-face)
    INKA>  		  ((eq state 'vi-state) viper-minibuffer-vi-face)
    INKA> @@ -1923,6 +1923,12 @@
    INKA>    :group 'viper-misc)
 
You donʼt need to check if 'viper-disable-minibuffer-faces'  is
bound. Itʼs a defcustom, it will always be bound.

    INKA> +(defcustom viper-disable-minibuffer-faces nil
    INKA> +  "If non-nil, viper does change the minibuffer faces."
    INKA> +  :type 'boolean
    INKA> +  :group 'viper-misc)
    INKA> +
    INKA> +

Setting things to 't' to disable functionality I always find
confusing. How about you invert it:

    (defcustom viper-enable-minibuffer-faces t
      "Whether viper changes the minibuffer faces."
      :type 'boolean
      :group 'viper-misc)

Thanks

Robert
-- 

bug#61432: 28.2; [PATCH] viper-init: disable face support

[andrés ramírez]

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

Hi. Robert.

>>>>> "Robert" == Robert Pluim <rpluim@gmail.com> writes:


[...]

 
    Robert> You donʼt need to check if 'viper-disable-minibuffer-faces' is bound. Itʼs a defcustom,
    Robert> it will always be bound.

Thanks for the clarification.

[...]


    Robert> Setting things to 't' to disable functionality I always find confusing. How about you
    Robert> invert it:

    Robert>     (defcustom viper-enable-minibuffer-faces t "Whether viper changes the minibuffer
    Robert> faces."  :type 'boolean :group 'viper-misc)

Done.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: mypatch --]
[-- Type: text/x-patch, Size: 930 bytes --]

diff -u /tmp/viper/viper-cmd.el.bak /tmp/viper/viper-cmd.el
--- /tmp/viper/viper-cmd.el.bak	2023-02-12 17:45:05.775987386 +0000
+++ /tmp/viper/viper-cmd.el	2023-02-12 18:48:17.986368416 +0000
@@ -564,7 +564,7 @@
        ))
 
   ;; minibuffer faces
-  (if (viper-has-face-support-p)
+  (if (and (viper-has-face-support-p) viper-enable-minibuffer-faces)
       (setq viper-minibuffer-current-face
 	    (cond ((eq state 'emacs-state) viper-minibuffer-emacs-face)
 		  ((eq state 'vi-state) viper-minibuffer-vi-face)
@@ -1923,6 +1923,12 @@
   :group 'viper-misc)
 
 
+(defcustom viper-enable-minibuffer-faces t
+  "Whether viper changes the minibuffer faces."
+  :type 'boolean
+  :group 'viper-misc)
+
+
 ;; Try to add a suitable suffix to files whose name ends with a `.'
 ;; Useful when the user hits RET on a non-completed file name.
 ;; Used as a minibuffer exit hook in read-file-name

Diff finished.  Sun Feb 12 18:48:25 2023

[-- Attachment #3: Type: text/plain, Size: 14 bytes --]


Best Regards

bug#61432: 28.2; [PATCH] viper-init: disable face support

[Robert Pluim]

>>>>> On Mon, 13 Feb 2023 15:02:52 +0000, andrés ramírez <rrandresf@hotmail.com> said:

 
    INKA> +(defcustom viper-enable-minibuffer-faces t
    INKA> +  "Whether viper changes the minibuffer faces."
    INKA> +  :type 'boolean
    INKA> +  :group 'viper-misc)
    INKA> +
    INKA> +

And one thing I forgot: defcustomʼs normally require a :version field,
but I see that *none* of the defcustomʼs in viper have one, so maybe
we could skip that here.

Robert
-- 

bug#61432: 28.2; [PATCH] viper-init: disable face support

[Eli Zaretskii]

bug#61432: 28.2; [PATCH] viper-init: disable face support

[Eli Zaretskii]

> From: andrés ramírez <rrandresf@hotmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,
> 	61432@debbugs.gnu.org
> Date: Mon, 13 Feb 2023 15:02:52 +0000
> 
>     Robert> You donʼt need to check if 'viper-disable-minibuffer-faces' is bound. Itʼs a defcustom,
>     Robert> it will always be bound.
> 
> Thanks for the clarification.
> 
> [...]
> 
> 
>     Robert> Setting things to 't' to disable functionality I always find confusing. How about you
>     Robert> invert it:
> 
>     Robert>     (defcustom viper-enable-minibuffer-faces t "Whether viper changes the minibuffer
>     Robert> faces."  :type 'boolean :group 'viper-misc)
> 
> Done.

Thanks, installed on master, with a few necessary adjustments.

bug#61432: 28.2; [PATCH] viper-init: disable face support

[Robert Pluim]

(restoring Eli and bug address)

>>>>> On Thu, 23 Feb 2023 14:47:42 +0000, andrés ramírez <rrandresf@hotmail.com> said:

    INKA> An I realized and additional line is needed (the last one in the patch)

    INKA> Let me know how should I proceed for amending the patch with the needed line.

Hi, I pushed your followup patch to master in your name (we never
rewrite history)

This was only a one liner, so it was easy, for bigger followup changes
itʼs best to have the second patch be on top of the first one (or
re-send two separate patches)

Robert
-- 

bug#61432: 28.2; [PATCH] viper-init: disable face support

[andrés ramírez]

Hi Robert. Hi Eli.
>>>>> "Robert" == Robert Pluim <rpluim@gmail.com> writes:


[...]

    Robert> Hi, I pushed your followup patch to master in your name (we never rewrite history)

Thanks Robert for the ammending. 

    Robert> This was only a one liner, so it was easy, for bigger followup changes itʼs best to have
    Robert> the second patch be on top of the first one (or re-send two separate patches)

Thanks for the information.

Thanks Eli for applying  the patch.
,---- [  ]
| http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=b3502b922821fd100fb86b8cda6a0a62fba7d394
`----

Best Regards