Re: [NonGNU ELPA] New package: tabspaces
Philip Kaludercic <[email protected]> Sat, 01 Aug 2026 10:00:56 +0000
| Newsgroups | gmane.emacs.devel |
|---|---|
| Message-ID | <[email protected]> |
--=-=-= Content-Type: text/plain Colin McLear <[email protected]> writes: > Hi, > > I'd like to propose my package tabspaces for inclusion in NonGNU ELPA. > > Repository: https://codeberg.org/mclear-tools/tabspaces Thanks, first of all, I would recommend adding an .elpaignore file to exclude the screenshots from the release tarball. I would also exclude the tests and the Makefile, but you don't have to and that is up to you to decide. Here is a diff of comments and suggestions I made while reviewing the code: --=-=-= Content-Type: text/x-patch Content-Disposition: inline diff --git a/tabspaces.el b/tabspaces.el index 89fa6f6..90df8c5 100644 --- a/tabspaces.el +++ b/tabspaces.el @@ -1,14 +1,14 @@ ;;; tabspaces.el --- Leverage tab-bar and project for buffer-isolated workspaces -*- lexical-binding: t -*- +;; Copyright (C) 2022 Colin McLear + ;; Author: Colin McLear <[email protected]> -;; Maintainer: Colin McLear +;; Maintainer: Colin McLear <[email protected]> ;; Version: 1.9.0 ;; Package-Requires: ((emacs "27.1") (project "0.8.1")) ;; Keywords: convenience, frames ;; Homepage: https://codeberg.org/mclear-tools/tabspaces -;; Copyright (C) 2022 Colin McLear - ;; This file is not part of GNU Emacs ;; This program is free software: you can redistribute it and/or modify @@ -24,6 +24,8 @@ ;; You should have received a copy of the GNU General Public License ;; along with this program. If not, see <https://www.gnu.org/licenses/>. +;; This file is not part of GNU Emacs. + ;;; Commentary: ;; This package provides several functions to facilitate a frame-based @@ -32,9 +34,8 @@ ;; workspace). The package assumes project.el and tab-bar.el are both present ;; (they are built-in to Emacs 27.1+). -;; This file is not part of GNU Emacs. +;;;; Acknowledgements -;;; Acknowledgements ;; Much of the package code is inspired by: ;; - https://github.com/kaz-yos/emacs @@ -75,12 +76,10 @@ (defcustom tabspaces-default-tab "Default" "Specify a default tab by name TAB." - :group 'tabspaces :type 'string) (defcustom tabspaces-remove-to-default t "Add buffer to default tabspace when removed from current tabspace." - :group 'tabspaces :type 'boolean) (defcustom tabspaces-include-buffers '("*scratch*") @@ -88,7 +87,6 @@ This is a list of buffer names, matched exactly (not as regular expressions), which overrides buffers excluded by `tabspaces-exclude-buffers'." - :group 'tabspaces :type '(repeat string)) (defcustom tabspaces-exclude-buffers nil @@ -96,31 +94,26 @@ expressions), which overrides buffers excluded by This is a list of buffer names, matched exactly (not as regular expressions), which does not override buffers inside `tabspaces-include-buffers'." - :group 'tabspaces :type '(repeat string)) (defcustom tabspaces-use-filtered-buffers-as-default nil "When t, remap `switch-to-buffer' to `tabspaces-switch-to-buffer'." - :group 'tabspaces :type 'boolean) (defcustom tabspaces-keymap-prefix "C-c TAB" "Key prefix for the tabspaces-prefix-map keymap. Set to nil to disable automatic keymap binding." - :group 'tabspaces :type '(choice (const :tag "Disabled" nil) - string)) + key-sequence)) (defcustom tabspaces-initialize-project-with-todo t "Whether to create a `tabspaces-todo-file-name' file in new workspaces. When non-nil, create the file in the project when creating a workspace for it." - :group 'tabspaces :type 'boolean) (defcustom tabspaces-todo-file-name "project-todo.org" "The name of the TODO file to create if non-existing for new workspaces." - :group 'tabspaces :type 'string) (defcustom tabspaces-project-switch-commands project-switch-commands @@ -128,8 +121,7 @@ workspace for it." Change this value if you wish to run a specific command, such as `find-file' on project switch. Otherwise this will default to the value of `project-switch-commands'." - :group 'tabspaces - :type 'sexp) + :type (get 'project-switch-commands 'custom-type)) (defcustom tabspaces-project-fallback-to-tab t "When non-nil, project.el commands fall back to the current tab's project. @@ -140,7 +132,6 @@ workspace has an obvious project. With this option, the current tab's project (per `tabspaces-project-tab-map') is used as a fallback. A buffer that is itself inside a project keeps its own project; the tab never overrides it." - :group 'tabspaces :type 'boolean) (defcustom tabspaces-project-switch-opens-workspace nil @@ -150,7 +141,6 @@ Routes the stock \\[project-switch-project] through project.el switching also creates or reuses the project's tab. Takes effect when `tabspaces-mode' is enabled; re-enable the mode after changing this option." - :group 'tabspaces :type 'boolean) (defcustom tabspaces-initialize-project-with-vc t @@ -159,27 +149,22 @@ Uses magit when available, otherwise `vc-create-repo'. When nil, write an empty \".project\" marker file into the new directory instead; add \".project\" to `project-vc-extra-root-markers' so project.el recognizes such directories as projects." - :group 'tabspaces :type 'boolean) (defcustom tabspaces-fully-resolve-paths nil "Resolve \".\", \"..\", etc. in project paths." - :group 'tabspaces :type 'boolean) (defcustom tabspaces-session t "Whether to save tabspaces across sessions." - :group 'tabspaces :type 'boolean) (defcustom tabspaces-session-auto-restore nil "Whether to restore tabspaces on session startup." - :group 'tabspaces :type 'boolean) (defcustom tabspaces-session-file (locate-user-emacs-file "tabsession.el") "File for saving tabspaces session." - :group 'tabspaces :type 'file) (defcustom tabspaces-session-auto-save-delay nil @@ -189,7 +174,6 @@ after this many seconds of idle time, so a crash or a killed Emacs loses at most the changes since the last idle period. Capturing window configurations briefly cycles through the tabs. The default \(nil) saves only on exit, via `kill-emacs-hook'." - :group 'tabspaces :type '(choice (const :tag "Disabled" nil) number)) (defcustom tabspaces-session-project-session-store 'project @@ -198,7 +182,6 @@ Can be one of: - \\='project (default) - Store in the project root directory - a string path - Store all project sessions in this directory - a function - Called with project root path to determine session file location" - :group 'tabspaces :type '(choice (const :tag "In project directory" project) (directory :tag "In specific directory") @@ -206,17 +189,14 @@ Can be one of: (defcustom tabspaces-echo-area-enable nil "Display tabs in echo area instead of tab-bar when enabled." - :group 'tabspaces :type 'boolean) (defcustom tabspaces-echo-area-format-function #'tabspaces--echo-area-format-tabs "Function to format tabs for echo area display." - :group 'tabspaces :type 'function) (defcustom tabspaces-echo-area-idle-delay 1.0 "Number of seconds to wait before showing tabs when idle." - :group 'tabspaces :type 'number :set (lambda (symbol value) (set-default symbol value) @@ -391,9 +371,9 @@ Only the current window buffers and buffers in `tabspaces-include-buffers' are kept in the `buffer-list' and `buried-buffer-list'." (interactive) - ;; https://www.gnu.org/software/emacs/manual/html_node/elisp/Current-Buffer.html - ;; The current-tab uses `buffer-list' and `buried-buffer-list'. - ;; A hidden tab keeps these as `wc-bl' and `wc-bbl'. + ;; (elisp) Current Buffer: The current-tab uses `buffer-list' and + ;; `buried-buffer-list'. A hidden tab keeps these as `wc-bl' and + ;; `wc-bbl'. (set-frame-parameter nil 'buffer-list (let ((window-buffers (mapcar #'window-buffer (window-list)))) @@ -449,6 +429,8 @@ non-nil, then specify a tab index in the given frame." ;;;; Project Workspace Helper Functions +;; why are these internal functions autoloaded? + ;;;###autoload (defun tabspaces--current-tab-name () "Get name of current tab." @@ -467,6 +449,7 @@ version-controlled projects and projects recognized through markers in `project-vc-extra-root-markers'." (let ((project (project-current))) (if project + ;; this is not necessarily unique, is that an issue? (file-name-nondirectory (directory-file-name (project-root project))) "-"))) @@ -486,6 +469,9 @@ of side effects and always return a string." "Add BUFFER to default tabspace buffer list." (let ((tab-names (mapcar (lambda (tab) (alist-get 'name tab)) + ;; In `tabspaces--list-tabspaces' you cann + ;; `tab-bar-tabs' directly, but here you use a + ;; variable. Why is that? (funcall tab-bar-tabs-function)))) (when (and tabspaces-remove-to-default (member tabspaces-default-tab tab-names)) @@ -589,8 +575,7 @@ If `tabspaces-remove-to-default' is t then add the buffer to the default tabspace." (interactive (list - (let ((blst (mapcar (lambda (b) (buffer-name b)) - (tabspaces--buffer-list)))) + (let ((blst (mapcar #'buffer-name (tabspaces--buffer-list)))) ;; select buffer (read-buffer (format "Remove buffer from `%s' tabspace: " (tabspaces--current-tab-name)) @@ -617,7 +602,7 @@ The arguments NORECORD and FORCE-SAME-WINDOW are passed to `switch-to-buffer'." (while xs (unless (member (car xs) ys) ; Don't check it if already known to be a dup. (when (member (car xs) (cdr xs)) (push (car xs) ys))) - (setq xs (cdr xs))) + (setq xs (cdr xs))) ys)) (defun tabspaces-switch-buffer-and-tab (buffer &optional norecord force-same-window) @@ -640,6 +625,11 @@ current tab. NORECORD and FORCE-SAME-WINDOW are passed to ;; This is the list of all buffers to search through. (bufflst (flatten-tree (dolist (tab (tabspaces--list-tabspaces) buflst) (push (mapcar #'buffer-name (tabspaces--buffer-list nil (tab-bar--tab-index-by-name tab))) buflst)))) + ;; You don't need to assemble an entire list of duplicates to + ;; just check for a single duplicate. It is easier to call + ;; member/memq twice. If (memq x (cdr (memq x list))) is + ;; non-nil, then the list must contain at least two same + ;; members. (dupe (member buffer (tabspaces--report-dupes bufflst)))) ;; Run through conditions: (cond @@ -647,14 +637,16 @@ current tab. NORECORD and FORCE-SAME-WINDOW are passed to ((and (get-buffer buffer) (not dupe)) (dolist (tab (tabspaces--list-tabspaces)) - (when (member buffer (mapcar #'buffer-name (tabspaces--buffer-list nil (tab-bar--tab-index-by-name tab)))) - (progn (tab-bar-switch-to-tab tab) - (tabspaces-switch-to-buffer buffer))))) + (when (cl-member buffer (tabspaces--buffer-list nil (tab-bar--tab-index-by-name tab)) + :key #'buffer-name) + (tab-bar-switch-to-tab tab) + (tabspaces-switch-to-buffer buffer)))) ;; 2. Buffer exists and is open in more than one tabspace. ((and (get-buffer buffer) dupe) (dolist (tab (tabspaces--list-tabspaces) tabcand) - (when (member buffer (mapcar #'buffer-name (tabspaces--buffer-list nil (tab-bar--tab-index-by-name tab)))) + (when (cl-member buffer (tabspaces--buffer-list nil (tab-bar--tab-index-by-name tab)) + :key #'buffer-name) (push tab tabcand))) (progn (tab-bar-switch-to-tab (completing-read "Select tab: " tabcand)) @@ -723,6 +715,7 @@ not mean what `tab-bar-select-tab' makes them mean). If tab names are not unique the diff can be ambiguous, in which case the map is left alone." (let* ((names-of (lambda () + ;; here again, isn't this `tabspaces--list-tabspaces'? (mapcar (lambda (tab) (alist-get 'name tab)) (funcall tab-bar-tabs-function)))) (before (funcall names-of)) @@ -783,7 +776,9 @@ match and base name without suffix." (defun tabspaces-generate-descriptive-tab-name (project-path existing-tab-names) "Generate a unique tab name from PROJECT-PATH. Checks for conflicts against EXISTING-TAB-NAMES." - (let* ((parts (reverse (split-string (directory-file-name project-path) "/"))) + ;; In the future, it will probably be better to use + ;; `file-name-split' (Emacs 29+ or via Compat) + (let* ((parts (nreverse (split-string (directory-file-name project-path) "/"))) (base-name (car parts)) (parent-dir (nth 1 parts)) (grandparent-dir (nth 2 parts)) @@ -803,9 +798,8 @@ Checks for conflicts against EXISTING-TAB-NAMES." ;; Use the complex name for the new tab to avoid future conflicts complex-tab-name) ;; No conflict, add to map and use the simple name - (progn - (add-to-list 'tabspaces-project-tab-map (cons project-path simple-tab-name)) - simple-tab-name)))) + (add-to-list 'tabspaces-project-tab-map (cons project-path simple-tab-name)) + simple-tab-name))) (defun tabspaces-generate-complex-name (project-path) "Generate a complex tab name from PROJECT-PATH. @@ -842,11 +836,14 @@ fall through to ORIG-FUN with ARGS." DIR, DEFAULT, and MUSTMATCH are passed to `read-directory-name'." (let ((dir-name (read-directory-name prompt dir default mustmatch))) (unless (file-directory-p dir-name) + ;; I would recommend consistently using `yes-or-no-p', because + ;; the people who want short answers will configure Emacs to + ;; support it while the people who want to be more careful + ;; prefer having to type out "yes" or "no". (when (y-or-n-p (format "Directory %s does not exist. Create it?" dir-name)) (make-directory dir-name t))) dir-name)) - ;; Replace project-prompt-project-dir for project creation (defun tabspaces-prompt-project-dir () "Prompt the user for a directory that is one of the known project roots. @@ -863,8 +860,12 @@ It's also possible to enter an arbitrary directory not in the list." (pr-dir "")) (while (equal pr-dir "") ;; If the user simply pressed RET, do this again until they don't. + ;; + ;; This seems passive aggressive, at least print a short message + ;; explaining what is happening. Something like (message "You + ;; have to decide") (sit-for 0.5) (setq pr-dir (completing-read "Select project: " choices nil t))) - (if (equal pr-dir dir-choice) + (if (file-equal-p pr-dir dir-choice) (tabspaces--read-directory-name "Select directory: " nil nil nil) pr-dir))) @@ -972,7 +973,7 @@ With universal argument PREFIX, always create a new tab for the project." ;; Update tabspaces-project-tab-map (only for the main tab, not numbered ;; duplicates). - (unless (string-match-p "<[0-9]+>$" tab-name) + (unless (string-match-p "<[0-9]+>\\'" tab-name) (tabspaces--remember-project-tab project-directory tab-name)))) ;;;; Tabspace Sessions @@ -1041,7 +1042,7 @@ non-nil wins." (when (buffer-live-p b) (with-current-buffer b (cond - (buffer-file-name buffer-file-name) + (buffer-file-name) (t (catch 'found (dolist (entry tabspaces--buffer-kind-handlers) (let ((rec (funcall (nth 1 entry) b))) @@ -1059,17 +1060,18 @@ Each record is either a file path string or a plist of the form "Write SESSION-LIST and `tabspaces-project-tab-map' to FILE. NOTE, if non-nil, is an extra comment line placed after the header." (with-temp-file file - (insert ";; -*- mode: emacs-lisp; lexical-binding:t; coding: utf-8-emacs; -*-\n" - tabspaces-session-header - ";; Created " (current-time-string) "\n\n") - (when note - (insert ";; " note "\n\n")) - (insert ";; Project to tab name mapping:\n" - "(setq tabspaces-project-tab-map '" - (format "%S" tabspaces-project-tab-map) ")\n\n" - ";; Tabs and buffers:\n" - "(setq tabspaces--session-list '" - (format "%S" session-list) ")"))) + (let ((standard-output (current-buffer))) + (insert ";; -*- mode: emacs-lisp; lexical-binding:t; coding: utf-8-emacs; -*-\n" + tabspaces-session-header + ";; Created " (current-time-string) "\n\n") + (when note + (insert ";; " note "\n\n")) + + (insert ";; Project to tab name mapping:") + (print `(setq tabspaces-project-tab-map ',tabspaces-project-tab-map)) + + (insert ";; Tabs and buffers:") + (print `(setq tabspaces--session-list ',session-list))))) ;; Save global session ;;;###autoload @@ -1081,14 +1083,14 @@ NOTE, if non-nil, is an extra comment line placed after the header." (let ((curr (tab-bar--current-tab-index))) ;; loop over tabs (cl-loop for tab in (tabspaces--list-tabspaces) - do (progn - (tab-bar-select-tab-by-name tab) - (setq tabspaces--session-list - (append tabspaces--session-list - (list (list - (tabspaces--store-buffers (tabspaces--buffer-list)) - tab - (window-state-get nil t))))))) + do + (tab-bar-select-tab-by-name tab) + (setq tabspaces--session-list + (append tabspaces--session-list + (list (list + (tabspaces--store-buffers (tabspaces--buffer-list)) + tab + (window-state-get nil t)))))) ;; As tab-bar-select-tab starts counting from 1, we need to add 1 to the index. (tab-bar-select-tab (+ curr 1))) ;; Write to file @@ -1245,22 +1247,19 @@ Does nothing unless both `tabspaces-session' and (error "Not in a project")))) (project-name (file-name-nondirectory (directory-file-name project-root))) (session-name (concat "." project-name "-tabspaces-session.el"))) - (cond - ((eq tabspaces-session-project-session-store 'project) - (expand-file-name session-name project-root)) - - ((stringp tabspaces-session-project-session-store) - (expand-file-name session-name tabspaces-session-project-session-store)) - - ((functionp tabspaces-session-project-session-store) - (funcall tabspaces-session-project-session-store project-root)) - - (t (expand-file-name session-name project-root))))) + (pcase tabspaces-session-project-session-store + ('project (expand-file-name session-name project-root)) + ((pred stringp) + (expand-file-name session-name tabspaces-session-project-session-store)) + ((pred functionp) + (funcall tabspaces-session-project-session-store project-root)) + (_ (expand-file-name session-name project-root))))) (defun tabspaces--get-project-session-file-for-restore (project) "Get the session file path for PROJECT based on configuration." (let* ((project-name (file-name-nondirectory (directory-file-name project))) (session-name (concat "." project-name "-tabspaces-session.el"))) + ;; if you like the pcase example above, you can use it here as well (cond ((eq tabspaces-session-project-session-store 'project) (expand-file-name session-name project)) @@ -1284,6 +1283,12 @@ dedup preserves workspace isolation when the same buffer name lives in multiple tabs." (let ((existing (get-buffer name))) (and existing + ;; unless I made a mistake, since `tabspaces--buffer-list' + ;; never contains nil (it is filtered to live buffers and nil + ;; is not a live buffer), you can simplify the entire + ;; function to just: + ;; + ;; (car (memq (get-buffer name) (tabspaces--buffer-list))) (memq existing (tabspaces--buffer-list)) existing))) @@ -1340,15 +1345,11 @@ otherwise evaluated." ;; variables and hang or crash later traversals of them. (let ((read-circle nil)) (while t - (let ((form (read (current-buffer)))) - (if (and (eq (car-safe form) 'setq) - (= (length form) 3) - (memq (nth 1 form) - '(tabspaces-project-tab-map tabspaces--session-list)) - (eq (car-safe (nth 2 form)) 'quote)) - (set (nth 1 form) (cadr (nth 2 form))) - (message "tabspaces: ignoring unexpected form in %s: %S" - file (car-safe form)))))) + (pcase (read (current-buffer)) + (`(setq ,(and (or 'tabspaces-project-tab-map 'tabspaces--session-list) var) ',val) + (set var val)) + (form (message "tabspaces: ignoring unexpected form in %s: %S" + file form))))) (end-of-file nil) (error (message "tabspaces: unreadable session file %s: %S" file err))))) --=-=-= Content-Type: text/plain Overall the code quality is good, I just suggested some minor formatting points in the package header, dropping the :group declarations since they default to the last defgroup and using pcase in a few places, if you like that macro. And just to be sure, is there a reason you are requesting to add the package to NonGNU rather than GNU ELPA? It is totally fine if that is so, I just want to make sure this is not due to some misconception. > Tabspaces leverages the built-in tab-bar and project libraries to > provide buffer-isolated workspaces, one per tab. It integrates with > project.el for project-based workspaces, filters buffer lists per > tab, and supports saving and restoring workspace sessions. It > requires Emacs 27.1 or later. > > The package is GPL-3+ and has been on MELPA since 2022. I am the > author and maintainer, and I'm happy to follow the NonGNU ELPA > conventions for releases (version bumps in the Version header). > > For the recipe, something like: > > (tabspaces :url "https://codeberg.org/mclear-tools/tabspaces") > > Best, > Colin > > -- > Colin McLear > colinmclear.net --=-=-=--