Subject: Re: bug#39181: 27.0.50; [PATCH] Allow users to store & restore gdb-mi layout

From: Yuan Fu <[email-masked]>
Date: Fri, 13 Mar 2020 17:40:30 -0400

> 
>> The window layout that we save to a file in gdb is a window
>> configuration structure with some small differences: simply loading
>> that window configuration with `window-state-put' doesn’t give back
>> your gdb layout. (because all the buffers (local, breakpoint, etc) are
>> different from session to session.) `gdb-load-window-configuration`
>> does some trick to recreate each buffers. Apart from that the layout
>> and window configuration is the same thing.
> 
> Thank you for the clarification! It would be nice to have that in the
> documentation as well.

I added some comments in the menu-map code. Hope that’s helps.

>> @@ -253,6 +255,25 @@ gdb-output-sink
>>               disposition of output generated by commands that
>>               gdb mode sends to gdb on its own behalf.")
>> 
>> +(defvar gdb--window-configuration-before nil
>> +  "Stores the window configuration before starting gdb.")
>> +
>> +(defcustom gdb-restore-window-configuration-after-quit nil
>> +  "Whether to restore the window configuration the user had before gdb starts.
> 
> Probably better stick to the standard "If non-nil, restore...".
> 
>> @@ -4491,6 +4538,15 @@ gdb-preempt-existing-or-display-buffer
>>   (define-key gud-menu-map [displays]
>>     `(menu-item "GDB-Windows" ,menu
>>                :visible (eq gud-minor-mode 'gdbmi)))
>> +  (define-key menu [gdb-restore-windows]
>> +    '(menu-item "Restore Default Layout" gdb-restore-windows
>> +      :help "Restore standard layout for debug session. I.e., the layout we get when session starts."))
> 
> How about just "Restore standard layout for debug session (the layout we
> get when session starts)." Or, even more concisely, "Restore the initial
> GDB window layout"?
> 
>> +  (define-key menu [load-layout] '("Load Layout" "Load GDB window configuration (i.e. window layout) from a file" . gdb-load-window-configuration))
> 
> You can omit the "i.e.". Actually, even better perhaps just "window
> configuration (layout)"? Similarly below.
> 
>> @@ -4606,41 +4659,174 @@ gdb-set-window-buffer
>>   (set-window-buffer window (get-buffer name))
>>   (set-window-dedicated-p window t))
>> 
>> +(defun gdb-toggle-restore-window-configuration ()
>> +  "Toggle whether to restore window configuration when GDB quit."
>                                                               ^^^^
> quits
> 
>> +  (interactive)
>> +  (setq gdb-restore-window-configuration-after-quit
>> +        (not gdb-restore-window-configuration-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 source location
>> +where GDB 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
>                   ^^^^^
> started
> 
>> +file\" where the GDB session starts (see `gdb-main-file')."
>> +  (if gud-last-last-frame
>> +      (gud-find-file (car gud-last-last-frame))
>> +    (when gdb-main-file
>> +      (gud-find-file gdb-main-file))))
>> +
>> (defun gdb-setup-windows ()
>>   "Layout the window pattern for option `gdb-many-windows'."
>       ^^^^^^
> "Lay out" (verb)

All fixed.

> 
>> @@ -4658,7 +4844,12 @@ 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 arrangement depends on the value of option
>> +`gdb-many-windows' and `gdb-default-window-configuration-file'."
> 
> How about "This arrangement depends on the values of `gdb-many-windows'
> and `gdb-default-window-configuration-file'."
> 

I changed “option” to “variable”, I have to keep “variable” otherwise the byte compiler complains about there existing function and variable “gdb-many-windows”.

>> diff --git a/lisp/window.el b/lisp/window.el
>> index bd825c09e1..d774d906cd 100644
>> --- a/lisp/window.el
>> +++ b/lisp/window.el
>> @@ -278,6 +278,24 @@ with-displayed-buffer-window
>>             (funcall ,vquit-function ,window ,value)
>>           ,value)))))
>> 
>> +(defmacro with-window-non-dedicated (window &rest body)
>> +  "Evaluate BODY with WINDOW selected and temporarily made non-dedicated.
>                                 ^^^^^^^^^^^^
> This is now no longer true.
> 

Fixed.

> Thank you,
>  Štěpán

Thanks for catching all the issues.

> 
> P.S.: This time your patch somehow ended up hidden as
> application/ocet-stream after the HTML part, instead of inline in plain
> text.

I’m not sure what causes that. I’ve been using Apple Mail and I just copy the patch file into my reply. It actually surprises me that they showed up in plain text before. This time I made the mail plain text, hopefully that makes the patch also in plain text.

Yuan