From d618bb4620d5d651027e772b8ccc490e1bab6d80 Mon Sep 17 00:00:00 2001 From: Craig Jennings Date: Sat, 16 May 2026 02:56:25 -0500 Subject: refactor(ui): four UI/navigation hygiene fixes from module-by-module re-review - popper-config.el: move `(popper-mode +1)` and `(popper-echo-mode +1)` from the use-package `:init` block into `:config`. `:disabled t' on use-package skips `:config' but still runs `:init', so the previous shape enabled popper-mode on every load, including batch / test runs, despite the disabled marker. - modeline-config.el: make `cj/modeline-vc-fetch' fall back when the internal `vc-git--symbolic-ref' is missing. `require' uses `nil 'noerror', the call sits inside an `fboundp' guard, and `ignore-errors' wraps the call itself so an Emacs version that renames or removes the accessor leaves `branch' at `vc-working-revision''s output instead of crashing the modeline. - ui-config.el: guard the cursor-color `post-command-hook' behind `(display-graphic-p)' both at install time and inside the function body. Batch / TTY runs short-circuit cleanly with no per-command overhead. A `server-after-make-frame-hook' catches the daemon case where the first GUI frame is created after ui-config loads and installs the hook lazily. Updates test-ui-config--buffer-cursor-state and test-ui-cursor-color-integration to stub `display-graphic-p' so the work body still runs under batch. - nerd-icons-config.el: drop `:demand t' (`:defer t' now), keeping the `:config' advice install as the natural lazy-on-load path. Add a `with-eval-after-load 'nerd-icons' block as a safety net for the already-loaded case on re-eval; the block uses `advice-member-p' so the advice never stacks. --- modules/modeline-config.el | 13 +++-- modules/nerd-icons-config.el | 12 ++++- modules/popper-config.el | 4 ++ modules/ui-config.el | 41 +++++++++----- tests/test-ui-config--buffer-cursor-state.el | 7 ++- tests/test-ui-cursor-color-integration.el | 11 ++++ todo.org | 80 +++++++++++++++------------- 7 files changed, 112 insertions(+), 56 deletions(-) diff --git a/modules/modeline-config.el b/modules/modeline-config.el index 771d960f..61ab0f9d 100644 --- a/modules/modeline-config.el +++ b/modules/modeline-config.el @@ -141,15 +141,20 @@ Uses built-in cached values for performance.") (defun cj/modeline-vc-fetch (file) "Fetch modeline VC data for FILE. -Return a plist with `:branch' and `:state', or nil when FILE has no VC data." +Return a plist with `:branch' and `:state', or nil when FILE has no VC data. +Uses `vc-git--symbolic-ref' for branch names when available (it returns the +symbolic ref like \"main\" instead of a SHA when HEAD is on a branch), but +falls back to `vc-working-revision' if the internal accessor is missing -- +the symbol is internal and can be renamed or removed between Emacs versions." (unless (and (file-remote-p file) (not cj/modeline-vc-show-remote)) (when-let* ((backend (vc-backend file)) (branch (vc-working-revision file backend))) (when (eq backend 'Git) (unless (fboundp 'vc-git--symbolic-ref) - (require 'vc-git)) - (when-let* ((symbolic (vc-git--symbolic-ref file))) - (setq branch symbolic))) + (require 'vc-git nil 'noerror)) + (when (fboundp 'vc-git--symbolic-ref) + (when-let* ((symbolic (ignore-errors (vc-git--symbolic-ref file)))) + (setq branch symbolic)))) (list :branch branch :state (vc-state file backend))))) diff --git a/modules/nerd-icons-config.el b/modules/nerd-icons-config.el index 4a8ce194..52a4627d 100644 --- a/modules/nerd-icons-config.el +++ b/modules/nerd-icons-config.el @@ -70,11 +70,21 @@ every call. The `memq' check skips when the face is already present." ;; ------------------------------- Packages ------------------------------------ (use-package nerd-icons - :demand t + :defer t :config (advice-add 'nerd-icons-icon-for-dir :filter-return #'cj/--nerd-icons-color-dir) (cj/nerd-icons-apply-tint)) +;; If nerd-icons is already loaded (e.g. when this module is re-evaluated +;; after a session in which a feature module already required it), the +;; `:config' block above won't fire again -- fall through to install the +;; advice and tint immediately. +(with-eval-after-load 'nerd-icons + (unless (advice-member-p #'cj/--nerd-icons-color-dir 'nerd-icons-icon-for-dir) + (advice-add 'nerd-icons-icon-for-dir + :filter-return #'cj/--nerd-icons-color-dir)) + (cj/nerd-icons-apply-tint)) + (use-package nerd-icons-completion :demand t :after (nerd-icons marginalia) diff --git a/modules/popper-config.el b/modules/popper-config.el index 359e789c..35780eb2 100644 --- a/modules/popper-config.el +++ b/modules/popper-config.el @@ -37,6 +37,10 @@ (side . bottom) (slot . 0) (window-height . 0.5))) ; Half the frame height + ;; Mode activation moves to :config so `:disabled t' actually + ;; disables it (`:init' runs even when `:disabled t', `:config' + ;; does not). + :config (popper-mode +1) (popper-echo-mode +1)) diff --git a/modules/ui-config.el b/modules/ui-config.el index 5d3a6643..39b86182 100644 --- a/modules/ui-config.el +++ b/modules/ui-config.el @@ -120,21 +120,36 @@ through to `read-only' and keeps the orange cursor." (defun cj/set-cursor-color-according-to-mode () "Change cursor color according to buffer state (modified, read-only, overwrite). -Only updates for real user buffers, not internal/temporary buffers." - ;; Only update cursor for real buffers (not internal ones like *temp*, *Echo Area*, etc.) - (unless (string-prefix-p " " (buffer-name)) ; Internal buffers start with space - (let ((color (alist-get (cj/--buffer-cursor-state) cj/buffer-status-colors))) - ;; Only skip if BOTH color AND buffer are the same (optimization) - ;; This allows color to update when buffer state changes - (unless (and (string= color cj/-cursor-last-color) - (string= (buffer-name) cj/-cursor-last-buffer)) - (set-cursor-color color) - (setq cj/-cursor-last-color color - cj/-cursor-last-buffer (buffer-name)))))) +Only updates for real user buffers, not internal/temporary buffers. +A no-op on non-graphical frames -- TTY/batch sessions have no cursor color +to set." + (when (display-graphic-p) + ;; Only update cursor for real buffers (not internal ones like *temp*, *Echo Area*, etc.) + (unless (string-prefix-p " " (buffer-name)) ; Internal buffers start with space + (let ((color (alist-get (cj/--buffer-cursor-state) cj/buffer-status-colors))) + ;; Only skip if BOTH color AND buffer are the same (optimization) + ;; This allows color to update when buffer state changes + (unless (and (string= color cj/-cursor-last-color) + (string= (buffer-name) cj/-cursor-last-buffer)) + (set-cursor-color color) + (setq cj/-cursor-last-color color + cj/-cursor-last-buffer (buffer-name))))))) ;; Use post-command-hook to update cursor color after every command -;; This ensures cursor color always matches the current buffer's state -(add-hook 'post-command-hook #'cj/set-cursor-color-according-to-mode) +;; This ensures cursor color always matches the current buffer's state. +;; The hook only registers under a graphical session so batch / TTY runs +;; don't pay per-command overhead for a no-op. +(when (display-graphic-p) + (add-hook 'post-command-hook #'cj/set-cursor-color-according-to-mode)) +;; Daemon mode: the first frame may be created after this module loads. +;; Re-attempt the hook install once a GUI frame appears. +(add-hook 'server-after-make-frame-hook + (lambda () + (when (and (display-graphic-p) + (not (memq #'cj/set-cursor-color-according-to-mode + post-command-hook))) + (add-hook 'post-command-hook + #'cj/set-cursor-color-according-to-mode)))) ;; Don’t show a cursor in non-selected windows: (setq cursor-in-non-selected-windows nil) diff --git a/tests/test-ui-config--buffer-cursor-state.el b/tests/test-ui-config--buffer-cursor-state.el index add0d030..ead05741 100644 --- a/tests/test-ui-config--buffer-cursor-state.el +++ b/tests/test-ui-config--buffer-cursor-state.el @@ -72,7 +72,9 @@ buffer the user navigates, so `read-only' (orange) is kept." (ert-deftest test-ui-config-set-cursor-color-live-vterm-not-orange () "Normal: in a live vterm the cursor-color hook picks a writeable color, -not the read-only orange -- even though the vterm buffer is read-only." +not the read-only orange -- even though the vterm buffer is read-only. +`display-graphic-p' is stubbed t so the function reaches its work body +in batch mode (the live function no-ops on TTY frames by design)." (let ((buf (cj/test--make-fake-vterm-buffer "*test-vterm-cursor-color*")) (applied 'unset)) (unwind-protect @@ -81,7 +83,8 @@ not the read-only orange -- even though the vterm buffer is read-only." (setq-local vterm-copy-mode nil) (let ((cj/-cursor-last-color nil) (cj/-cursor-last-buffer nil)) - (cl-letf (((symbol-function 'set-cursor-color) + (cl-letf (((symbol-function 'display-graphic-p) (lambda () t)) + ((symbol-function 'set-cursor-color) (lambda (c) (setq applied c)))) (cj/set-cursor-color-according-to-mode))) (should (stringp applied)) diff --git a/tests/test-ui-cursor-color-integration.el b/tests/test-ui-cursor-color-integration.el index 00b7f57b..c28bde92 100644 --- a/tests/test-ui-cursor-color-integration.el +++ b/tests/test-ui-cursor-color-integration.el @@ -9,6 +9,17 @@ (require 'ert) (require 'user-constants) + +;; `cj/set-cursor-color-according-to-mode' and the `post-command-hook' +;; install both gate on `display-graphic-p' -- a TTY / batch run is a +;; no-op for cursor coloring by design. These integration tests +;; exercise the work body, so we pretend we're in a graphical session +;; for the whole file. Stubbing the symbol BEFORE loading ui-config +;; matters because the hook install reads `display-graphic-p' at load +;; time. +(advice-add 'display-graphic-p :around + (lambda (orig &rest args) (or (apply orig args) t))) + (require 'ui-config) ;;; Hook Integration Tests diff --git a/todo.org b/todo.org index 017b1584..3e219489 100644 --- a/todo.org +++ b/todo.org @@ -1334,25 +1334,24 @@ Expected outcome: This is low priority, but it is a good example of load graph noise to clean up during the =init.el= deferral work. -**** TODO [#B] Move =popper-mode= activation out of =:init= so =:disabled t= actually disables it :bug: - -=modules/popper-config.el:40= activates =(popper-mode +1)= inside the -=use-package= =:init= block. =use-package='s =:disabled t= keyword -prevents =:config= from running but =:init= forms run unconditionally. -The =:disabled= marker is therefore a no-op: popper-mode is enabled -on every load, including batch / test runs. Move the activation into -=:config= so disabling actually disables it. - -**** TODO [#C] Gracefully fallback in =cj/modeline-vc-fetch= when =vc-git--symbolic-ref= is missing :safety: - -=modules/modeline-config.el:149-151= calls the internal -=vc-git--symbolic-ref= without a fallback if Emacs's =vc-git= -implementation changes. Internal functions can be renamed or -removed between Emacs versions. When that happens, the modeline -errors during render, which blocks =mode-line-format= and produces a -confusing failure. Add an =fboundp= guard and fall back to -=(vc-working-revision file backend)= when the internal accessor is -unavailable. +**** 2026-05-16 Sat @ 02:55:14 -0500 Moved popper-mode activation from :init to :config + +=popper-mode +1= and =popper-echo-mode +1= now live in the +=:config= block of =modules/popper-config.el='s use-package form. +=:disabled t= now actually disables the mode (=:config= is skipped +when disabled; =:init= still runs but it only sets the reference- +buffer list and the display-buffer-alist entry, both of which are +harmless no-ops when popper itself never loads). Comment in the +module explains the split. + +**** 2026-05-16 Sat @ 02:55:14 -0500 Made cj/modeline-vc-fetch fall back when vc-git--symbolic-ref is missing + +The =require 'vc-git= now uses =nil 'noerror=, and the call to +=vc-git--symbolic-ref= is gated on =(fboundp ...)= so an Emacs +version that renames or removes the internal accessor just leaves +=branch= at =vc-working-revision='s output instead of crashing the +modeline render. Added =ignore-errors= around the call too in case +the internal accessor signals on unusual inputs. **** TODO [#C] Use theme-aware faces in =cj/display-available-fonts= :refactor: @@ -1402,23 +1401,32 @@ isearch into consult-line. Move the =C-s= binding into the consult =:bind= block under =:map isearch-mode-map= to preserve isearch's in-mode contract. -**** TODO [#C] Guard cursor-color =post-command-hook= behind =display-graphic-p= :safety: - -=modules/ui-config.el:125,137= registers =cj/set-cursor-color-according-to-mode= -on =post-command-hook= unconditionally. In batch / test runs and TTY -sessions, the hook fires on every command but the cursor color logic -is meaningless. Guard the =add-hook= behind -=(display-graphic-p)= or move it inside =server-after-make-frame-hook= -so it activates only for GUI frames. - -**** TODO [#C] Defer =nerd-icons-config= advice with =with-eval-after-load= :refactor: - -=modules/nerd-icons-config.el:73-75= uses =:demand t= and applies -icon-color advice via =dolist= at module load. This makes -nerd-icons eagerly load on every Emacs start (not just when an icon -is rendered). Wrap the advice block in -=(with-eval-after-load 'nerd-icons ...)= so the advice still applies -but nerd-icons can stay deferred for batch and headless contexts. +**** 2026-05-16 Sat @ 02:55:14 -0500 Guarded cursor-color hook behind display-graphic-p (with daemon-mode catch) + +Both the install (=add-hook= on =post-command-hook=) and the function +body now gate on =(display-graphic-p)=. Batch and TTY runs short- +circuit cleanly: no per-command overhead, no =set-cursor-color= calls +on frames that don't have a cursor color. A =server-after-make-frame-hook= +catches the daemon case where the first GUI frame is created after +=ui-config= loads -- it installs the hook lazily the first time a +GUI frame appears. + +The two cursor-color test files +(=test-ui-config--buffer-cursor-state.el=, +=test-ui-cursor-color-integration.el=) stub =display-graphic-p= to +return t so the work body still runs in batch. + +**** 2026-05-16 Sat @ 02:55:14 -0500 Deferred nerd-icons by dropping :demand t plus an after-load safety net + +=(use-package nerd-icons :demand t ...)= flipped to =:defer t=. The +=:config= block already wraps the advice + tint in lazy-on-load +semantics, so the advice now installs the first time nerd-icons +loads (typically when a feature module like =dashboard-icon-type= +or =dirvish-attributes= triggers a load). An additional +=(with-eval-after-load 'nerd-icons ...)= block at module bottom +catches the "already-loaded when this module re-evaluates" case -- +it checks =advice-member-p= so it doesn't stack the advice on every +re-eval. *** DOING [#B] Harden Org workflow modules :harden: -- cgit v1.2.3