syl20bnr / spacemacs

A community-driven Emacs distribution - The best editor is neither Emacs nor Vim, it's Emacs *and* Vim!
http://spacemacs.org
GNU General Public License v3.0
23.58k stars 4.9k forks source link

dired-x is not loaded as needed #16284

Closed xuan-w closed 3 months ago

xuan-w commented 4 months ago

Description :octocat:

dired-x is not loaded as needed.

Reproduction guide :beetle:

Observed behaviour: :eyes: :broken_heart:

dired-x is not activated. Command "dired-mark-extension" and the corresponding keyboard shortcut "* ." is not available.

Expected behaviour: :heart: :smile:

Command "dired-mark-extension" shall be available on M-x and the corresponding keyboard shortcut "* ." can be used to evoke dired-mark-extension.

The below function in layers/+spacemacs/spacemacs-defaults/packages.el could be related to this issue?

(defun spacemacs-defaults/init-dired-x ()
  (use-package dired-x
    :commands (dired-jump
               dired-jump-other-window
               dired-omit-mode)))

System Info :computer:

Backtrace :paw_prints:

emacs18 commented 4 months ago

I don't think this is related with spacemacs. It is just a quirk of emacs itself.

dired-mark-extension is not autoloaded which means you cannot call it unless you have already done (require 'dired-x) or something similar.

Also * . key binding seems to be added after you load dired-x.el by the following code found within dired-x.el:

(when (keymapp (lookup-key dired-mode-map "*"))
  (define-key dired-mode-map "*(" 'dired-mark-sexp)
  (define-key dired-mode-map "*O" 'dired-mark-omitted)
  (define-key dired-mode-map "*." 'dired-mark-extension))

Hence the current design of emacs is that you need to load dired-x yourself first if you want * . key binding to be available.

This has nothing to do with spacemacs. Spacemacs does not mention dired-mark-extension at all.

If you want * . key binding to be valid when you start emacs, then you simply need to add (require 'dired-x) within your startup file.

Having said that dired-jump and dired-jump-other-window are defined in dired.el rather than in dired-x.el. Hence they should not be associated with (use-package dired-x ...) expression. This I think should be fixed within spacemacs.

xuan-w commented 4 months ago

Ahh, I see, thank you for explaining that to me.

I traced back the changes and it seems that dired-jump-other-window was in dired-x but merged into dired.el later. So, with an older version of Emacs, dired-x is indeed loaded whenever dired launches (at least, when they launch dired with "SPC j d", which is the fastest way? )

If someone used an old version of Emacs with Spacemacs, after upgrading Emacs, they would notice some behavior difference.

(In my case, when I read my old cheatsheet of dired I fount I cannot use "* ." any more. )

I think the question is more about "Do we want Spacemacs' behavior to be consistent across different version of Emacs and depending packages?" A user who upgrades Emacs might find losing "* ." a surprise, though this key binding was not introduced by Spacemacs.

Richard Kim @.***> writes:

I don't think this is related with spacemacs. It is just a quirk of emacs itself.

dired-mark-extention is not autoloaded which means you cannot call it unless you have already done (require 'dired-x) or something similar.

Also * . key binding seems to be added after you load dired-x.el by the following code found within dired-x.el:

(when (keymapp (lookup-key dired-mode-map "")) (define-key dired-mode-map "(" 'dired-mark-sexp) (define-key dired-mode-map "O" 'dired-mark-omitted) (define-key dired-mode-map "." 'dired-mark-extension))

Hence the current design of emacs is that you need to load dired-x yourself first if you want * . key binding to be available.

This has nothing to do with spacemacs. Spacemacs does not mention dired-mark-extension at all.

If you want * . key binding to be valid when you start emacs, then you simply need to add (require 'dired-x) within your startup file.

Having said that dired-jump and dired-jump-other-window are defined in dired.el rather than in dired-x.el. Hence they should not be associated with (use-package dired-x ...) expression. This I think should be fixed within spacemacs.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.

emacs18 commented 4 months ago

Xuan,

Following patch does what you want, i.e., if dired.el is loaded, then dired-x.el is also loaded. This also seems to be the way the official documentation of dired-x says to set it up.

Perhaps you could submit PR with this change?

diff --git a/layers/+spacemacs/spacemacs-defaults/packages.el b/layers/+spacemacs/spacemacs-defaults/packages.el
index d8cbc2a87..5891e505a 100644
--- a/layers/+spacemacs/spacemacs-defaults/packages.el
+++ b/layers/+spacemacs/spacemacs-defaults/packages.el
@@ -150,6 +150,8 @@
     (evil-define-key 'normal dired-mode-map (kbd "N") 'evil-search-previous)))

 (defun spacemacs-defaults/init-dired-x ()
+  (with-eval-after-load 'dired
+    (require 'dired-x))
   (use-package dired-x
     :straight nil
     :commands (dired-jump
emacs18 commented 4 months ago

Same change as before, but with comment to explain why autoloading is not used.

diff --git a/layers/+spacemacs/spacemacs-defaults/packages.el b/layers/+spacemacs/spacemacs-defaults/packages.el
index d8cbc2a87..800d4a86e 100644
--- a/layers/+spacemacs/spacemacs-defaults/packages.el
+++ b/layers/+spacemacs/spacemacs-defaults/packages.el
@@ -150,6 +150,10 @@
     (evil-define-key 'normal dired-mode-map (kbd "N") 'evil-search-previous)))

 (defun spacemacs-defaults/init-dired-x ()
+  ;; Load dired-x rather than depending on autoloading, because dired-x.el adds
+  ;; additional key bindings such as "* ." to dired mode.
+  (with-eval-after-load 'dired
+    (require 'dired-x))
   (use-package dired-x
     :straight nil
     :commands (dired-jump
xuan-w commented 4 months ago

Thank you for your kind help and guidance!

I am drafting the PR and digging around to see if we can safely remove the original auto loading with "dired-jump" if we want to be compatible to older version Emacs.

Once I found out, I will submit the PR.