Package: emacs;
Reported by: Yuan Fu <casouri <at> gmail.com>
Date: Sat, 18 Jan 2020 20:58:02 UTC
Severity: normal
Tags: patch
Found in version 27.0.50
Done: Yuan Fu <casouri <at> gmail.com>
Bug is archived. No further changes may be made.
View this message in rfc822 format
From: Štěpán Němec <stepnem <at> gmail.com> To: Yuan Fu <casouri <at> gmail.com>, martin rudalics <rudalics <at> gmx.at> Cc: 39181 <at> debbugs.gnu.org Subject: bug#39181: 27.0.50; [PATCH] Allow users to store & restore gdb-mi layout Date: Mon, 09 Mar 2020 20:18:44 +0100
Again, having little experience with gdb(-mi) I have mostly checked the
doc strings; I hope Martin will provide better feedback.
Other than the nit picks below, I have one question: is there any
difference between "window layout" and "window configuration" in this
context? You seem to be using both interchangeably, both in
documentation and the function/variable names. There seems to be some
prior usage of "layout" in gdb-mi, but the general Emacs term AFAIK is
"configuration". Wouldn't it make sense to unify the usage somewhat, at
least in the new code? Just an observation (I found it confusing.)
On Mon, 09 Mar 2020 13:59:31 -0400
Yuan Fu wrote:
> From c27f39a3e321a7a1111f71dd95573104f025c89c Mon Sep 17 00:00:00 2001
> From: Yuan Fu <casouri <at> gmail.com>
> Date: Tue, 3 Mar 2020 18:30:03 -0500
> Subject: [PATCH] Add window streo/restore feature for gdb-mi
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> Add a feature that allows a user to save a gdb window layout to a file
> with 'gdb-save-window-layout' and load it back it with
^^
Typo?
[...]
> +(defcustom gdb-restore-window-layout-after-quit nil
> + "Specify whether to restore the window layout the user had before gdb starts.
> +
> +Possible values are:
> + t -- Always restore.
> + nil -- Don't restore.
> + 'if-show-main -- Restore only if `gdb-show-main' is non-nil
> + 'if-many-windows -- Restore only if variable `gdb-many-windows' is non-nil."
^^^^^^^^^^^^^^^^
The documented symbols don't match the actual ones below. Also, if you
want to quote them in the doc string, the convention (which you do
follow elsewhere) is `like-this', 'this is confusing.
> + :type '(choice
> + (const :tag "Always restore" t)
> + (const :tag "Don't restore" nil)
> + (const :tag "Depends on `gdb-show-main'" 'if-gdb-show-main)
> + (const :tag "Depends on `gdb-many-windows'" 'if-gdb-many-windows))
^^^^^^^^^^^^^^^^^^^
[...]
> +(defun gdb-toggle-restore-window-layout ()
> + "Toggle whether to restore window layout when GDB quit."
^^^^
quits
> + (interactive)
> + (setq gdb-restore-window-layout-after-quit
> + (not gdb-restore-window-layout-after-quit)))
> +
> +(defun gdb-get-source-buffer ()
> + "Return a buffer displaying source file or nil if we can't find one.
> +
> +The source file is the file that contains the code at where GDB
^^^^^^^^
Just "where", or perhaps "source location where"?
> +stops. There could be multiple source files during a debugging
> +session, we get the most recently showed one. If program hasn't
> +start running yet, the source file is the \"main file\" at where
^^^^^^^^
Same as above.
> +the GDB session starts (see `gdb-main-file')."
[...]
> +(defun gdb-function-buffer-p (buffer)
> + "Return t if BUFFER is a GDB function buffer.
> +
> +Function buffers are locals buffer, registers buffer, etc, but
> +not including main command buffer (the one in where you type GDB
^^^^^^^^
Again, just "where".
> +commands) or source buffers (that displays program source code)."
^
"display"
[...]
> +(defun gdb-load-window-layout (file)
> + "Restore window layout (window configuration) from FILE.
> +
> +FILE should be a window layout file saved by
> +`gdb-save-window-layout'."
> + (interactive (list (read-file-name
> + "Restore window configuration from file: "
> + (or gdb-window-layout-directory default-directory))))
> + ;; Basically, we restore window configuration and go through each
> + ;; window and restore the function buffers.
> + (let* ((placeholder (get-buffer-create " *gdb-placeholder*")))
> + (unwind-protect ; Don't leak buffer.
> + (let ((window-config (with-temp-buffer
> + (insert-file-contents file)
> + ;; We need to go to point-min even we
> + ;; are reading the whole buffer.
I can't understand this comment. Maybe "even" should have been "so that"?
> + (goto-char (point-min))
> + (read (current-buffer))))
[...]
> @@ -4659,6 +4847,9 @@ gdb-many-windows
> (defun gdb-restore-windows ()
> "Restore the basic arrangement of windows used by gdb.
> This arrangement depends on the value of option `gdb-many-windows'."
> + ;; This function is used when the user messed up window
> + ;; configuration and want to "reset to default". The function that
^^^^
"wants"
[...]
> +(defmacro with-window-undedicated (window &rest body)
> + "Select WINDOW, set it to be undedicated and execute BODY.
> +
> +WINDOW is only set to be undedicated temporarily while executing
> +BODY. That is, the original value of WINDOW's dedication is
> +restored after executing BODY. If WINDOW is nil, use the
> +selected window. The value returned is the value of the last
> +form in BODY.
The "temporarily" thing is actually expected/understood with with-
macros, so I think it could be simplified to something like the
following (BTW, while there are occurences or "non-dedicated" in Emacs
sources, there are no occurences of "undedicated". Another thing to
maybe consider for the sake of consistency/least surprise.):
"Evaluate BODY with WINDOW selected and temporarily made non-dedicated.
If WINDOW is nil, use the selected window. Return the value of the last
form in BODY."
Thank you,
Štěpán
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.