Philip Kaludercic writes: > Akib Azmain Turja writes: > >> Philip Kaludercic writes: >> >>>>> OK, then adding them to NonGNU ELPA seems like the safer bet. >>>>> >>>>> I'd like to add them, but I'll have to take the time to review them >>>>> first, which might take a bit. >>>> >>>> What do you want to review? The patches, or the packages? >>> >>> The packages, unless you aren't interested in comments. >> >> Review if you wish, I welcome feedback. > > I've managed to skim through workroom.el. The code looks great, so I > just have a non-comprehensive list of comments and ideas: Really? It was one of my worst packages until I almost rewrote it over the last few weeks. > > diff --git a/workroom.el b/workroom.el > index 7091645b85..8bb564e495 100644 > --- a/workroom.el > +++ b/workroom.el > @@ -217,12 +217,13 @@ The value is a mode line terminal like `mode-line-format'." > keymap) > "Keymap containing all useful commands of Workroom.") > > -(defvar workroom-mode-map (make-sparse-keymap) > +(defvar workroom-mode-map > + (let ((map (make-sparse-keymap))) > + (define-key map workroom-command-map-prefix > + workroom-command-map) > + map) > "Keymap for Workroom mode.") > > -(define-key workroom-mode-map workroom-command-map-prefix > - workroom-command-map) > - I just removed that define-key completely. `workroom-mode` will bind it when enabled. > (defun workroom-rebind-command-map-prefix () > "Rebind command prefix key sequence `workroom-command-map-prefix'." > (substitute-key-definition > @@ -234,6 +235,7 @@ The value is a mode line terminal like `mode-line-format'." > ;;;; Workroom and View Manipulation. > > (cl-defstruct (workroom--room > + (:predicate workroomp) > (:constructor workroom--make-room) > (:copier workroom--copy-room)) > "Structure for workroom." > @@ -253,6 +255,7 @@ The value is a mode line terminal like `mode-line-format'." > :documentation "`completing-read' history of view names.")) > > (cl-defstruct (workroom--view > + (:predicate workroom-view-p) > (:constructor workroom--make-view) > (:copier workroom--copy-view)) > "Structure for view of workroom." > @@ -268,7 +271,7 @@ The value is a mode line terminal like `mode-line-format'." > (defvar workroom--dont-clear-new-view nil > "Non-nil mean don't clear empty new views.") They don't get a docstring on my machine. :( > > -(defvar workroom--rooms nil > +(defvar workroom--rooms nil ;maybe some comments on the structure > "List of currently live workrooms.") As the docstring describes, it's a list, and all elements satisfies `workroomp'. > > (defvar workroom-room-history nil > @@ -283,10 +286,6 @@ that.") > > (defvar workroom-mode) > > -(defun workroomp (object) > - "Return non-nil if OBJECT is a workroom object." > - (workroom--room-p object)) > - > (defun workroom-name (room) > "Return the name of workroom ROOM." > (workroom--room-name room)) > @@ -384,10 +383,6 @@ effect, it is not unaltered." > "Completing read history of view of workroom ROOM." > (workroom--room-view-history room)) > > -(defun workroom-view-p (object) > - "Return non-nil if OBJECT is a view object." > - (workroom--view-p object)) > - I just wanted to give it a better docstring. ;) > (defun workroom-view-name (view) > "Return the name of view VIEW." > (workroom--view-name view)) > @@ -435,10 +430,9 @@ A copy is returned, so it can be modified with side-effects." > "Return the workroom named NAME. > > If no such workroom exists, return nil." > - (catch 'found > - (dolist (room workroom--rooms nil) > - (when (string= name (workroom-name room)) > - (throw 'found room))))) > + (cl-find name workroom--rooms > + :key #'workroom-name > + :test #'string=)) Done, also changed workroom-view-get. > > (defun workroom-get-create (name) > "Return the workroom named NAME. > @@ -500,7 +494,7 @@ If no such view exists, create a new one named NAME and return that." > (unless view > (setq view (workroom--make-view :name name)) > (setf (workroom--room-view-list room) > - (nconc (workroom--room-view-list room) `(,view)))) > + (nconc (workroom--room-view-list room) (list view)))) > view)) Done. > > (defun workroom-generate-new-view-name (room name) > @@ -516,7 +510,7 @@ name." > (let ((n 2)) > (while t > (let ((str (format "%s<%i>" name n))) > - (when (not (workroom-view-get room str)) > + (unless (workroom-view-get room str) > (cl-return str)) > (cl-incf n))))))) I can't believe I wrote (when (not ...) ...)! > > @@ -558,7 +552,7 @@ Return DEF when input is empty, where DEF is either a string or nil. > > REQUIRE-MATCH and PREDICATE is same as in `completing-read'." > (completing-read > - (concat prompt (when def (format " (default %s)" def)) ": ") > + (concat prompt (and def (format " (default %s)" def)) ": ") ;Compat has `format-prompt' > (mapcar #'workroom-name workroom--rooms) predicate require-match > nil 'workroom-room-history def)) I don't have much idea about Compat, how does it work? > > @@ -670,7 +664,7 @@ If WRITABLE, return a writable object." > (defun workroom--load-window-config (state) > "Load window configuration STATE." > (if state > - (cl-labels > + (cl-labels ;perhaps this should be split up? > ((sanitize (entry) > (cond > ;; Do nothing. Yeah, it's too monolithic. > @@ -1254,7 +1248,7 @@ ACTION and ARGS are also described there." > (setf (workroom-buffer-manager-data room) > (cl-delete-if-not #'buffer-live-p > (workroom-buffer-manager-data room))) > - (pcase action > + (pcase action ;perhaps match on (cons action args)? > (:initialize > (cl-destructuring-bind () args > (setf (workroom-buffer-manager-data room) I wonder why I used cl-destructuring-bind when there isn't any keyword arguments. Fixing... Fixing...done > @@ -1507,9 +1501,14 @@ restrict." > > (defun workroom--encode-view-1 (view) > "Encode view VIEW to a writable object." > - `( :name ,(workroom-view-name view) > - :window-config ,(workroom-view-window-configuration > - view 'writable))) > + ;; I think it is preferable to define plists with keywords using > + ;; `list', as all the members of the list can be evaluated during > + ;; application. > + (list :name > + (workroom-view-name view) > + :window-config > + (workroom-view-window-configuration > + view 'writable))) > Done. Maybe I did too much. > (defun workroom--decode-view-1 (object) > "Decode encoded view OBJECT to a view." > @@ -1589,7 +1588,7 @@ when ROOM was encoded." > > (defun workroom-decode-buffer-bookmark (object) > "Decode OBJECT using `bookmark-jump'." > - (let* ((buffer nil)) > + (let ((buffer nil)) > (bookmark-jump object (lambda (buf) (setq buffer buf))) > buffer)) > > @@ -1664,7 +1663,7 @@ any previous bookmark with the same name." > bookmark))))) > (pcase (plist-get data :version) > (1 > - (let* ((buffers (cl-delete-if > + (let ((buffers (cl-delete-if > #'null > (workroom--decode-buffers > (plist-get data :buffers))))) Ah, I wasted two bytes for each copy of Workroom worldwide. :( Fixed. > @@ -1787,6 +1786,7 @@ any previous bookmark with the same name." > ;; Inject restoring code. > (when workroom-mode > (let ((time (format-time-string "%s%N"))) > + ;; I like to use `prin1-to-string' for things like these. > (insert > (format > " Done, and I got syntax highlighting as a bonus! > @@ -1951,6 +1951,7 @@ argument while setting as the buffer manager, PROJECT, the project." > (defun workroom--project-name (project) > "Return a name for project PROJECT." > (let ((root (project-root project))) > + ;; Isn't this `file-name-base' or `directory-file-name'? > (if (string-match "/\\([^/]+\\)/?\\'" root) > (match-string 1 root) > root))) > Yeah, my bad, it's actually `file-name-base'. I threw the function away. -- Akib Azmain Turja, GPG key: 70018CE5819F17A3BBA666AFE74F0EFA922AE7F5 Fediverse: akib@hostux.social Codeberg: akib emailselfdefense.fsf.org | "Nothing can be secure without encryption."