bug#49069: 28.0.50; [PATCH] Use display-buffer for re-builder

bug#49069: 28.0.50; [PATCH] Use display-buffer for re-builder

[pillule]

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


Hi, I wished re-builder was in a dedicated window (otherwise killing its buffer let you with an unwanted buffer in a narrow window), so I wrote a patch about it.
It also have the benefit to uses display-buffer and so, to allow user-customization.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: re-builder with display-buffer --]
[-- Type: text/x-diff, Size: 1589 bytes --]

From 388e4e63646af79b537e25fc92f3c3933819a18c Mon Sep 17 00:00:00 2001
From: Trust me I am a doctor <pillule@riseup.net>
Date: Thu, 17 Jun 2021 03:07:57 +0200
Subject: [PATCH] Use display-buffer for re-builder

* lisp/emacs-lisp/re-builder.el (re-builder): Uses 'display-buffer'
with 'display-buffer-in-direction' to display the reb-buffer. This
allow user-customizations. Add a dedication to its window so
killing this buffer actually quit its window.
---
 lisp/emacs-lisp/re-builder.el | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/lisp/emacs-lisp/re-builder.el b/lisp/emacs-lisp/re-builder.el
index 7d042a9102..39bb0b36ba 100644
--- a/lisp/emacs-lisp/re-builder.el
+++ b/lisp/emacs-lisp/re-builder.el
@@ -355,11 +355,13 @@ re-builder
       (reb-delete-overlays))
     (setq reb-target-buffer (current-buffer)
           reb-target-window (selected-window))
-    (select-window (or (get-buffer-window reb-buffer)
-		       (progn
-			 (setq reb-window-config (current-window-configuration))
-			 (split-window (selected-window) (- (window-height) 4)))))
-    (switch-to-buffer (get-buffer-create reb-buffer))
+    (select-window
+     (or (get-buffer-window reb-buffer)
+         (progn (setq reb-window-config (current-window-configuration))
+                (display-buffer (get-buffer-create reb-buffer)
+                                '((display-buffer-in-direction)
+                                  (direction . down)
+                                  (dedicated . t))))))
     (font-lock-mode 1)
     (reb-initialize-buffer)))
 
-- 
2.20.1


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


--

bug#49069: 28.0.50; [PATCH] Use display-buffer for re-builder

[pillule]

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


pillule <pillule@riseup.net> writes:

> Hi, I wished re-builder was in a dedicated window (otherwise killing its buffer let
> you with an unwanted buffer in a narrow window), so I wrote a patch about it.
> It also have the benefit to uses display-buffer and so, to allow user-customization.

This second one is more reliable when it comes to call re-builder from a side
window.
(the original version simply prevented that case since a side window is not
splittable)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Use display-buffer for re-builder --]
[-- Type: text/x-diff, Size: 1695 bytes --]

From a52953ef8ef146cd105ad6dee7882e0dc298512d Mon Sep 17 00:00:00 2001
From: Trust me I am a doctor <pillule@riseup.net>
Date: Thu, 17 Jun 2021 05:56:34 +0200
Subject: [PATCH] Use display-buffer with re-builder

* lisp/emacs-lisp/re-builder.el (re-builder): Uses 'display-buffer'
with 'display-buffer-in-direction' to display the reb-buffer. This
allow user-customizations and using windows usually not splitables.
Add a dedication to its window so killing this buffer actually quit
its window.
---
 lisp/emacs-lisp/re-builder.el | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/lisp/emacs-lisp/re-builder.el b/lisp/emacs-lisp/re-builder.el
index 7d042a9102..747f741091 100644
--- a/lisp/emacs-lisp/re-builder.el
+++ b/lisp/emacs-lisp/re-builder.el
@@ -355,11 +355,15 @@ re-builder
       (reb-delete-overlays))
     (setq reb-target-buffer (current-buffer)
           reb-target-window (selected-window))
-    (select-window (or (get-buffer-window reb-buffer)
-		       (progn
-			 (setq reb-window-config (current-window-configuration))
-			 (split-window (selected-window) (- (window-height) 4)))))
-    (switch-to-buffer (get-buffer-create reb-buffer))
+    (select-window
+     (or (get-buffer-window reb-buffer)
+         (progn (setq reb-window-config (current-window-configuration))
+                (display-buffer
+                 (get-buffer-create reb-buffer)
+                 `((display-buffer-in-direction)
+                   (direction . ,(if (eq (window-dedicated-p nil) 'side)
+                                     'bottom 'down))
+                   (dedicated . t))))))
     (font-lock-mode 1)
     (reb-initialize-buffer)))
 
-- 
2.20.1


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


I also tested it against the atomic windows showcased in the manual
and it seems ok, but I am not familiar with them.

bug#49069: 28.0.50; [PATCH] Use display-buffer for re-builder

[pillule]

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


pillule <pillule@riseup.net> writes:

> pillule <pillule@riseup.net> writes:
>
>> Hi, I wished re-builder was in a dedicated window (otherwise killing its buffer let
>> you with an unwanted buffer in a narrow window), so I wrote a patch about it.
>> It also have the benefit to uses display-buffer and so, to allow user-customization.
>
> This second one is more reliable when it comes to call re-builder from a side
> window.
> (the original version simply prevented that case since a side window is not
> splittable)
>
> I also tested it against the atomic windows showcased in the manual
> and it seems ok, but I am not familiar with them.

This one is a little bit more reliable (use window-parameter instead of window-dedicated-p) and cleaner I hope.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: with window-parameter --]
[-- Type: text/x-diff, Size: 1671 bytes --]

From f463239252116d15cee9878c2952b9825152852f Mon Sep 17 00:00:00 2001
From: Trust me I am a doctor <pillule@riseup.net>
Date: Thu, 17 Jun 2021 05:56:34 +0200
Subject: [PATCH] Use display-buffer with re-builder

* lisp/emacs-lisp/re-builder.el (re-builder): Uses 'display-buffer'
with 'display-buffer-in-direction' to display the reb-buffer.  This
allow user-customizations and using it on not splitables windows.
Add a dedication to its window so killing this buffer quit the window.
---
 lisp/emacs-lisp/re-builder.el | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/lisp/emacs-lisp/re-builder.el b/lisp/emacs-lisp/re-builder.el
index 7d042a9102..396949d59a 100644
--- a/lisp/emacs-lisp/re-builder.el
+++ b/lisp/emacs-lisp/re-builder.el
@@ -355,11 +355,16 @@ re-builder
       (reb-delete-overlays))
     (setq reb-target-buffer (current-buffer)
           reb-target-window (selected-window))
-    (select-window (or (get-buffer-window reb-buffer)
-		       (progn
-			 (setq reb-window-config (current-window-configuration))
-			 (split-window (selected-window) (- (window-height) 4)))))
-    (switch-to-buffer (get-buffer-create reb-buffer))
+    (select-window
+     (or (get-buffer-window reb-buffer)
+         (let ((dir (if (window-parameter nil 'window-side)
+                        'bottom 'down)))
+           (setq reb-window-config (current-window-configuration))
+           (display-buffer
+            (get-buffer-create reb-buffer)
+            `((display-buffer-in-direction)
+              (direction . ,dir)
+              (dedicated . t))))))
     (font-lock-mode 1)
     (reb-initialize-buffer)))
 
-- 
2.20.1


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


--

bug#49069: 28.0.50; [PATCH] Use display-buffer for re-builder

[Trust me I am a Doctor]

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


But maybe we don't care at all about user customizations, the point being
rather to stop the re-builder window to be stealed by other buffers, and
to clean it on kill-buffer, then we could use
'display-buffer-overriding-actions'.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: pop re-buffer in a dedicated window --]
[-- Type: text/x-diff, Size: 1678 bytes --]

From b4630049c6b1c4b1704fb463edfa8140ed4df729 Mon Sep 17 00:00:00 2001
From: Trust me I am a doctor <pillule@riseup.net>
Date: Tue, 29 Jun 2021 17:46:07 +0200
Subject: [PATCH] Pop re-buffer in a dedicated window

* lisp/emacs-lisp/re-builder.el (re-builder): Uses
'display-buffer-overriding-action' to pop 're-buffer' in a dedicated
window, also make possible to call it from a side window.
---
 lisp/emacs-lisp/re-builder.el | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/lisp/emacs-lisp/re-builder.el b/lisp/emacs-lisp/re-builder.el
index 7d042a9102..331f17cb5e 100644
--- a/lisp/emacs-lisp/re-builder.el
+++ b/lisp/emacs-lisp/re-builder.el
@@ -355,11 +355,15 @@ re-builder
       (reb-delete-overlays))
     (setq reb-target-buffer (current-buffer)
           reb-target-window (selected-window))
-    (select-window (or (get-buffer-window reb-buffer)
-		       (progn
-			 (setq reb-window-config (current-window-configuration))
-			 (split-window (selected-window) (- (window-height) 4)))))
-    (switch-to-buffer (get-buffer-create reb-buffer))
+    (select-window
+     (or (get-buffer-window reb-buffer)
+         (let* ((dir (if (window-parameter nil 'window-side)
+                         'bottom 'down))
+                (display-buffer-overriding-action `((display-buffer-in-direction)
+                                                    (direction . ,dir)
+                                                    (dedicated . t))))
+           (setq reb-window-config (current-window-configuration))
+           (pop-to-buffer (get-buffer-create reb-buffer)))))
     (font-lock-mode 1)
     (reb-initialize-buffer)))
 
-- 
2.20.1

bug#49069: 28.0.50; [PATCH] Use display-buffer for re-builder

[Juri Linkov]

> But maybe we don't care at all about user customizations, the point being
> rather to stop the re-builder window to be stealed by other buffers, and
> to clean it on kill-buffer, then we could use
> 'display-buffer-overriding-actions'.

Sorry, preventing the users from customizing windows is the wrong thing.

Your previous patch was much better.  If you have no more improvements
for the previous patch, then it could be pushed.

bug#49069: 28.0.50; [PATCH] Use display-buffer for re-builder

[Trust me I am a Doctor]

Juri Linkov <juri@linkov.net> writes:

>> But maybe we don't care at all about user customizations, the point being
>> rather to stop the re-builder window to be stealed by other buffers, and
>> to clean it on kill-buffer, then we could use
>> 'display-buffer-overriding-actions'.
>
> Sorry, preventing the users from customizing windows is the wrong thing.

I don't know really, let me explain my rational : some users have a very
constrained display-buffer-alist and match ".*". You can see such
rational explained at the end of this page :

https://github.com/nex3/perspective-el

This kind of constraint may be even more important for people with low
vision, as emacs poping windows on a display of 40 columns by 9 lines
is very disruptive.

So with a display-buffer-alist matching ".*", the rule will pop the
re-buffer as the main window, and this create a poor interaction :
the targeted buffer not being visible anymore, the re-buffer taking
a full window.

If really the user customization of the display of re-builder is a
thing, I could put a variable as user-customization to split the apple
in two.

But I may be wrong, after all that's not my case.
Anyway I put a pop-to-buffer in the last patch where I should kept
display-buffer.

bug#49069: 28.0.50; [PATCH] Use display-buffer for re-builder

[Juri Linkov]

>> Sorry, preventing the users from customizing windows is the wrong thing.
>
> I don't know really, let me explain my rational : some users have a very
> constrained display-buffer-alist and match ".*". You can see such
> rational explained at the end of this page :
>
> https://github.com/nex3/perspective-el

Thanks for the reference.  Now I understand the reason for such absurdity
in org-mode:

  (defmacro org-no-popups (&rest body)
    "Suppress popup windows and evaluate BODY."
    `(let (pop-up-frames display-buffer-alist)
       ,@body))

So different packages are fighting with each other:
one package recommends using display-buffer-alist to display
all buffers only in the same window, another package
throws away user customization on such grounds
that users might follow such poor recommendations.

The correct way to display all buffers in the same window
is documented in (info "(emacs) Window Choice") where
display-buffer-base-action should be customized like this:

  (customize-set-variable
   'display-buffer-base-action
   '((display-buffer-reuse-window display-buffer-same-window)))

And the problem is solved.  So please avoid ignoring user customization
with display-buffer-overriding-action because users with such
display-buffer-base-action will be able to customize "*RE-Builder*" with
display-buffer-alist.

bug#49069: 28.0.50; [PATCH] Use display-buffer for re-builder

[Trust me I am a Doctor]

Juri Linkov <juri@linkov.net> writes:

>>> Sorry, preventing the users from customizing windows is the wrong thing.
>>
>> I don't know really, let me explain my rational : some users have a very
>> constrained display-buffer-alist and match ".*". You can see such
>> rational explained at the end of this page :
>>
>> https://github.com/nex3/perspective-el
>
> Thanks for the reference.  Now I understand the reason for such absurdity
> in org-mode:
>
>   (defmacro org-no-popups (&rest body)
>     "Suppress popup windows and evaluate BODY."
>     `(let (pop-up-frames display-buffer-alist)
>        ,@body))
>
> So different packages are fighting with each other:
> one package recommends using display-buffer-alist to display
> all buffers only in the same window, another package
> throws away user customization on such grounds
> that users might follow such poor recommendations.

To be fair, blaming the different repositories gives no evidence that
this macro is a reaction to perspective.el

I CC its maintainer,
Hi Constantine Vetoshev,
While we was discussing window configurations, I introduced the
recommendations of your README that I used to follow in a particular
situation; in regard of the remarks of Juri, you maybe want to update
them?

> The correct way to display all buffers in the same window
> is documented in (info "(emacs) Window Choice") where
> display-buffer-base-action should be customized like this:
>
>   (customize-set-variable
>    'display-buffer-base-action
>    '((display-buffer-reuse-window display-buffer-same-window)))
>
> And the problem is solved.  So please avoid ignoring user customization
> with display-buffer-overriding-action because users with such
> display-buffer-base-action will be able to customize "*RE-Builder*" with
> display-buffer-alist.

Ok you convinced me. Thanks for the tip, I installed it on a low-vision
setup of a knowledge of me. We will see more in detail the differences
that it implies.

So it seems that the patch of the Thu, 17 Jun 2021 16:50:07 +0200
was indeed the one we want to keep.

I tested it again, with :

(customize-set-variable
 'display-buffer-base-action
 '((display-buffer-reuse-window display-buffer-same-window)))

(setf (alist-get "*RE-Builder*" display-buffer-alist nil nil #'string=)
      `((display-buffer-in-direction)
        (direction . top)
        (dedicated . t)))

(re-builder)

And it obeys to display-buffer-alist, be it for an other
function or others parameters, and keep being functional ... etc.
--

bug#49069: 28.0.50; [PATCH] Use display-buffer for re-builder

[Juri Linkov]

tags 49069 fixed
close 49069 28.0.50
thanks

> To be fair, blaming the different repositories gives no evidence that
> this macro is a reaction to perspective.el

Indeed, we don't know for sure, it was just a hypothesis.
But anyway such "catch-all" tricks as ".*" should be avoided.

Also I don't know if there is any chance to fix Org-mode.
Anyway, I'm using such workaround for Org-mode problem:

#+begin_src emacs-lisp
;; Unfortunately, Org window customization has no effect because
;; ‘display-buffer-alist’ is ignored in ‘org-no-popups’
;; until ‘org-switch-to-buffer-other-window’ is overridden below:
(defalias 'org-switch-to-buffer-other-window 'switch-to-buffer-other-window)

;; This allows such user customizations:
(push '("\\`\\*Org \\(Links\\|Select\\)\\*\\'"
        display-buffer-below-selected
        (window-height . fit-window-to-buffer)
        (preserve-size . (nil . t)))
      display-buffer-alist)
#+end_src

> Ok you convinced me. Thanks for the tip, I installed it on a low-vision
> setup of a knowledge of me. We will see more in detail the differences
> that it implies.
>
> So it seems that the patch of the Thu, 17 Jun 2021 16:50:07 +0200
> was indeed the one we want to keep.
>
> I tested it again, with :
>
> (customize-set-variable
>  'display-buffer-base-action
>  '((display-buffer-reuse-window display-buffer-same-window)))
>
> (setf (alist-get "*RE-Builder*" display-buffer-alist nil nil #'string=)
>       `((display-buffer-in-direction)
>         (direction . top)
>         (dedicated . t)))
>
> (re-builder)
>
> And it obeys to display-buffer-alist, be it for an other
> function or others parameters, and keep being functional ... etc.

Thanks for confirming and for implementing the patch.
Now your patch was pushed in commit 57354bc64b.

bug#49069: 28.0.50; [PATCH] Use display-buffer for re-builder

[Constantine Vetoshev]

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

Thank you for bringing this to my attention. I’ll test the display-buffer-base-action approach over the next couple of days, and if it works as I expect, I’ll update the Perspective README.

Note that Juri’s example is not quite the same as mine. Please test with this instead:

(customize-set-variable 'display-buffer-base-action
 '((display-buffer-reuse-window display-buffer-same-window)
 (reusable-frames . t)))

That said: it seems to me that rebinding display-buffer-overriding-action is intended for this very purpose: a judicious, rarely-used, and highly-targeted change to display-buffer behavior where it is imperative for the user’s customizations to not have an undesirable effect.
On Jul 8, 2021, 08:40 -0700, Trust me I am a Doctor <pillule@riseup.net>, wrote:
>
> Juri Linkov <juri@linkov.net> writes:
>
> > > > Sorry, preventing the users from customizing windows is the wrong thing.
> > >
> > > I don't know really, let me explain my rational : some users have a very
> > > constrained display-buffer-alist and match ".*". You can see such
> > > rational explained at the end of this page :
> > >
> > > https://github.com/nex3/perspective-el
> >
> > Thanks for the reference. Now I understand the reason for such absurdity
> > in org-mode:
> >
> > (defmacro org-no-popups (&rest body)
> > "Suppress popup windows and evaluate BODY."
> > `(let (pop-up-frames display-buffer-alist)
> > ,@body))
> >
> > So different packages are fighting with each other:
> > one package recommends using display-buffer-alist to display
> > all buffers only in the same window, another package
> > throws away user customization on such grounds
> > that users might follow such poor recommendations.
>
> To be fair, blaming the different repositories gives no evidence that
> this macro is a reaction to perspective.el
>
> I CC its maintainer,
> Hi Constantine Vetoshev,
> While we was discussing window configurations, I introduced the
> recommendations of your README that I used to follow in a particular
> situation; in regard of the remarks of Juri, you maybe want to update
> them?
>
> > The correct way to display all buffers in the same window
> > is documented in (info "(emacs) Window Choice") where
> > display-buffer-base-action should be customized like this:
> >
> > (customize-set-variable
> > 'display-buffer-base-action
> > '((display-buffer-reuse-window display-buffer-same-window)))
> >
> > And the problem is solved. So please avoid ignoring user customization
> > with display-buffer-overriding-action because users with such
> > display-buffer-base-action will be able to customize "*RE-Builder*" with
> > display-buffer-alist.
>
> Ok you convinced me. Thanks for the tip, I installed it on a low-vision
> setup of a knowledge of me. We will see more in detail the differences
> that it implies.
>
> So it seems that the patch of the Thu, 17 Jun 2021 16:50:07 +0200
> was indeed the one we want to keep.
>
> I tested it again, with :
>
> (customize-set-variable
> 'display-buffer-base-action
> '((display-buffer-reuse-window display-buffer-same-window)))
>
> (setf (alist-get "*RE-Builder*" display-buffer-alist nil nil #'string=)
> `((display-buffer-in-direction)
> (direction . top)
> (dedicated . t)))
>
> (re-builder)
>
> And it obeys to display-buffer-alist, be it for an other
> function or others parameters, and keep being functional ... etc.
> --

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

bug#49069: 28.0.50; [PATCH] Use display-buffer for re-builder

[Trust me I am a Doctor]

Juri Linkov <juri@linkov.net> writes:

> Also I don't know if there is any chance to fix Org-mode.
> Anyway, I'm using such workaround for Org-mode problem:
>
> #+begin_src emacs-lisp
> ;; Unfortunately, Org window customization has no effect because
> ;; ‘display-buffer-alist’ is ignored in ‘org-no-popups’
> ;; until ‘org-switch-to-buffer-other-window’ is overridden below:
> (defalias 'org-switch-to-buffer-other-window 'switch-to-buffer-other-window)
>
> ;; This allows such user customizations:
> (push '("\\`\\*Org \\(Links\\|Select\\)\\*\\'"
>         display-buffer-below-selected
>         (window-height . fit-window-to-buffer)
>         (preserve-size . (nil . t)))
>       display-buffer-alist)
> #+end_src

Nice, I was aware of overriding 'org-switch-to-buffer-other-window but
didn't customized anything yet for theses buffers.


Constantine Vetoshev <gepardcv@gmail.com> writes:

> Thank you for bringing this to my attention. I’ll test the display-buffer-base-action approach over
> the next couple of days, and if it works as I expect, I’ll update the Perspective README.
>
> Note that Juri’s example is not quite the same as mine. Please test with this instead:
>
> (customize-set-variable 'display-buffer-base-action
>  '((display-buffer-reuse-window display-buffer-same-window)
>  (reusable-frames . t)))

Ok I will update the recipe with yours -even if until now I had no issue
with emacs creating new frames. (I know ediff do it but I also now the
setting to tweak).

For now the only package that seems really difficult to work with is Magit;
transient.el is a brillant idea until you cannot see its whole windows,
but that's beyond the scope of this patch ;

Thank you both.

--

bug#49069: Fwd: Re: bug#49069: 28.0.50; [PATCH] Use display-buffer for re-builder

[Trust me I am a Doctor]

Constantine Vetoshev <gepardcv@gmail.com> writes:

> I dont have any problems with transient windows in Magit with my
> settings. They pop up just as they should. Pretty sure the magit code
> uses display-buffer-overriding-action. What bad behavior are you
> seeing?

That is in perspective to help a low vision knowledge of me. Magit
works, yes, but his screen view is 40 lines by 9 column, and so, he
cannot view all the content of the transient windows, and cannot
navigate easily inside, nor inspect the current keybindings as opposed
as a regular keymap ; that is defeating the purpose of Magit to
facilitate the interaction with git for him.

For that reason we tend to look more into the builtin vc, that is more
easily introspectable, and have a much simpler window system.

With enough thought, we could improve the accessibility of Transient and
Magit for low vision people, I have opened an issue at this subject on
the repository of Transient, eventually someday I will take the time to
look deeper into transient.el and do something about that.
--