Undo mode

Undo mode

[Juri Linkov]

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

It would be more convenient to have a special mode `undo-mode'
that is disabled by default.

To enable undo-mode, it will possible to select from the main menu:

  Edit >
   Enable Undo Mode

Then after enabling it, more menu items will appear on the menu:

  Edit >
   Disable Undo Mode
   Undo
   Redo

Also the current number of undo-limit will be hard-coded
for more convenience:

diff --git a/src/undo.c b/src/undo.c
index 5d705945c4..510d6e8692 100644
--- a/src/undo.c
+++ b/src/undo.c
@@ -386,7 +386,7 @@ truncate_undo_list (struct buffer *b)
 	  if (size_so_far > undo_strong_limit)
 	    break;
 	  last_boundary = prev;
-	  if (size_so_far > undo_limit)
+	  if (size_so_far > 160000)
 	    break;
 	}

Does this make sense?

If not, wouldn't be more preferable the following patch
that does the opposite?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: undelete-frame-max.patch --]
[-- Type: text/x-diff, Size: 8199 bytes --]

diff --git a/lisp/frame.el b/lisp/frame.el
index 5926a4d748..599ffe591a 100644
--- b/lisp/frame.el
+++ a/lisp/frame.el
@@ -2529,6 +2529,13 @@
         (if iconify (iconify-frame this) (delete-frame this)))
       (setq this next))))
 
+\f
+(defcustom undelete-frame-max 1
+  "Maximum number of deleted frames before oldest are thrown away."
+  :type 'integer
+  :group 'frames
+  :version "29.1")
+
 (eval-when-compile (require 'frameset))
 
 (defvar undelete-frame--deleted-frames nil
@@ -2536,7 +2543,7 @@
 
 (defun undelete-frame--handle-delete-frame (frame)
   "Save the configuration of frames deleted with `delete-frame'.
-Only the 16 most recently deleted frames are saved."
+Only the `undelete-frame-max' most recently deleted frames are saved."
   (when (frame-live-p frame)
     (setq undelete-frame--deleted-frames
           (cons
@@ -2555,54 +2562,45 @@
                         (cons '(display . :never)
                               frameset-filter-alist))))
            undelete-frame--deleted-frames))
-    (if (> (length undelete-frame--deleted-frames) 16)
+    (if (> (length undelete-frame--deleted-frames) undelete-frame-max)
         (setq undelete-frame--deleted-frames
               (butlast undelete-frame--deleted-frames)))))
 
-(define-minor-mode undelete-frame-mode
-  "Enable the `undelete-frame' command."
-  :group 'frames
-  :global t
-  (if undelete-frame-mode
-      (add-hook 'delete-frame-functions
-                #'undelete-frame--handle-delete-frame -75)
-    (remove-hook 'delete-frame-functions
-                 #'undelete-frame--handle-delete-frame)
-    (setq undelete-frame--deleted-frames nil)))
+(add-hook 'after-init-hook
+          (lambda ()
+            (add-hook 'delete-frame-functions
+                      #'undelete-frame--handle-delete-frame -75)))
 
 (defun undelete-frame (&optional arg)
   "Undelete a frame deleted with `delete-frame'.
-Without a prefix argument, undelete the most recently deleted
-frame.
-With a numerical prefix argument ARG between 1 and 16, where 1 is
-most recently deleted frame, undelete the ARGth deleted frame.
+Without a prefix argument, undelete the most recently deleted frame.
+With a numerical prefix argument ARG between 1 and `undelete-frame-max',
+where 1 is most recently deleted frame, undelete the ARGth deleted frame.
 When called from Lisp, returns the new frame."
   (interactive "P")
-  (if (not undelete-frame-mode)
-      (user-error "Undelete-Frame mode is disabled")
-    (if (consp arg)
-        (user-error "Missing deleted frame number argument")
-      (let* ((number (pcase arg ('nil 1) ('- -1) (_ arg)))
-             (frames (frame-list))
-             (frameset (nth (1- number) undelete-frame--deleted-frames))
-             (graphic (display-graphic-p)))
-        (if (not (<= 1 number 16))
-            (user-error "%d is not a valid deleted frame number argument"
-                        number)
-          (if (not frameset)
-              (user-error "No deleted frame with number %d" number)
-            (if (not (eq graphic (car frameset)))
-                (user-error
-                 "Cannot undelete a %s display frame on a %s display"
-                 (if graphic "non-graphic" "graphic")
-                 (if graphic "graphic" "non-graphic"))
-              (setq undelete-frame--deleted-frames
-                    (delq frameset undelete-frame--deleted-frames))
-              (frameset-restore (cdr frameset))
-              (let ((frame (car (seq-difference (frame-list) frames))))
-                (when frame
-                  (select-frame-set-input-focus frame)
-                  frame)))))))))
+  (if (consp arg)
+      (user-error "Missing deleted frame number argument")
+    (let* ((number (pcase arg ('nil 1) ('- -1) (_ arg)))
+           (frames (frame-list))
+           (frameset (nth (1- number) undelete-frame--deleted-frames))
+           (graphic (display-graphic-p)))
+      (if (not (<= 1 number undelete-frame-max))
+          (user-error "%d is not a valid deleted frame number argument"
+                      number)
+        (if (not frameset)
+            (user-error "No deleted frame with number %d" number)
+          (if (not (eq graphic (car frameset)))
+              (user-error
+               "Cannot undelete a %s display frame on a %s display"
+               (if graphic "non-graphic" "graphic")
+               (if graphic "graphic" "non-graphic"))
+            (setq undelete-frame--deleted-frames
+                  (delq frameset undelete-frame--deleted-frames))
+            (frameset-restore (cdr frameset))
+            (let ((frame (car (seq-difference (frame-list) frames))))
+              (when frame
+                (select-frame-set-input-focus frame)
+                frame))))))))
 \f
 ;;; Window dividers.
 (defgroup window-divider nil
diff --git a/lisp/menu-bar.el b/lisp/menu-bar.el
index e5a070b24a..36cbd6a9c5 100644
--- b/lisp/menu-bar.el
+++ a/lisp/menu-bar.el
@@ -109,14 +109,9 @@
       (bindings--define-key menu [separator-tab]
         menu-bar-separator))
 
-    (bindings--define-key menu [enable-undelete-frame-mode]
-      '(menu-item "Enable Undeleting Frames" undelete-frame-mode
-                  :visible (null undelete-frame-mode)
-                  :help "Enable undeleting frames in this session"))
     (bindings--define-key menu [undelete-last-deleted-frame]
       '(menu-item "Undelete Frame" undelete-frame
-                  :visible (and undelete-frame-mode
-                                (car undelete-frame--deleted-frames))
+                  :visible (car undelete-frame--deleted-frames)
                   :help "Undelete the most recently deleted frame"))
 
     ;; Don't use delete-frame as event name because that is a special
diff --git a/src/frame.c b/src/frame.c
index 959f0c9c14..e5d74edc16 100644
--- b/src/frame.c
+++ a/src/frame.c
@@ -2385,7 +2385,7 @@
        doc: /* Delete FRAME, eliminating it from use.
 FRAME must be a live frame and defaults to the selected one.
 
-When `undelete-frame-mode' is enabled, the 16 most recently deleted
+When `undelete-frame-max' is more than 0, the most recently deleted
 frames can be undeleted with `undelete-frame', which see.
 
 A frame may not be deleted if its minibuffer serves as surrogate
diff --git a/doc/emacs/frames.texi b/doc/emacs/frames.texi
index ba58f70caf..c641b8ccb1 100644
--- b/doc/emacs/frames.texi
+++ a/doc/emacs/frames.texi
@@ -515,12 +515,14 @@
 @item C-x 5 u
 @kindex C-x 5 u
 @findex undelete-frame
-@findex undelete-frame-mode
-When @code{undelete-frame-mode} is enabled, undelete one of the 16
-most recently deleted frames.  Without a prefix argument, undelete the
-most recently deleted frame.  With a numerical prefix argument between
-1 and 16, where 1 is the most recently deleted frame, undelete the
-corresponding deleted frame.
+@findex undelete-frame-max
+Undelete one of the recently deleted frames.  The user option
+@code{undelete-frame-max} specifies the maximum number of deleted
+frames to keep (the default is 1).  Without a prefix argument,
+undelete the most recently deleted frame.  With a numerical prefix
+argument between 1 and the number specified by @code{undelete-frame-max},
+where 1 is the most recently deleted frame, undelete the corresponding
+deleted frame.
 
 @item C-z
 @kindex C-z @r{(X windows)}
diff --git a/etc/NEWS b/etc/NEWS
index fdbfd9b1be..2e748ce7c5 100644
--- b/etc/NEWS
+++ a/etc/NEWS
@@ -287,11 +287,12 @@
 
 +++
 *** Deleted frames can now be undeleted.
-The 16 most recently deleted frames can be undeleted with 'C-x 5 u' when
-'undelete-frame-mode' is enabled.  Without a prefix argument, undelete
-the most recently deleted frame.  With a numerical prefix argument
-between 1 and 16, where 1 is the most recently deleted frame, undelete
-the corresponding deleted frame.
+The most recently deleted frame can be undeleted with 'C-x 5 u' when
+the new user option 'undelete-frame-max' has its default value 1.
+Without a prefix argument, undelete the most recently deleted frame.
+With a numerical prefix argument between 1 and 'undelete-frame-max',
+where 1 is the most recently deleted frame, undelete the corresponding
+deleted frame.
 

Re: Undo mode

[Stefan Monnier]

> Does this make sense?

Sounds like Antinews alright ;-)

> If not, wouldn't be more preferable the following patch
> that does the opposite?

I generally agree, yes.

> +(defcustom undelete-frame-max 1
> +  "Maximum number of deleted frames before oldest are thrown away."
> +  :type 'integer
> +  :group 'frames
> +  :version "29.1")

Any reason why this default is so low?
Is it really that expensive to keep old frames's info?

>  (defun undelete-frame--handle-delete-frame (frame)

Could you find a better name for that function?
I mean a name that describes what the function does, rather than
where/when it's used?

> +(add-hook 'after-init-hook
> +          (lambda ()
> +            (add-hook 'delete-frame-functions
> +                      #'undelete-frame--handle-delete-frame -75)))

Why do we need to postpone it via `after-init-hook`?


        Stefan

Re: Undo mode

[Po Lu]

Juri Linkov <juri@linkov.net> writes:

> If not, wouldn't be more preferable the following patch
> that does the opposite?

IMO, consistency doesn't matter at all when the ship has already sailed.
`undelete-frame-mode' has been here for nearly a week, so removing it
should be out of the question.

Introducing an option to allow customizing the amount of frames that are
kept is okay, however.

RE: [External] : Re: Undo mode

[Drew Adams]

> IMO, consistency doesn't matter at all when the ship has already sailed.
> `undelete-frame-mode' has been here for nearly a week, so removing it
> should be out of the question.

I'm not saying anything about this feature or any
ship on which someone thinks something has already
sailed.

But if something isn't in an Emacs release (the
latest is 27.2) then I'd say it hasn't sailed.

Something that someone threw into master or
whatever "nearly a week" ago hasn't even been
loaded onto a ship yet, IMHO.

It might be, but it isn't necessarily, sitting
on some dock somewhere.

And yes, it should be possible to remove it
from any dock or any ship it's sitting on.

Re: [External] : Re: Undo mode

[Po Lu]

Drew Adams <drew.adams@oracle.com> writes:

> I'm not saying anything about this feature or any
> ship on which someone thinks something has already
> sailed.
>
> But if something isn't in an Emacs release (the
> latest is 27.2) then I'd say it hasn't sailed.

What if someone tracking master (and there are enough of them to make a
difference, I think) has customized `undelete-frame-mode' to t?

That's the specific case I was thinking of.

RE: [External] : Re: Undo mode

[Drew Adams]

> > I'm not saying anything about this feature or any
> > ship on which someone thinks something has already
> > sailed.
> >
> > But if something isn't in an Emacs release (the
> > latest is 27.2) then I'd say it hasn't sailed.
> 
> What if someone tracking master (and there are enough of them to make a
> difference, I think) has customized `undelete-frame-mode' to t?
> 
> That's the specific case I was thinking of.

Bleeding edge.  Not a sailing ship.

(And again, I'm not saying anything
about this feature.)

Re: Undo mode

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Thu, 20 Jan 2022 22:45:00 +0200
> 
> If not, wouldn't be more preferable the following patch
> that does the opposite?

We are not going to enable undelete-frame-mode by default.  People who
do want it enabled by default can do so in their customizations, so I
see no problem here that we need to solve.

Tracking master (was: [External] : Re: Undo mode)

[Kévin Le Gouguec]

Po Lu <luangruo@yahoo.com> writes:

> Drew Adams <drew.adams@oracle.com> writes:
>
>> I'm not saying anything about this feature or any
>> ship on which someone thinks something has already
>> sailed.
>>
>> But if something isn't in an Emacs release (the
>> latest is 27.2) then I'd say it hasn't sailed.
>
> What if someone tracking master (and there are enough of them to make a
> difference, I think) has customized `undelete-frame-mode' to t?
>
> That's the specific case I was thinking of.

As someone using master as my daily driver for years: I have zero
expectation that features introduced on this development branch will
still be around next week, in their current shape, or in any shape at
all.

My only hope (and still, not an expectation) is that good features will
be discussed and reworked into their best possible iteration by the time
a release is cut.

(Luckily, since collectively, Emacs hackers are as considerate as they
are innovative, I can generally start sitting on newly-installed rugs
without worrying that they'll be pulled from under me.  As a
bleeding-edge user, this brings me a level of comfort that is well worth
the occasional .emacs adjusment)

While patches can and are discussed on the bugtracker before being
applied to master, and feature branches can and are discussed before
being merged, they don't get nearly as much exposure as they do once
they land on master.  For that reason alone, I think it'd be a waste to
miss the chance of using master to gather further feedback and iterate
more before committing to the maintenance burden.

(Although take all that with a grain of salt: I can't remember if the
ship departure schedule has been formally spelled out.  My expectations
(or lack thereof) regarding master might not reflect the maintainers'
policy)

Re: Undo mode

[Juri Linkov]

>> Does this make sense?
>
> Sounds like Antinews alright ;-)
>
>> If not, wouldn't be more preferable the following patch
>> that does the opposite?
>
> I generally agree, yes.
>
>> +(defcustom undelete-frame-max 1
>> +  "Maximum number of deleted frames before oldest are thrown away."
>> +  :type 'integer
>> +  :group 'frames
>> +  :version "29.1")
>
> Any reason why this default is so low?
> Is it really that expensive to keep old frames's info?

If the memory consumption is not a problem, it could be increased.
But 1 is the minimum value that it useful for all users:
when the user accidentally deletes a frame, it will be possible
to undelete it immediately, without enabling some special mode.

>>  (defun undelete-frame--handle-delete-frame (frame)
>
> Could you find a better name for that function?
> I mean a name that describes what the function does, rather than
> where/when it's used?

Maybe `undelete-frame--save-deleted-frame', I will change this
in the next version of the patch.

>> +(add-hook 'after-init-hook
>> +          (lambda ()
>> +            (add-hook 'delete-frame-functions
>> +                      #'undelete-frame--handle-delete-frame -75)))
>
> Why do we need to postpone it via `after-init-hook`?

The problem is that during Emacs GUI startup, some temporary TTY frame
is created and deleted.  Maybe this is a bug?

Re: Undo mode

[Juri Linkov]

> `undelete-frame-mode' has been here for nearly a week, so removing it
> should be out of the question.

If the entire history of Emacs was compressed into a single 24-hour day,
then a week will last only 30 seconds!

Re: Undo mode

[Gregory Heytings]

It is deeply saddening to see that the "Undelete Frame" feature has become 
the subject of a quarrel.

The original design added a single "Undelete Frame" item in the File menu, 
under the "Delete Frame" item.  The conclusion of the discussion in 
bug#51883 was that this feature shouldn't be enabled by default, which 
means that this "Undelete Frame" menu item would have remained inactive, 
unlike other items in the menus whose state change from inactive to active 
when their actions become feasible.

Users could have reasonably expected to see this menu item become active 
after they used the "Delete Frame" menu item, just like the "Remove Other 
Windows" item becomes active after using one of the "New Window" items.

Therefore, to help users discover that this menu item required an explicit 
activation in their init files to become usable, I added a "Enable Frame 
Undeletion" item in the File menu, with a tooltip which said "Enable frame 
undeletion for this session", and which replaced the "Undelete Frame" item 
when undelete-frame-mode was not active.  IOW, this feature always used 
one and only one item in the File menu: either "Enable Frame Undeletion", 
or "Undelete Frame".

The hard-coded limit of 16 deleted frames was also a conscious design 
choice, to avoid adding yet another defcustom that nobody had asked (yet). 
A user might want to undelete a few frames, so recording a single deleted 
frame is not enough, and 16 seemed like a reasonable upper limit that 
should cover all needs.

I respectfully suggest to go back to that original design.

Re: Undo mode

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

> If not, wouldn't be more preferable the following patch
> that does the opposite?

Yes, I'd prefer your patch.

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

Re: Undo mode

[Lars Ingebrigtsen]

Po Lu <luangruo@yahoo.com> writes:

> IMO, consistency doesn't matter at all when the ship has already sailed.
> `undelete-frame-mode' has been here for nearly a week, so removing it
> should be out of the question.

It's not -- we do development on the master branch, and we add and
remove things at will.  And certainly if it's been on the branch for
such a short period as a week.  `undelete-frame-mode' makes no sense to
me.

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

Re: Undo mode

[Po Lu]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> It's not -- we do development on the master branch, and we add and
> remove things at will.  And certainly if it's been on the branch for
> such a short period as a week.

Duly noted, but I think the period between when releases are made is
still long enough to warrant some guarantee of stability.  Maybe a month
or so would make more sense?

> `undelete-frame-mode' makes no sense to me.

I thought there was a discussion around its implementation that led to
the creation of `undelete-frame-mode'.

I did not follow it, so I don't really have an opinion as to whether or
not it's a good idea, but it seems that a decision was made there with
input from many people that we shouldn't reject only a week later as a
bad idea.

Thanks.

Re: Undo mode

[Lars Ingebrigtsen]

Po Lu <luangruo@yahoo.com> writes:

> Duly noted, but I think the period between when releases are made is
> still long enough to warrant some guarantee of stability.  Maybe a month
> or so would make more sense?

I don't think we need to have any specified rules in this area.  Of
course we don't want to be needlessly disruptive for the people that
follow the master branch, so nothing is changed wilfully, but we're not
ruling out changing anything on that branch -- it's up to the maintainers.

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

Re: Undo mode

[Po Lu]

Gregory Heytings <gregory@heytings.org> writes:

> The original design added a single "Undelete Frame" item in the File
> menu, under the "Delete Frame" item.  The conclusion of the discussion
> in bug#51883 was that this feature shouldn't be enabled by default,
> which means that this "Undelete Frame" menu item would have remained
> inactive, unlike other items in the menus whose state change from
> inactive to active when their actions become feasible.
>
> Users could have reasonably expected to see this menu item become
> active after they used the "Delete Frame" menu item, just like the
> "Remove Other Windows" item becomes active after using one of the "New
> Window" items.
>
> Therefore, to help users discover that this menu item required an
> explicit activation in their init files to become usable, I added a
> "Enable Frame Undeletion" item in the File menu, with a tooltip which
> said "Enable frame undeletion for this session", and which replaced
> the "Undelete Frame" item when undelete-frame-mode was not active.
> IOW, this feature always used one and only one item in the File menu:
> either "Enable Frame Undeletion", or "Undelete Frame".
>
> The hard-coded limit of 16 deleted frames was also a conscious design
> choice, to avoid adding yet another defcustom that nobody had asked
> (yet). A user might want to undelete a few frames, so recording a
> single deleted frame is not enough, and 16 seemed like a reasonable
> upper limit that should cover all needs.
>
> I respectfully suggest to go back to that original design.

We haven't yet decided to change the design (aside from me turning the
"Enable Frame Undeletion" menu item into a toggle, so someone can figure
out how to turn it off should he want to.)

At least, I think we haven't.

Re: Undo mode

[Gregory Heytings]

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


>> I respectfully suggest to go back to that original design.
>
> We haven't yet decided to change the design
>

Going back to the original design is not changing the design, it's 
restoring the menu item as it was originally meant to be.  See attached 
patch.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Fix-Undelete-Frame-menu-item.patch --]
[-- Type: text/x-diff; name=Fix-Undelete-Frame-menu-item.patch, Size: 1732 bytes --]

From 9b025659fdbca5a0fe9ae38a05dbc13ec05a1c8a Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Fri, 21 Jan 2022 09:47:02 +0000
Subject: [PATCH] Fix Undelete Frame menu item

* lisp/menu-bar.el (menu-bar-file-menu): Re-add the "Enable Frame Undeletion"
menu item, and restore the "Undelete Frame" menu item.
---
 lisp/menu-bar.el | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/lisp/menu-bar.el b/lisp/menu-bar.el
index d1ca16dbf6..3a630912a9 100644
--- a/lisp/menu-bar.el
+++ b/lisp/menu-bar.el
@@ -108,16 +108,15 @@ menu-bar-file-menu
     (bindings--define-key menu [separator-tab]
       menu-bar-separator)
 
-    (bindings--define-key menu [undelete-frame-mode]
-      '(menu-item "Allow Undeleting Frames" undelete-frame-mode
-                  :help "Allow frames to be restored after deletion"
-                  :button (:toggle . undelete-frame-mode)))
-
     (bindings--define-key menu [undelete-last-deleted-frame]
       '(menu-item "Undelete Frame" undelete-frame
-                  :visible (and undelete-frame-mode
-                                (car undelete-frame--deleted-frames))
+                  :visible undelete-frame-mode
+                  :enable undelete-frame--deleted-frames
                   :help "Undelete the most recently deleted frame"))
+    (bindings--define-key menu [enable-undelete-frame-mode]
+      '(menu-item "Enable Frame Undeletion" undelete-frame-mode
+                  :visible (null undelete-frame-mode)
+                  :help "Enable frame undeletion for this session"))
 
     ;; Don't use delete-frame as event name because that is a special
     ;; event.
-- 
2.34.1

Re: Undo mode

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> We are not going to enable undelete-frame-mode by default.

I don't think I understand the benefit of hiding this behind a mode.
It seems to me that the major difference it will make is that the
`undelete-frame' command won't work by default.

Re: Undo mode

[Po Lu]

Gregory Heytings <gregory@heytings.org> writes:

> Going back to the original design is not changing the design, it's
> restoring the menu item as it was originally meant to be.  See
> attached patch.

Thanks, but I don't see how that's any different, aside from not telling
the user how to turn it off.

Either way, this change was discussed in bug#53382, so I suggest to look
there.

Re: Undo mode

[Gregory Heytings]

>> We are not going to enable undelete-frame-mode by default.
>
> I don't think I understand the benefit of hiding this behind a mode. It 
> seems to me that the major difference it will make is that the 
> `undelete-frame' command won't work by default.
>

See bug#51883.  The conclusion was that undeleting frames is not a feature 
that all users need, and that enabling it by default uses some memory.

Re: Undo mode

[Gregory Heytings]

>> Going back to the original design is not changing the design, it's 
>> restoring the menu item as it was originally meant to be.  See attached 
>> patch.
>
> Thanks, but I don't see how that's any different, aside from not telling 
> the user how to turn it off.
>
> Either way, this change was discussed in bug#53382, so I suggest to look 
> there.
>

Discussed?  You filed a bug report, Eli replied "I don't see that as a 
problem", you did what you wanted to do two hours later, and closed the 
bug report.

There's no point in telling the user "how to turn it off", as I said 
earlier the "Enable Frame Undeletion" menu item was there as a hint to 
tell users that it's a feature that needs to be turned on in their init 
files.

Re: Undo mode

[Po Lu]

Gregory Heytings <gregory@heytings.org> writes:

> Discussed?  You filed a bug report, Eli replied "I don't see that as a
> problem", you did what you wanted to do two hours later, and closed
> the bug report.

Eli also added an option first to disable the mode, and I turned it into
a toggle.  And yes, that counts as a discussion, since it involved more
than one person.

> There's no point in telling the user "how to turn it off", as I said
> earlier the "Enable Frame Undeletion" menu item was there as a hint to
> tell users that it's a feature that needs to be turned on in their
> init files.

Let's say a hypothetical user with limited memory turns frame undeletion
on, realizes that it uses too much memory to be useful to him, and then
wants to turn it off.

By the time he realizes he wants it off, the output of view-lossage is
no longer likely to be helpful to determine exactly how the feature was
enabled, so naturally he will look in the place where he turned it on,
which is the File menu, and get very confused if all that is there is
"Undelete Frame", and not "Disable Frame Undeletion" or some variant
thereof.

Re: Undo mode

[Gregory Heytings]

>> There's no point in telling the user "how to turn it off", as I said 
>> earlier the "Enable Frame Undeletion" menu item was there as a hint to 
>> tell users that it's a feature that needs to be turned on in their init 
>> files.
>
> Let's say a hypothetical user with limited memory turns frame undeletion 
> on, realizes that it uses too much memory to be useful to him, and then 
> wants to turn it off.
>

I suggest to wait before such an (unlikely) hypothetical situation happens 
before concluding that something which disfigures the File menu must be 
done.

And your hypothetical user can always do M-x undelete-frame TAB to see 
that there is an undelete-frame-mode.

Re: Undo mode

[Po Lu]

Gregory Heytings <gregory@heytings.org> writes:

> I suggest to wait before such an (unlikely) hypothetical situation
> happens before concluding that something which disfigures the File
> menu must be done.

While the scenario I presented is hypothetical, the problem itself is
not.  I spent a minute searching for a way to turn frame undeletion off
after enabling it, to debug a input focus related problem on X.

It turned out to be a bug in either GNOME Shell or XWayland, but at
first it only manifested after I turned on frame undeletion.

> And your hypothetical user can always do M-x undelete-frame TAB to see
> that there is an undelete-frame-mode.

What if he only knows to use `undelete-frame' from the menu bar?

Re: Undo mode

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Fri, 21 Jan 2022 09:56:50 +0100
> Cc: emacs-devel@gnu.org, Juri Linkov <juri@linkov.net>
> 
> `undelete-frame-mode' makes no sense to me.

It does to me, and it was discussed at length as part of bug#51883.

Re: Undo mode

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Fri, 21 Jan 2022 10:16:45 +0200
> Cc: emacs-devel@gnu.org
> 
> > Why do we need to postpone it via `after-init-hook`?
> 
> The problem is that during Emacs GUI startup, some temporary TTY frame
> is created and deleted.  Maybe this is a bug?

No, this is normal.  temacs uses a surrogate TTY frame, and that frame
is dumped when the Emacs executable is built.  It is also used by the
daemon.

Re: Undo mode

[Eli Zaretskii]

> Date: Fri, 21 Jan 2022 08:41:42 +0000
> From: Gregory Heytings <gregory@heytings.org>
> Cc: emacs-devel@gnu.org
> 
> I respectfully suggest to go back to that original design.

Which part(s) of the original design changed in ways that you disagree
with?

Re: Undo mode

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Fri, 21 Jan 2022 02:09:41 -0800
> Cc: emacs-devel@gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > We are not going to enable undelete-frame-mode by default.
> 
> I don't think I understand the benefit of hiding this behind a mode.

That was the conclusion of our discussion of the original issue.
Please re-read it if you want to understand the reasons.

> It seems to me that the major difference it will make is that the
> `undelete-frame' command won't work by default.

That's right, we concluded that it would be best to have that disabled
by default.

Re: Undo mode

[Eli Zaretskii]

> Date: Fri, 21 Jan 2022 09:57:18 +0000
> From: Gregory Heytings <gregory@heytings.org>
> Cc: emacs-devel@gnu.org, Juri Linkov <juri@linkov.net>
> 
> Going back to the original design is not changing the design, it's 
> restoring the menu item as it was originally meant to be.  See attached 
> patch.

Why is it a problem to allow disabling undeletion of frames once that
was enabled?


Or if that is not the problem you are trying to fix, what is that
problem?

Re: Undo mode

[Eli Zaretskii]

> Date: Fri, 21 Jan 2022 10:26:54 +0000
> From: Gregory Heytings <gregory@heytings.org>
> Cc: emacs-devel@gnu.org, Juri Linkov <juri@linkov.net>
> 
> There's no point in telling the user "how to turn it off", as I said 
> earlier the "Enable Frame Undeletion" menu item was there as a hint to 
> tell users that it's a feature that needs to be turned on in their init 
> files.

It is unusual to have a setting that can be turned on via the menu,
but not turned off via the same menu.  The mode can be turned off via
M-x, so what would be our reason not to allow the same from the menu
bar?

Re: [External] : Re: Undo mode

[Stefan Monnier]

> But if something isn't in an Emacs release (the
> latest is 27.2) then I'd say it hasn't sailed.

100% agreed.

> What if someone tracking master (and there are enough of them to make a
> difference, I think) has customized `undelete-frame-mode' to t?

What about that?


        Stefan

Re: Undo mode

[Stefan Monnier]

Lars Ingebrigtsen [2022-01-21 10:17:45] wrote:
> Po Lu <luangruo@yahoo.com> writes:
>> Duly noted, but I think the period between when releases are made is
>> still long enough to warrant some guarantee of stability.  Maybe a month
>> or so would make more sense?
> I don't think we need to have any specified rules in this area.

Especially since changes cost efforts, so there's a natural resistance
to change, without needing any extra rules to enforce it.


        Stefan

Re: Undo mode

[Yuri Khan]

On Fri, 21 Jan 2022 at 16:38, Gregory Heytings <gregory@heytings.org> wrote:

> Therefore, to help users discover that this menu item required an explicit
> activation in their init files to become usable, I added a "Enable Frame
> Undeletion" item in the File menu, with a tooltip which said "Enable frame
> undeletion for this session"

Why would it be on the File menu rather than Options?

Re: Undo mode

[Gregory Heytings]

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


>> Going back to the original design is not changing the design, it's 
>> restoring the menu item as it was originally meant to be.  See attached 
>> patch.
>
> Why is it a problem to allow disabling undeletion of frames once that 
> was enabled?
>

It's not a problem at all, but it's not what the "Enable Frame Undeletion" 
menu item was meant to do.  It was meant to tell users that it is 
necessary to add somethig their init files to enable that feature.

I attach three possible solutions, in order of personal preference:

1. The original design: the "Enable Frame Undeletion" item disappears once 
it has been used, and is replaced by the "Undelete Frame" item.  Users 
have to use M-x undelete-frame-mode to turn that feature off, and it is 
turned on only during the current session.

2. The only menu item is "Undelete Frame", but it has a "Undelete-Frame 
mode is disabled" tooltip when undelete-frame-mode is off, and a "Undelete 
the most recently deleted frame" tooltip when it is on.

3. The "Enable Frame Undeletion" item disappears, and a "Disable Frame 
Undeletion" item is added after the "Undelete Frame" item.  It uses one or 
two menu items, but a least it doesn't shift the whole File menu to the 
right because of a checkbox.

[-- Attachment #2: Fix-Undelete-Frame-menu-item-Option-1.patch --]
[-- Type: text/x-diff, Size: 1743 bytes --]

From ac56be4475b773e08f33f9d1e91eb8191865ebf8 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Fri, 21 Jan 2022 13:08:50 +0000
Subject: [PATCH] Fix Undelete Frame menu item (Option 1)

* lisp/menu-bar.el (menu-bar-file-menu): Re-add the "Enable Frame Undeletion"
menu item, and restore the "Undelete Frame" menu item.
---
 lisp/menu-bar.el | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/lisp/menu-bar.el b/lisp/menu-bar.el
index d1ca16dbf6..3a630912a9 100644
--- a/lisp/menu-bar.el
+++ b/lisp/menu-bar.el
@@ -108,16 +108,15 @@ menu-bar-file-menu
     (bindings--define-key menu [separator-tab]
       menu-bar-separator)
 
-    (bindings--define-key menu [undelete-frame-mode]
-      '(menu-item "Allow Undeleting Frames" undelete-frame-mode
-                  :help "Allow frames to be restored after deletion"
-                  :button (:toggle . undelete-frame-mode)))
-
     (bindings--define-key menu [undelete-last-deleted-frame]
       '(menu-item "Undelete Frame" undelete-frame
-                  :visible (and undelete-frame-mode
-                                (car undelete-frame--deleted-frames))
+                  :visible undelete-frame-mode
+                  :enable undelete-frame--deleted-frames
                   :help "Undelete the most recently deleted frame"))
+    (bindings--define-key menu [enable-undelete-frame-mode]
+      '(menu-item "Enable Frame Undeletion" undelete-frame-mode
+                  :visible (null undelete-frame-mode)
+                  :help "Enable frame undeletion for this session"))
 
     ;; Don't use delete-frame as event name because that is a special
     ;; event.
-- 
2.34.1


[-- Attachment #3: Fix-Undelete-Frame-menu-item-Option-2.patch --]
[-- Type: text/x-diff, Size: 1751 bytes --]

From 298fb61779e652016dda3fc4b4af2c0a23c0f5cc Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Fri, 21 Jan 2022 13:16:04 +0000
Subject: [PATCH] Fix Undelete Frame menu item (Option 2)

* lisp/menu-bar.el (menu-bar-file-menu): Restore the "Undelete Frame"
menu item, and add a disabled item with a tooltip explaining how to
enable it.
---
 lisp/menu-bar.el | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/lisp/menu-bar.el b/lisp/menu-bar.el
index d1ca16dbf6..d2ed6dc3b7 100644
--- a/lisp/menu-bar.el
+++ b/lisp/menu-bar.el
@@ -108,15 +108,15 @@ menu-bar-file-menu
     (bindings--define-key menu [separator-tab]
       menu-bar-separator)
 
-    (bindings--define-key menu [undelete-frame-mode]
-      '(menu-item "Allow Undeleting Frames" undelete-frame-mode
-                  :help "Allow frames to be restored after deletion"
-                  :button (:toggle . undelete-frame-mode)))
-
+    (bindings--define-key menu [disabled-undelete-frame]
+      '(menu-item "Undelete Frame" undelete-frame
+                  :visible (null undelete-frame-mode)
+                  :enable nil
+                  :help "Undelete-Frame mode is disabled"))
     (bindings--define-key menu [undelete-last-deleted-frame]
       '(menu-item "Undelete Frame" undelete-frame
-                  :visible (and undelete-frame-mode
-                                (car undelete-frame--deleted-frames))
+                  :visible undelete-frame-mode
+                  :enable undelete-frame--deleted-frames
                   :help "Undelete the most recently deleted frame"))
 
     ;; Don't use delete-frame as event name because that is a special
-- 
2.34.1


[-- Attachment #4: Fix-Undelete-Frame-menu-item-Option-3.patch --]
[-- Type: text/x-diff, Size: 2044 bytes --]

From 6b5f070ac30cef7c61418a399023d2258e642930 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Fri, 21 Jan 2022 13:20:16 +0000
Subject: [PATCH] Fix Undelete Frame menu item (Option 3)

* lisp/menu-bar.el (menu-bar-file-menu): Re-add the "Enable Frame Undeletion"
menu item, restore the "Undelete Frame" menu item, and add a "Disable Frame
Undeletion" menu item.
---
 lisp/menu-bar.el | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/lisp/menu-bar.el b/lisp/menu-bar.el
index d1ca16dbf6..a7b6dfc995 100644
--- a/lisp/menu-bar.el
+++ b/lisp/menu-bar.el
@@ -108,16 +108,19 @@ menu-bar-file-menu
     (bindings--define-key menu [separator-tab]
       menu-bar-separator)
 
-    (bindings--define-key menu [undelete-frame-mode]
-      '(menu-item "Allow Undeleting Frames" undelete-frame-mode
-                  :help "Allow frames to be restored after deletion"
-                  :button (:toggle . undelete-frame-mode)))
-
+    (bindings--define-key menu [disable-undelete-frame-mode]
+      '(menu-item "Disable Frame Undeletion" undelete-frame-mode
+                  :visible undelete-frame-mode
+                  :help "Disable frame undeletion for this session"))
     (bindings--define-key menu [undelete-last-deleted-frame]
       '(menu-item "Undelete Frame" undelete-frame
-                  :visible (and undelete-frame-mode
-                                (car undelete-frame--deleted-frames))
+                  :visible undelete-frame-mode
+                  :enable undelete-frame--deleted-frames
                   :help "Undelete the most recently deleted frame"))
+    (bindings--define-key menu [enable-undelete-frame-mode]
+      '(menu-item "Enable Frame Undeletion" undelete-frame-mode
+                  :visible (null undelete-frame-mode)
+                  :help "Enable frame undeletion for this session"))
 
     ;; Don't use delete-frame as event name because that is a special
     ;; event.
-- 
2.34.1

Re: Undo mode

[Stefan Monnier]

>>> +(add-hook 'after-init-hook
>>> +          (lambda ()
>>> +            (add-hook 'delete-frame-functions
>>> +                      #'undelete-frame--handle-delete-frame -75)))
>> Why do we need to postpone it via `after-init-hook`?
> The problem is that during Emacs GUI startup, some temporary TTY frame
> is created and deleted.  Maybe this is a bug?

I think you're talking about the initial frame (which is actually not
a TTY frame either, it's the pseudo frame used for example during batch
processing, which sends messages to stdout/stderr and reads "minibuffer"
input from stdin).

It's kind of a pain to recognize in ELisp.  In C it's recognized by:

    #define FRAME_INITIAL_P(f) ((f)->output_method == output_initial)

But in ELisp, the only simple&convenient way I know is
(eq frame frame-initial-frame), but I'm not sure if that var is still
non-nil when that code is run.

I think it's better to add a specific workaround in
`undelete-frame--handle-delete-frame` such as:

    (unless (eq frame frame-initial-frame) ...)

And if that specific one doesn't work, we'll come up with
a different test.


        Stefan

Re: Undo mode

[Eli Zaretskii]

> From: Yuri Khan <yuri.v.khan@gmail.com>
> Date: Fri, 21 Jan 2022 20:18:03 +0700
> Cc: Emacs developers <emacs-devel@gnu.org>, Juri Linkov <juri@linkov.net>
> 
> On Fri, 21 Jan 2022 at 16:38, Gregory Heytings <gregory@heytings.org> wrote:
> 
> > Therefore, to help users discover that this menu item required an explicit
> > activation in their init files to become usable, I added a "Enable Frame
> > Undeletion" item in the File menu, with a tooltip which said "Enable frame
> > undeletion for this session"
> 
> Why would it be on the File menu rather than Options?

Because that's where Delete Frame is.

Re: Undo mode

[Eli Zaretskii]

> Date: Fri, 21 Jan 2022 13:50:03 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: luangruo@yahoo.com, juri@linkov.net, emacs-devel@gnu.org
> 
> > Why is it a problem to allow disabling undeletion of frames once that 
> > was enabled?
> 
> It's not a problem at all, but it's not what the "Enable Frame Undeletion" 
> menu item was meant to do.  It was meant to tell users that it is 
> necessary to add somethig their init files to enable that feature.

So now we have that indication, plus the ability of turning the mdoe
back off.  Why is that a problem?  Your original feature is still
there (albeit under a slightly different name), and there's an
_additional_ feature as well.  How is that a problem?

> I attach three possible solutions, in order of personal preference:

I'm sorry, I'm unable to discuss solutions to a problem I don't
understand.

Once again: what is the problem that needs to be fixed here, and why?
That Emacs behaves slightly differently from your original code is not
a problem in itself.

Re: Undo mode

[Gregory Heytings]

>> I attach three possible solutions, in order of personal preference:
>
> I'm sorry, I'm unable to discuss solutions to a problem I don't 
> understand.
>
> Once again: what is the problem that needs to be fixed here, and why? 
> That Emacs behaves slightly differently from your original code is not a 
> problem in itself.
>

The three problems of the current state are:

1. All File menu items are shifted to the right because of a single 
checkbox.

2. The "Undelete Frame" item appears and disappears, instead of being 
disabled and enabled.

3. The frame undeletion items uses two entries in the File menu, IMO 
that's one too much, and with the proposed options 1 and 2 it uses only 
one.

Re: Undo mode

[Gregory Heytings]

>
> I think it's better to add a specific workaround in 
> `undelete-frame--handle-delete-frame` such as:
>
>    (unless (eq frame frame-initial-frame) ...)
>
> And if that specific one doesn't work, we'll come up with a different 
> test.
>

That problem was already identified and solved (with a boolean state 
variable) in bug#51883, before the current solution (using a minor mode 
that users can turn on in their init file) was adopted.  With a minor mode 
this test becomes unnecessary.

Re: Undo mode

[Yuri Khan]

On Fri, 21 Jan 2022 at 21:29, Eli Zaretskii <eliz@gnu.org> wrote:

> > Why would it be on the File menu rather than Options?
>
> Because that's where Delete Frame is.

That does not explain much. ‘Cut’, ‘Paste’ and ‘Search’ are on Edit
but ‘Use CUA Keys’ and ‘Default Search Options’ are on Options. Buffer
names are on Buffers, but ‘Use Directory Names in Buffer Names’ is on
Options.

RE: [External] : Re: Undo mode

[Drew Adams]

> See bug#51883.  The conclusion was that undeleting frames is not a feature
> that all users need, and that enabling it by default uses some memory.

I haven't read that bug thread, but a
quick look at its status suggests that
there hasn't yet been _any_ conclusion.

Is this incorrect?

RE: [External] : Re: Undo mode

[Drew Adams]

You quoted two different participants, using
the same quote level.  Please don't do that -
it can confuse or mislead.  (Thx.)

Here's the text you quoted, but clarified to
indicate who said what.

>da> But if something isn't in an Emacs
>da> release (the latest is 27.2) then
>da> I'd say it hasn't sailed.
> 
> 100% agreed.
> 
>pl> What if someone tracking master (and
>pl> there are enough of them to make a
>pl> difference, I think) has customized
>pl> `undelete-frame-mode' to t?
> 
> What about that?

RE: [External] : Re: Undo mode

[Drew Adams]

> > Why would it be on the File menu rather than Options?
> 
> Because that's where Delete Frame is.

FWIW, a `Frames' menu or submenu might
be useful, for grouping frame commands.
___

My library menu-bar+.el adds a top-level
`Frames' menu, if library frame-cmds.el
or fit-frame.el has been loaded.

Menu `Frames' has these menu items:

 _________
 Hide Frames / Show Buffers        C-M-S-z
 Iconify All Frames
 _________
 Maximize Frame
 Maximize Frame Horizontally
 Maximize Frame Vertically
 Toggle Max Frame
 Toggle Max Frame Horizontally
 Toggle Max Frame Vertically
 _________
 Tile Frames Horizontally...
 Tile Frames Vertically...
 _________
 Set Frame Parameter from Frame...
 Set All Frame Parameters from Frame...
 Fit This Frame                    C-x C-_
 Delete Frame                      C-x 5 0
 New Frame                         C-x 5 2
 New Frame on Display...
 _________

Vanilla Emacs has its own set of commands
that could be appropriate for a `Frames'
(sub)menu.

Re: Undo mode

[Eli Zaretskii]

> From: Yuri Khan <yuri.v.khan@gmail.com>
> Date: Fri, 21 Jan 2022 22:07:44 +0700
> Cc: Gregory Heytings <gregory@heytings.org>, Emacs developers <emacs-devel@gnu.org>, 
> 	Juri Linkov <juri@linkov.net>
> 
> On Fri, 21 Jan 2022 at 21:29, Eli Zaretskii <eliz@gnu.org> wrote:
> 
> > > Why would it be on the File menu rather than Options?
> >
> > Because that's where Delete Frame is.
> 
> That does not explain much. ‘Cut’, ‘Paste’ and ‘Search’ are on Edit
> but ‘Use CUA Keys’ and ‘Default Search Options’ are on Options. Buffer
> names are on Buffers, but ‘Use Directory Names in Buffer Names’ is on
> Options.

The options in your examples are much more distant from the
corresponding commands, so I see no analogy and no need to do
something similar in this case.

Re: Undo mode

[Eli Zaretskii]

> Date: Fri, 21 Jan 2022 14:48:44 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: luangruo@yahoo.com, juri@linkov.net, emacs-devel@gnu.org
> 
> The three problems of the current state are:
> 
> 1. All File menu items are shifted to the right because of a single 
> checkbox.

Doesn't sound too bad to me.  We have the same in Text menu, for
example.

> 2. The "Undelete Frame" item appears and disappears, instead of being 
> disabled and enabled.

That's easy to change, but why is it better to show it disabled
instead of not showing it at all?

> 3. The frame undeletion items uses two entries in the File menu, IMO 
> that's one too much, and with the proposed options 1 and 2 it uses only 
> one.

That's not a separate issue, so I don't think it counts.

Re: Undo mode

[Gregory Heytings]

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


>> 1. All File menu items are shifted to the right because of a single 
>> checkbox.
>
> Doesn't sound too bad to me.
>

It depends on the toolkit you use I guess.  With Lucid the effect is 
awful, see attached screenshots.

>
> We have the same in Text menu, for example.
>

Perhaps we're looking at different Text menus, but I see five entries in 
that menu, two of which have a checkbox.

>> 2. The "Undelete Frame" item appears and disappears, instead of being 
>> disabled and enabled.
>
> That's easy to change, but why is it better to show it disabled instead 
> of not showing it at all?
>

It's what happens elsewhere in that menu and in others, some items are 
disabled or enabled depending on whether their corresponding action can be 
performed, but none disappear when their corresponding action cannot be 
performed, at least not in the standard menus.

>> 3. The frame undeletion items uses two entries in the File menu, IMO 
>> that's one too much, and with the proposed options 1 and 2 it uses only 
>> one.
>
> That's not a separate issue, so I don't think it counts.
>

What do you mean?  Problem 1 above is the appearance of the checkbox, 
problem 2 about the fact that a menu item appears and disappears, problem 
3 about the number of menu entries.  These are three separate things.

My conclusion so far is that the most reasonable compromise would be to 
use the second patch (attached again).

[-- Attachment #2: with-checkbox.png --]
[-- Type: image/png, Size: 33372 bytes --]

[-- Attachment #3: without-checkbox.png --]
[-- Type: image/png, Size: 31754 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: Fix-Undelete-Frame-menu-item-Option-2.patch --]
[-- Type: text/x-diff; name=Fix-Undelete-Frame-menu-item-Option-2.patch, Size: 1751 bytes --]

From 298fb61779e652016dda3fc4b4af2c0a23c0f5cc Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Fri, 21 Jan 2022 13:16:04 +0000
Subject: [PATCH] Fix Undelete Frame menu item (Option 2)

* lisp/menu-bar.el (menu-bar-file-menu): Restore the "Undelete Frame"
menu item, and add a disabled item with a tooltip explaining how to
enable it.
---
 lisp/menu-bar.el | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/lisp/menu-bar.el b/lisp/menu-bar.el
index d1ca16dbf6..d2ed6dc3b7 100644
--- a/lisp/menu-bar.el
+++ b/lisp/menu-bar.el
@@ -108,15 +108,15 @@ menu-bar-file-menu
     (bindings--define-key menu [separator-tab]
       menu-bar-separator)
 
-    (bindings--define-key menu [undelete-frame-mode]
-      '(menu-item "Allow Undeleting Frames" undelete-frame-mode
-                  :help "Allow frames to be restored after deletion"
-                  :button (:toggle . undelete-frame-mode)))
-
+    (bindings--define-key menu [disabled-undelete-frame]
+      '(menu-item "Undelete Frame" undelete-frame
+                  :visible (null undelete-frame-mode)
+                  :enable nil
+                  :help "Undelete-Frame mode is disabled"))
     (bindings--define-key menu [undelete-last-deleted-frame]
       '(menu-item "Undelete Frame" undelete-frame
-                  :visible (and undelete-frame-mode
-                                (car undelete-frame--deleted-frames))
+                  :visible undelete-frame-mode
+                  :enable undelete-frame--deleted-frames
                   :help "Undelete the most recently deleted frame"))
 
     ;; Don't use delete-frame as event name because that is a special
-- 
2.34.1

Re: Undo mode

[Po Lu]

Gregory Heytings <gregory@heytings.org> writes:

> It depends on the toolkit you use I guess.  With Lucid the effect is
> awful, see attached screenshots.

We should fix the toolkit, then.  And perhaps provide an option for the
user to customize the behaviour.

Re: Undo mode

[Eli Zaretskii]

> Date: Fri, 21 Jan 2022 20:58:54 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: luangruo@yahoo.com, juri@linkov.net, emacs-devel@gnu.org
> 
> >> 1. All File menu items are shifted to the right because of a single 
> >> checkbox.
> >
> > Doesn't sound too bad to me.
> 
> It depends on the toolkit you use I guess.  With Lucid the effect is 
> awful, see attached screenshots.
> 
> > We have the same in Text menu, for example.
> 
> Perhaps we're looking at different Text menus, but I see five entries in 
> that menu, two of which have a checkbox.

Yes, and how is this different from File?  Some items have the
checkbox, others don't, and all of them are shifted to the right as
result.

> >> 2. The "Undelete Frame" item appears and disappears, instead of being 
> >> disabled and enabled.
> >
> > That's easy to change, but why is it better to show it disabled instead 
> > of not showing it at all?
> 
> It's what happens elsewhere in that menu and in others, some items are 
> disabled or enabled depending on whether their corresponding action can be 
> performed, but none disappear when their corresponding action cannot be 
> performed, at least not in the standard menus.

That's factually untrue: there are several menu items that disappear
when they are not relevant.  Examples: "New Frame on Monitor...",
"Close Tab", and a few others.  But okay, I've now changed this
particular entry to be always present.

> >> 3. The frame undeletion items uses two entries in the File menu, IMO 
> >> that's one too much, and with the proposed options 1 and 2 it uses only 
> >> one.
> >
> > That's not a separate issue, so I don't think it counts.
> 
> What do you mean?

I mean if we add an item, there's one more item as a consequence.  So
the number of items is not a separate issue.

> My conclusion so far is that the most reasonable compromise would be to 
> use the second patch (attached again).

Given that I made the change above, I see no reason to change anything
else at this time.

Re: Undo mode

[Gregory Heytings]

>>>> The frame undeletion items uses two entries in the File menu, IMO 
>>>> that's one too much, and with the proposed options 1 and 2 it uses 
>>>> only one.
>>>
>>> That's not a separate issue, so I don't think it counts.
>>
>> What do you mean?
>
> I mean if we add an item, there's one more item as a consequence.  So 
> the number of items is not a separate issue.
>

The point is that there should be no more than one menu item for that 
feature.  There are two, that's one too much.  There is no reason to turn 
that feature, which has no practical effect except that it uses a few more 
bytes in the heap, on and off from the menu.

Re: Undo mode

[Eli Zaretskii]

> Date: Sat, 22 Jan 2022 08:32:44 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: luangruo@yahoo.com, emacs-devel@gnu.org, juri@linkov.net
> 
> The point is that there should be no more than one menu item for that 
> feature.  There are two, that's one too much.  There is no reason to turn 
> that feature, which has no practical effect except that it uses a few more 
> bytes in the heap, on and off from the menu.

I already explained why it would be very unusual to have a feature
that can only be turned on from the menu, but cannot be turned off.  I
see no reason for such strange behavior for this single mode.

Re: Undo mode

[Gregory Heytings]

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


>> The point is that there should be no more than one menu item for that 
>> feature.  There are two, that's one too much.  There is no reason to 
>> turn that feature, which has no practical effect except that it uses a 
>> few more bytes in the heap, on and off from the menu.
>
> I already explained why it would be very unusual to have a feature that 
> can only be turned on from the menu, but cannot be turned off.  I see no 
> reason for such strange behavior for this single mode.
>

Which is why I suggested to remove the possibility of enabling it from the 
menu.  See the attached patch (again).

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Fix-Undelete-Frame-menu-item.patch --]
[-- Type: text/x-diff; name=Fix-Undelete-Frame-menu-item.patch, Size: 1739 bytes --]

From af5dea30cc40ca7b8aabc58898b1e2bd755beae6 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Sat, 22 Jan 2022 08:21:41 +0000
Subject: [PATCH] Fix Undelete Frame menu item

* lisp/menu-bar.el (menu-bar-file-menu): Replace the checkbox with a
a disabled item with a tooltip telling that the mode is disabled.
---
 lisp/menu-bar.el | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/lisp/menu-bar.el b/lisp/menu-bar.el
index 817c2d485e..8d4ac1692f 100644
--- a/lisp/menu-bar.el
+++ b/lisp/menu-bar.el
@@ -108,15 +108,15 @@ menu-bar-file-menu
     (bindings--define-key menu [separator-tab]
       menu-bar-separator)
 
-    (bindings--define-key menu [undelete-frame-mode]
-      '(menu-item "Allow Undeleting Frames" undelete-frame-mode
-                  :help "Allow frames to be restored after deletion"
-                  :button (:toggle . undelete-frame-mode)))
-
+    (bindings--define-key menu [disabled-undelete-frame-mode]
+      '(menu-item "Undelete Frame" undelete-frame
+                  :visible (null undelete-frame-mode)
+                  :enable nil
+                  :help "Undelete-Frame mode is currently disabled"))
     (bindings--define-key menu [undelete-last-deleted-frame]
       '(menu-item "Undelete Frame" undelete-frame
-                  :enable (and undelete-frame-mode
-                                (car undelete-frame--deleted-frames))
+                  :visible undelete-frame-mode
+                  :enable undelete-frame--deleted-frames
                   :help "Undelete the most recently deleted frame"))
 
     ;; Don't use delete-frame as event name because that is a special
-- 
2.34.1

Re: Undo mode

[Po Lu]

Gregory Heytings <gregory@heytings.org> writes:

> Which is why I suggested to remove the possibility of enabling it from
> the menu.  See the attached patch (again).

Why are you opposed to there being two menu items for such a feature?

I haven't heard a single argument to that end.  All I hear is some
variant of "it defaces the files menu", which is just another way of
repeating that you don't want those items to be there.

I also count 3 items in the Files menu related to multi-tty, which is
objectively less likely to be used than frame undeletion, and probably
less important to tell new users about as well.

Thanks.

Re: Undo mode

[Stefan Kangas]

Gregory Heytings <gregory@heytings.org> writes:

> Which is why I suggested to remove the possibility of enabling it from the
> menu.  See the attached patch (again).

If it is already relegated to second-class citizenship and not a default
feature, I don't understand why it belongs in the default menu at all.

The menu entry could be made visible only if the minor mode is enabled.

Re: Undo mode

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> I don't think I understand the benefit of hiding this behind a mode.
>
> That was the conclusion of our discussion of the original issue.
> Please re-read it if you want to understand the reasons.

I did of course, but I still fail to see it.  Is it about the memory
consumption?  If so, did anyone measure its impact?  I'd have thought
that the memory impact was insignificant.

Or is there some other important principle at stake here?  Feel free to
elaborate.

Re-reading that discussion (now twice) just takes me back to the
conclusion that this makes Emacs harder to use, with no clear benefit.

Perhaps we should have a `low-memory-mode' to turn down _several_
defaults and disable some "memory intensive" features.  That might have
a measurable impact (we would need to check that), and would make much
more sense to me than disabling this undo command for that reason.

Re: Undo mode

[Po Lu]

Stefan Kangas <stefankangas@gmail.com> writes:

> If it is already relegated to second-class citizenship and not a default
> feature, I don't understand why it belongs in the default menu at all.

Not being enabled by default doesn't give a feature "second-class
citizenship", I should think.

Besides, we have lots of such features in the default menu bar: the
Options and Tools menus are full of them.

Thanks.

Re: Undo mode

[Eli Zaretskii]

> Date: Sat, 22 Jan 2022 09:08:46 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: luangruo@yahoo.com, emacs-devel@gnu.org, juri@linkov.net
> 
> > I already explained why it would be very unusual to have a feature that 
> > can only be turned on from the menu, but cannot be turned off.  I see no 
> > reason for such strange behavior for this single mode.
> >
> 
> Which is why I suggested to remove the possibility of enabling it from the 
> menu.  See the attached patch (again).

Sorry, I don't agree with such a change, as it would be a step back.

Re: Undo mode

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Sat, 22 Jan 2022 09:51:39 +0000
> Cc: juri@linkov.net, emacs-devel@gnu.org
> 
> > That was the conclusion of our discussion of the original issue.
> > Please re-read it if you want to understand the reasons.
> 
> I did of course, but I still fail to see it.  Is it about the memory
> consumption?  If so, did anyone measure its impact?  I'd have thought
> that the memory impact was insignificant.

Memory consumption, and also turning on by default a new mode that
wasn't there before.

> Re-reading that discussion (now twice) just takes me back to the
> conclusion that this makes Emacs harder to use, with no clear benefit.

How can it make Emacs harder to use when by default absolutely nothing
has changed??  Please make your claims at least remotely aligned with
the facts.

> Perhaps we should have a `low-memory-mode' to turn down _several_
> defaults and disable some "memory intensive" features.  That might have
> a measurable impact (we would need to check that), and would make much
> more sense to me than disabling this undo command for that reason.

Bikeshedding go!

Re: Undo mode

[Gregory Heytings]

>>> I already explained why it would be very unusual to have a feature 
>>> that can only be turned on from the menu, but cannot be turned off. 
>>> I see no reason for such strange behavior for this single mode.
>>
>> Which is why I suggested to remove the possibility of enabling it from 
>> the menu.  See the attached patch (again).
>
> Sorry, I don't agree with such a change, as it would be a step back.
>

I give up.

I'm sorry to have contributed to that mess.

Re: Undo mode

[Eli Zaretskii]

> Date: Sat, 22 Jan 2022 11:15:36 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: luangruo@yahoo.com, emacs-devel@gnu.org, juri@linkov.net
> 
> > Sorry, I don't agree with such a change, as it would be a step back.
> >
> 
> I give up.
> 
> I'm sorry to have contributed to that mess.

You have absolutely nothing to be sorry about.  Your original code is
present with very minor changes, and the feature works as you designed
it.  Thanks for contributing it to Emacs.

Re: Undo mode

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> I did of course, but I still fail to see it.  Is it about the memory
>> consumption?  If so, did anyone measure its impact?  I'd have thought
>> that the memory impact was insignificant.
>
> Memory consumption,

Feel free to provide details.

>  and also turning on by default a new mode that
> wasn't there before.

We don't need any new mode, see Juri's patch.

>> Re-reading that discussion (now twice) just takes me back to the
>> conclusion that this makes Emacs harder to use, with no clear benefit.
>
> How can it make Emacs harder to use

It is harder to use this feature, because you first have to enable a new
mode, or the command will just error out.

There is also the mode itself, and the additional menu entry/entries.

The question is: what do we win from this additional complexity?
I didn't see any satisfactory answer to that question.

Re: Undo mode

[Gregory Heytings]

>
> Your original code is present with very minor changes, and the feature 
> works as you designed it.
>

A feature with a poor user interface is a misfeature.  That double menu 
entry and that checkbox is like a wart on the nose; I cannot consider such 
a change to the user interface as a "very minor change".

You may remind that I turned that feature into a minor mode upon your 
request, and that I turned it into an opt-in minor mode upon your request. 
My only point is this thread was to at least keep the visible user 
interface clean.

Re: Undo mode

[Juri Linkov]

> The hard-coded limit of 16 deleted frames was also a conscious design
> choice, to avoid adding yet another defcustom that nobody had asked
> (yet).

A customizable option will be needed anyway, and it has such nice side effect,
that its value 0 obviates the need for the special mode, because 0 effectively
disables this feature.  So if the memory consumption is a real problem, its
default value could be 0.