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

classic Classic list List threaded Threaded
45 messages Options
123
Reply | Threaded
Open this post in threaded view
|

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

Fu Yuan
Right now if a user wants to use gdb with many windows, the only layout option is the default 6-window layout. This patch  allows a user to save her own layout, and use this layout in gdb-mi sessions. I also included my layout (`default-rearrange`) Maybe we can even ship with some default layouts that a user can choose from?







In GNU Emacs 27.0.50 (build 3, x86_64-apple-darwin19.0.0, NS appkit-1894.10 Version 10.15.1 (Build 19B88))
of 2019-11-30 built on missSilver
Repository revision: e2828795d73637577c7726965974a047fe2d7119
Repository branch: master
Windowing system distributor 'Apple', version 10.3.1894
System Description:  Mac OS X 10.15.2

Recent messages:
Checking 71 files in /Users/yuan/attic/emacs/lisp/erc...
Checking 34 files in /Users/yuan/attic/emacs/lisp/emulation...
Checking 180 files in /Users/yuan/attic/emacs/lisp/emacs-lisp...
Checking 24 files in /Users/yuan/attic/emacs/lisp/cedet...
Checking 59 files in /Users/yuan/attic/emacs/lisp/calendar...
Checking 87 files in /Users/yuan/attic/emacs/lisp/calc...
Checking 113 files in /Users/yuan/attic/emacs/lisp/obsolete...
Checking for load-path shadows...done
Auto-saving...
Buffer *unsent mail to [hidden email]*<2> modified; kill anyway? (y or n) y

Configured using:
'configure --with-modules --with-pdumper=yes
--oldincludedir=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/libxml2/'

Configured features:
NOTIFY KQUEUE ACL GNUTLS LIBXML2 ZLIB TOOLKIT_SCROLL_BARS NS MODULES
THREADS PDUMPER LCMS2

Important settings:
  value of $LC_CTYPE: UTF-8
  value of $LANG: en_CN.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Emacs-Lisp

Minor modes in effect:
  magit-todos-mode: t
  bug-reference-prog-mode: t
  desktop-save-mode: t
  ghelp-global-minor-mode: t
  minibuffer-electric-default-mode: t
  flymake-mode: t
  global-magit-file-mode: t
  magit-file-mode: t
  global-git-commit-mode: t
  async-bytecomp-package-mode: t
  shell-dirtrack-mode: t
  flyspell-mode: t
  outshine-mode: t
  ws-butler-global-mode: t
  ws-butler-mode: t
  minions-mode: t
  eyebrowse-mode: t
  savehist-mode: t
  global-hl-todo-mode: t
  hl-todo-mode: t
  global-highlight-parentheses-mode: t
  highlight-parentheses-mode: t
  rainbow-delimiters-mode: t
  global-undo-tree-mode: t
  undo-tree-mode: t
  electric-pair-mode: t
  winner-mode: t
  aggressive-indent-mode: t
  ivy-prescient-mode: t
  prescient-persist-mode: t
  recentf-mode: t
  which-key-mode: t
  general-override-mode: t
  outline-minor-mode: t
  ivy-mode: t
  company-mode: t
  override-global-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-quote-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  transient-mark-mode: t
  hs-minor-mode: t

Load-path shadows:
/Users/yuan/.emacs.d/ranch/winman/windman hides /Users/yuan/.emacs.d/ranch/windman/windman
/Users/yuan/.emacs.d/ranch/nerd-font/test/test-helper hides /Users/yuan/.emacs.d/ranch/doom-themes/test/test-helper
/Users/yuan/.emacs.d/ranch/julia-mode/julia-mode hides /Users/yuan/.emacs.d/package/julia-mode-20190813.1326/julia-mode
/Users/yuan/.emacs.d/ranch/julia-mode/julia-latexsubs hides /Users/yuan/.emacs.d/package/julia-mode-20190813.1326/julia-latexsubs
/Users/yuan/.emacs.d/ranch/matlab-emacs/mlint hides /Users/yuan/.emacs.d/package/matlab-mode-20180928.1526/mlint
/Users/yuan/.emacs.d/ranch/matlab-emacs/company-matlab-shell hides /Users/yuan/.emacs.d/package/matlab-mode-20180928.1526/company-matlab-shell
/Users/yuan/.emacs.d/ranch/matlab-emacs/linemark hides /Users/yuan/.emacs.d/package/matlab-mode-20180928.1526/linemark
/Users/yuan/.emacs.d/ranch/matlab-emacs/semanticdb-matlab hides /Users/yuan/.emacs.d/package/matlab-mode-20180928.1526/semanticdb-matlab
/Users/yuan/.emacs.d/ranch/matlab-emacs/semantic-matlab hides /Users/yuan/.emacs.d/package/matlab-mode-20180928.1526/semantic-matlab
/Users/yuan/.emacs.d/ranch/matlab-emacs/srecode-matlab hides /Users/yuan/.emacs.d/package/matlab-mode-20180928.1526/srecode-matlab
/Users/yuan/.emacs.d/ranch/matlab-emacs/matlab hides /Users/yuan/.emacs.d/package/matlab-mode-20180928.1526/matlab
/Users/yuan/.emacs.d/ranch/matlab-emacs/cedet-matlab hides /Users/yuan/.emacs.d/package/matlab-mode-20180928.1526/cedet-matlab
/Users/yuan/.emacs.d/ranch/matlab-emacs/tlc hides /Users/yuan/.emacs.d/package/matlab-mode-20180928.1526/tlc
/Users/yuan/.emacs.d/ranch/matlab-emacs/matlab-publish hides /Users/yuan/.emacs.d/package/matlab-mode-20180928.1526/matlab-publish
/Users/yuan/.emacs.d/ranch/matlab-emacs/matlab-mode-pkg hides /Users/yuan/.emacs.d/package/matlab-mode-20180928.1526/matlab-mode-pkg
/Users/yuan/.emacs.d/package/faceup-20170925.1946/faceup hides /Users/yuan/attic/emacs/lisp/emacs-lisp/faceup

Features:
(magit-todos pcre2el rxt re-builder grep checkdoc lisp-mnt bug-reference
vc-mtn vc-hg ffap tramp tramp-loaddefs trampver tramp-integration
files-x tramp-compat parse-time iso8601 ls-lisp shadow sort mail-extr
emacsbug sendmail vc-git vc-bzr vc-src vc-sccs vc-svn vc-cvs vc-rcs vc
vc-dispatcher magit-bookmark bookmark company-oddmuse company-keywords
company-etags etags fileloop company-gtags company-dabbrev-code
company-dabbrev company-files company-capf company-cmake company-xcode
company-clang company-semantic company-eclim company-template
company-bbdb hideshow desktop frameset trivial-copy ghelp-eglot
ghelp-helpful ghelp-builtin ghelp cus-edit cus-start cus-load
luna-publish utility pause luna-general-config minibuf-eldef eglot array
jsonrpc ert pp ewoc debug flymake-proc flymake warnings url-util
magit-submodule magit-obsolete magit-blame magit-stash magit-reflog
magit-bisect magit-push magit-pull magit-fetch magit-clone magit-remote
magit-commit magit-sequence magit-notes magit-worktree magit-tag
magit-merge magit-branch magit-reset magit-files magit-refs magit-status
magit magit-repos magit-apply magit-wip magit-log which-func magit-diff
smerge-mode diff-mode magit-core magit-autorevert autorevert filenotify
magit-margin magit-transient magit-process magit-mode transient
git-commit magit-git magit-section magit-utils crm log-edit message rmc
puny rfc822 mml mml-sec epa derived epg epg-config gnus-util rmail
rmail-loaddefs text-property-search mm-decode mm-bodies mm-encode
mail-parse rfc2231 rfc2047 rfc2045 mm-util ietf-drums mail-prsvr
mailabbrev mail-utils gmm-utils mailheader pcvs-util add-log with-editor
async-bytecomp async shell server flyspell ispell outshine
outshine-org-cmds outorg isolate inline expand-region
text-mode-expansions the-org-mode-expansions er-basic-expansions
thingatpt expand-region-core expand-region-custom ws-butler minions
eyebrowse savehist buffer-move windmove hl-todo highlight-parentheses
rainbow-delimiters doom-cyberpunk-theme undo-tree diff
doom-one-light-theme elec-pair winner doom-themes doom-themes-base
windman aggressive-indent find-char ivy-prescient prescient recentf-ext
recentf tree-widget wid-edit which-key general helpful imenu trace
edebug backtrace info-look f dash-functional help-fns radix-tree
elisp-refs s loop dash org-element avl-tree generator org advice
org-macro org-footnote org-pcomplete pcomplete org-list org-faces
org-entities time-date noutline outline org-version ob-emacs-lisp ob
ob-tangle org-src ob-ref ob-lob ob-table ob-keys ob-exp ob-comint
ob-core ob-eval org-compat org-macs org-loaddefs format-spec find-func
cal-menu calendar cal-loaddefs counsel xdg xref project dired
dired-loaddefs compile comint ansi-color swiper cl-extra help-mode ivy
delsel ring colir color ivy-overlay company edmacro kmacro pcase
use-package use-package-ensure use-package-delight use-package-diminish
use-package-bind-key bind-key easy-mmode use-package-core finder-inf
tex-site info cowboy package easymenu browse-url url-handlers url-parse
auth-source cl-seq eieio eieio-core cl-macs eieio-loaddefs
password-cache json subr-x map url-vars cl-loaddefs cl-lib lunary
lunary-ui luna-f rx seq byte-opt gv bytecomp byte-compile cconv tooltip
eldoc electric uniquify ediff-hook vc-hooks lisp-float-type mwheel
term/ns-win ns-win ucs-normalize mule-util term/common-win tool-bar dnd
fontset image regexp-opt fringe tabulated-list replace newcomment
text-mode elisp-mode lisp-mode prog-mode register page tab-bar menu-bar
rfn-eshadow isearch timer select scroll-bar mouse jit-lock font-lock
syntax facemenu font-core term/tty-colors frame minibuffer cl-generic
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite charscript charprop
case-table epa-hook jka-cmpr-hook help simple abbrev obarray
cl-preloaded nadvice loaddefs button faces cus-face macroexp files
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget hashtable-print-readable backquote threads kqueue cocoa ns
lcms2 multi-tty make-network-process emacs)

Memory information:
((conses 16 198008 22858)
(symbols 48 9376 47)
(strings 32 38332 2464)
(string-bytes 1 1117094)
(vectors 16 24687)
(vector-slots 8 290292 27848)
(floats 8 516 434)
(intervals 56 14668 1334)
(buffers 1000 28))

default-rearrange.png (46K) Download Attachment
store-window-new.patch (23K) Download Attachment
default-rearrange (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> From: Yuan Fu <[hidden email]>
> Date: Sat, 18 Jan 2020 15:57:32 -0500
>
> Right now if a user wants to use gdb with many windows, the only layout option is the default 6-window layout.

This is not entirely accurate.  The user could start "M-x gdb"
normally, then selectively display some of the other windows via the
Gud->GDB-Windows or Gud->GDB-Frames menus.

> This patch  allows a user to save her own layout, and use this layout in gdb-mi sessions.

I'd prefer to extend the existing capability by adding a feature, both
in the menu bar and as a command, to save and restore the window
configuration set via the above-mentioned existing facilities.  WDYT?

Please also fix all the minor nits I mentioned in review of your
previous patches: quoting, 2 blanks between sentences, complete
sentences in comments and log messages, etc.  Last, but not least,
please don't break a single patch into several separate ones, unless
each one of the separate patches can make sense if applied on its own.

Thanks.



Reply | Threaded
Open this post in threaded view
|

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

Fu Yuan


> On Jan 31, 2020, at 5:13 AM, Eli Zaretskii <[hidden email]> wrote:
>
>> From: Yuan Fu <[hidden email]>
>> Date: Sat, 18 Jan 2020 15:57:32 -0500
>>
>> Right now if a user wants to use gdb with many windows, the only layout option is the default 6-window layout.
>
> This is not entirely accurate.  The user could start "M-x gdb"
> normally, then selectively display some of the other windows via the
> Gud->GDB-Windows or Gud->GDB-Frames menus.
>
>> This patch  allows a user to save her own layout, and use this layout in gdb-mi sessions.
>
> I'd prefer to extend the existing capability by adding a feature, both
> in the menu bar and as a command, to save and restore the window
> configuration set via the above-mentioned existing facilities.  WDYT?

Thanks makes sense; will do. I’ll put them in GUD windows menu.


> Please also fix all the minor nits I mentioned in review of your
> previous patches: quoting, 2 blanks between sentences, complete
> sentences in comments and log messages, etc.  Last, but not least,
> please don't break a single patch into several separate ones, unless
> each one of the separate patches can make sense if applied on its own.

Will do.

Yuan




Reply | Threaded
Open this post in threaded view
|

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

Fu Yuan
Here is the new patch. New-store-window.patch contains the one allowing user store/restore window config. I added the two menu items. And I changed the name to gdb-save-window-layout and gdb-load-window-layout because I think they are shorter and easier to understand; WDYT?

Restore-after-quit.patch contain the patch which restores the  original window configuration after gdb quits. Maybe I should send it as a separate patch?

BTW, It used to be three commits, I merged the first one on window.el into new-restore-window.patch.

Yuan


new-store-window.patch (2K) Download Attachment
restore-after-quit.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

martin rudalics
 > Martin, any comments?

IMHO "restoring the previous window layout when gdb quits" should be
opt-in (or at least opt-out).  Maybe it also should restore the layout
only when 'gdb-many-windows' is non-nil, so we'd have the three option
values nil, t, and if-gdb-many-windows (in a menu entry).

martin



Reply | Threaded
Open this post in threaded view
|

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

Fu Yuan


On Feb 15, 2020, at 5:19 AM, Eli Zaretskii <[hidden email]> wrote:

Cc: [hidden email]
From: martin rudalics <[hidden email]>
Date: Sat, 15 Feb 2020 10:55:34 +0100

Martin, any comments?

IMHO "restoring the previous window layout when gdb quits" should be
opt-in (or at least opt-out).  Maybe it also should restore the layout
only when 'gdb-many-windows' is non-nil, so we'd have the three option
values nil, t, and if-gdb-many-windows (in a menu entry).

I had the same thoughts myself, and I agree on both counts.

I agree. I made it into a custom variable with choices, is that what you mean by menu entry?

Yuan



restore-after-quit-new.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

martin rudalics
 > I agree. I made it into a custom variable with choices,

That's the first part of what I meant.

 > is that what you mean by menu entry?

The menu entry could be written on top of the 'defcustom' and appear as
a toggle somewhere near the "Restore Window Layout" menu entry.  Also we
should explain the differences between that entry and your new option
since otherwise users might get confused.

martin



Reply | Threaded
Open this post in threaded view
|

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

Fu Yuan
Sorry for the delay, I missed the mail and was waiting for your reply. I added a toggle button in the menu with help echos. How does it look? Since there are more than two options, I make the button to toggle between the two basic ones. Also I combined two patches into one.

Yuan



new-window.patch (26K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Fu Yuan
Actually, it is more intuitive to _alwaya_ save the original window layout on startup and restore when quit  depending on the toggle variable. Let me fix that tonight. Sorry for the fuzz.

Yuan

> 在 2020年3月3日,下午6:41,Yuan Fu <[hidden email]> 写道:
>
> Sorry for the delay, I missed the mail and was waiting for your reply. I added a toggle button in the menu with help echos. How does it look? Since there are more than two options, I make the button to toggle between the two basic ones. Also I combined two patches into one.
>
> Yuan
>
>
> <new-window.patch>



Reply | Threaded
Open this post in threaded view
|

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

Fu Yuan
Here is the patch.

Yuan





new-window.patch (26K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

martin rudalics
 > Here is the patch.

Thanks. It gets me here the following warnings when building on master
(sorry, I'm a complete ignorant of the 'cl-' snafu):

   ELC      progmodes/gdb-mi.elc

In toplevel form:
../../lisp/progmodes/gdb-mi.el:4738:1: Warning: Unused lexical variable
     ‘window-config’

In end of data:
../../lisp/progmodes/gdb-mi.el:5069:1: Warning: the following functions might
     not be defined at runtime: cl-delete-if, cl-find-if, cl-mapcar

A few remarks:

+  "If non-nil, gdb loads this window layout file on startup.
+If not absolute, GDB will look under `gdb-window-layout-directory'."

We should settle in descriptions on whether we write gdb or GDB.  Also,
"if not absolute" is too terse IMHO.

+  (define-key menu [load-layout] '("Load window layout" "Load GDB window layout from a file" . gdb-load-window-layout))
+  (define-key menu [save-layout] '("Save window layout" "Save current GDB window layout to a file" . gdb-save-window-layout))

I think we could omit the "window" in these which should allow us to,
instead of

+    '(menu-item "Restore window layout"

say

+    '(menu-item "Restore layout after quit"

so it becomes more clear that "load" and "save" act _immediately_ on the
current state while "restore" is a more general setting that becomes
effective only when the user quits.  (Note also that in general we
cannot guarantee that menu tooltips are always shown on each and every
system where Emacs runs.)

Also, I would mention all four possible values of
'gdb-restore-window-layout-after-quit' (currently "toggle" indicates
that there are only two of them) like, for example, with the side values
for tool-bar mode.

+  "Return a buffer displaying source file or nil.
+
+The source file would be the most relevant file or the main file."

This is IMHO too terse in every respect.  Neither "source file" nor
"main file" are canonical terms in the context of GDB so we should
explain here how they are set up (what is nil in this context?).

+E.g., locals buffer, registers buffer, but don't include the main

I would say "Function buffers are locals buffers, ...".

+(defun gdb--buffer-type (buffer)
+  "Return the buffer type of BUFFER or nil.

Maybe

"Return the type of BUFFER if it is a GDB function buffer."

would be better.

Thanks for your work on this, martin




Reply | Threaded
Open this post in threaded view
|

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

Fu Yuan


> On Mar 5, 2020, at 4:14 AM, martin rudalics <[hidden email]> wrote:
>
> > Here is the patch.
>
> Thanks. It gets me here the following warnings when building on master
> (sorry, I'm a complete ignorant of the 'cl-' snafu):
>
>  ELC      progmodes/gdb-mi.elc
>
> In toplevel form:
> ../../lisp/progmodes/gdb-mi.el:4738:1: Warning: Unused lexical variable
>    ‘window-config’
>
> In end of data:
> ../../lisp/progmodes/gdb-mi.el:5069:1: Warning: the following functions might
>    not be defined at runtime: cl-delete-if, cl-find-if, cl-mapcar
Fixed.

>
> A few remarks:
>
> +  "If non-nil, gdb loads this window layout file on startup.
> +If not absolute, GDB will look under `gdb-window-layout-directory'."
>
> We should settle in descriptions on whether we write gdb or GDB.  Also,
> "if not absolute" is too terse IMHO.
>
> +  (define-key menu [load-layout] '("Load window layout" "Load GDB window layout from a file" . gdb-load-window-layout))
> +  (define-key menu [save-layout] '("Save window layout" "Save current GDB window layout to a file" . gdb-save-window-layout))
>
> I think we could omit the "window" in these which should allow us to,
> instead of
>
> +    '(menu-item "Restore window layout"
>
> say
>
> +    '(menu-item "Restore layout after quit"
>
> so it becomes more clear that "load" and "save" act _immediately_ on the
> current state while "restore" is a more general setting that becomes
> effective only when the user quits.  (Note also that in general we
> cannot guarantee that menu tooltips are always shown on each and every
> system where Emacs runs.)

>
> Also, I would mention all four possible values of
> 'gdb-restore-window-layout-after-quit' (currently "toggle" indicates
> that there are only two of them) like, for example, with the side values
> for tool-bar mode.

I removed “window” in their names. I mentioned the other possible values in the help echo, is that fine? Not sure where else I can put it.

>
> +  "Return a buffer displaying source file or nil.
> +
> +The source file would be the most relevant file or the main file."
>
> This is IMHO too terse in every respect.  Neither "source file" nor
> "main file" are canonical terms in the context of GDB so we should
> explain here how they are set up (what is nil in this context?).
>
> +E.g., locals buffer, registers buffer, but don't include the main
>
> I would say "Function buffers are locals buffers, ...".
>
> +(defun gdb--buffer-type (buffer)
> +  "Return the buffer type of BUFFER or nil.
>
> Maybe
>
> "Return the type of BUFFER if it is a GDB function buffer."
>
> would be better.
I added some explanations. I hope they are clear now.

> Thanks for your work on this, Martin

Thanks for reviewing :-)

Here is the new patch.

Yuan







new-window.patch (27K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Štěpán Němec
On Sat, 7 Mar 2020 13:09:53 -0500
Yuan Fu wrote:

[...]

> diff --git a/lisp/window.el b/lisp/window.el
> index bd825c09e1..229400966a 100644
> --- a/lisp/window.el
> +++ b/lisp/window.el
> @@ -278,6 +278,19 @@ with-displayed-buffer-window
>       (funcall ,vquit-function ,window ,value)
>     ,value)))))
>  
> +(defmacro with-selected-window-undedicated (&rest body)
> +  "Run BODY in the selected window temporarily undedicated."
> +  (let ((window-dedicated-sym (gensym)))
> +    `(let ((,window-dedicated-sym (window-dedicated-p)))
> +       (when ,window-dedicated-sym
> +         (set-window-dedicated-p nil nil))
> +       ,@body
> +       (when ,window-dedicated-sym
> +         ;; `window-dedicated-p' returns the value set by
> +         ;; `set-window-dedicated-p', which differentiates
> +         ;; non-nil and t, so we cannot simply set to t.
> +         (set-window-dedicated-p nil ,window-dedicated-sym)))))
> +
>  ;; The following two functions are like `window-next-sibling' and
>  ;; `window-prev-sibling' but the WINDOW argument is _not_ optional (so
>  ;; they don't substitute the selected window for nil), and they return

I'm sorry, I only skimmed through your patch, but shouldn't this use
'unwind-protect'? Otherwise the "temporarily" won't hold in case of
abnormal exit from BODY, unless I'm missing something.

--
Štěpán



Reply | Threaded
Open this post in threaded view
|

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

Fu Yuan


> On Mar 7, 2020, at 2:07 PM, Štěpán Němec <[hidden email]> wrote:
>
> On Sat, 7 Mar 2020 13:09:53 -0500
> Yuan Fu wrote:
>
> [...]
>
>> diff --git a/lisp/window.el b/lisp/window.el
>> index bd825c09e1..229400966a 100644
>> --- a/lisp/window.el
>> +++ b/lisp/window.el
>> @@ -278,6 +278,19 @@ with-displayed-buffer-window
>>     (funcall ,vquit-function ,window ,value)
>>   ,value)))))
>>
>> +(defmacro with-selected-window-undedicated (&rest body)
>> +  "Run BODY in the selected window temporarily undedicated."
>> +  (let ((window-dedicated-sym (gensym)))
>> +    `(let ((,window-dedicated-sym (window-dedicated-p)))
>> +       (when ,window-dedicated-sym
>> +         (set-window-dedicated-p nil nil))
>> +       ,@body
>> +       (when ,window-dedicated-sym
>> +         ;; `window-dedicated-p' returns the value set by
>> +         ;; `set-window-dedicated-p', which differentiates
>> +         ;; non-nil and t, so we cannot simply set to t.
>> +         (set-window-dedicated-p nil ,window-dedicated-sym)))))
>> +
>> ;; The following two functions are like `window-next-sibling' and
>> ;; `window-prev-sibling' but the WINDOW argument is _not_ optional (so
>> ;; they don't substitute the selected window for nil), and they return
>
> I'm sorry, I only skimmed through your patch, but shouldn't this use
> 'unwind-protect'? Otherwise the "temporarily" won't hold in case of
> abnormal exit from BODY, unless I'm missing something.
>
> —
> Štěpán
Thanks for spotting that. I added the unwind-protext form.

Yuan







new-window.patch (27K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

martin rudalics
 >> I'm sorry, I only skimmed through your patch, but shouldn't this use
 >> 'unwind-protect'? Otherwise the "temporarily" won't hold in case of
 >> abnormal exit from BODY, unless I'm missing something.
 >>
 >> —
 >> Štěpán
 >
 > Thanks for spotting that. I added the unwind-protext form.

If we want to be more strict about this macro then how about the
following forms:

(with-selected-window-undedicated
  (set-window-dedicated-p nil t))

will leave the selected window dedicated which does not really violate
the contract of the macro but is unexpected at least.

The following is more serious: Suppose a user has a >= 2 windows layout
and does

(set-window-dedicated-p nil t)
(with-selected-window-undedicated
  (other-window 1))

which will have the macro make some other window dedicated and the
initially selected window undedicated.  A similar thing happens with

(set-window-dedicated-p nil t)
(with-selected-window-undedicated
  (delete-window))

The macro should be named 'with-window-undedicated', take a WINDOW (nil
for the selected one) as first argument and BODY as second.  It should
restore the dedicated status of WINDOW to what it was before running
BODY and leave the dedicated status of all other windows alone.  IMHO.

martin




Reply | Threaded
Open this post in threaded view
|

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

Fu Yuan


> On Mar 9, 2020, at 5:01 AM, martin rudalics <[hidden email]> wrote:
>
> >> I'm sorry, I only skimmed through your patch, but shouldn't this use
> >> 'unwind-protect'? Otherwise the "temporarily" won't hold in case of
> >> abnormal exit from BODY, unless I'm missing something.
> >>
> >> —
> >> Štěpán
> >
> > Thanks for spotting that. I added the unwind-protext form.
>
> If we want to be more strict about this macro then how about the
> following forms:
>
> (with-selected-window-undedicated
> (set-window-dedicated-p nil t))
>
> will leave the selected window dedicated which does not really violate
> the contract of the macro but is unexpected at least.
>
> The following is more serious: Suppose a user has a >= 2 windows layout
> and does
>
> (set-window-dedicated-p nil t)
> (with-selected-window-undedicated
> (other-window 1))
>
> which will have the macro make some other window dedicated and the
> initially selected window undedicated.  A similar thing happens with
>
> (set-window-dedicated-p nil t)
> (with-selected-window-undedicated
> (delete-window))
>
> The macro should be named 'with-window-undedicated', take a WINDOW (nil
> for the selected one) as first argument and BODY as second.  It should
> restore the dedicated status of WINDOW to what it was before running
> BODY and leave the dedicated status of all other windows alone.  IMHO.
>
> Martin
>
I updated the patch accordingly. Could you have a look at the docsting? I had a hard time writing it.

Yuan





new-window.patch (28K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Štěpán Němec

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 <[hidden email]>
> 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



Reply | Threaded
Open this post in threaded view
|

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

Fu Yuan

On Mar 9, 2020, at 3:18 PM, Štěpán Němec <[hidden email]> wrote:


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.)


No, not really, that’s my fault. Before I go in and change every word, which one do you think I should use: configuration or layout? Configuration is the conventional Emacs term but I felt that layout is easier to understand for a user. If you are find with both I guess I will change all to configuration.

Yuan 
Reply | Threaded
Open this post in threaded view
|

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

Štěpán Němec
On Mon, 09 Mar 2020 16:17:59 -0400
Yuan Fu wrote:

>> On Mar 9, 2020, at 3:18 PM, Štěpán Němec <[hidden email]> wrote:
>>
>>
>> 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.)
>>
>
> No, not really, that’s my fault. Before I go in and change every word,
> which one do you think I should use: configuration or layout?
> Configuration is the conventional Emacs term but I felt that layout is
> easier to understand for a user. If you are find with both I guess I
> will change all to configuration.

If they mean the same thing, then I'd say "configuration" is definitely
the way to go.

It is true that some Emacs terms might be unfamiliar to new users, but
thanks to the excellent documentation capabilities it's very easy to
learn the respective definitions (e.g., in this case 'C-h r i
configuration' will display the relevant manual entries). It is
important to stay consistent.

--
Štěpán



Reply | Threaded
Open this post in threaded view
|

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

martin rudalics
In reply to this post by Fu Yuan
 > I updated the patch accordingly. Could you have a look at the docsting? I had a hard time writing it.

Please don't select WINDOW in 'with-window-undedicated'.  In general,
try to avoid selecting a window unless it is really needed.  This is
particularly important when WINDOW can be on another frame where
selecting WINDOW entails switching to that frame with all its overhead.

I would use something like the untested below.  This could be then
useful for 'ffap-other-frame' or 'ffap-dired-other-frame' as well.

martin


(defmacro with-window-undedicated (window &rest body)
   "Execute BODY with WINDOW temporarily undedicated.
WINDOW must be a live window and defaults to the selected one."
   (declare (indent 1) (debug t))
   (let ((window-dedicated-sym (gensym))
         (window-sym (gensym)))
     `(let* ((,window-sym (window-normalize-window window t))
            (,window-dedicated-sym (window-dedicated-p ,window-sym)))
        (set-window-dedicated-p ,window-sym nil)
        (unwind-protect
            (progn ,@body)
         (set-window-dedicated-p ,window-sym ,window-dedicated-sym)))))



123