Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

counsel.el (counsel-linux-apps-directories): Add XDG defaults. #1851

Closed
wants to merge 1 commit into from

Conversation

@jellelicht
Copy link

commented Dec 9, 2018

This PR has several goals;

  • Make counsel-linux-app "Just Work" by default in more settings
  • Make resolving *.desktop files work similar to how other tools do it.
  • Make maintenance of a sane default easier going forward (e.g. no adding paths to counsel-linux-apps-directories like what happened for flatpack.)

CHANGES

  • counsel.el (counsel-linux-apps-directories): Use XDG_DATA_DIRS
    environment variable to determine directories to search.

@jellelicht jellelicht force-pushed the jellelicht:master branch from c581631 to 6c5ced7 Dec 9, 2018

@jellelicht

This comment has been minimized.

Copy link
Author

commented Dec 9, 2018

See https://wiki.archlinux.org/index.php/XDG_Base_Directory for the default.

Also, I made sure to use string= instead of string-empty-p, as this was only introduced in Emacs 24.4

counsel.el Outdated Show resolved Hide resolved
counsel.el Outdated Show resolved Hide resolved
counsel.el Outdated Show resolved Hide resolved
@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Dec 9, 2018

See https://wiki.archlinux.org/index.php/XDG_Base_Directory for the default.

So why aren't we also defaulting to ~/.local/share?

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Dec 9, 2018

Why not try to reuse the Emacs 26 built-in library xdg.el:

diff --git a/counsel.el b/counsel.el
index 4c1055f..808f76a 100644
--- a/counsel.el
+++ b/counsel.el
@@ -4655,13 +4655,21 @@ counsel-rhythmbox
             :caller 'counsel-rhythmbox))
 
 ;;** `counsel-linux-app'
+(defalias 'counsel--xdg-data-dirs
+  (if (and (require 'xdg nil t)
+           (fboundp 'xdg-data-dirs))
+      #'xdg-data-dirs
+    (lambda ()
+      (let ((path (getenv "XDG_DATA_DIRS")))
+        (if (and path (not (string= path "")))
+            (parse-colon-path path)
+          '("/usr/local/share/" "/usr/share/")))))
+  "Compatibility shim for `xdg-data-dirs'.")
+
 (defcustom counsel-linux-apps-directories
-  (let ((xdg-data-dirs (getenv "XDG_DATA_DIRS")))
-    (mapcar (lambda (dir) (concat dir "/applications"))
-            (split-string (if (and xdg-data-dirs (not (string= xdg-data-dirs "")))
-                              xdg-data-dirs
-                            "/usr/local/share:/usr/share")
-                          ":")))
+  (mapcar (lambda (dir)
+            (expand-file-name "applications" dir))
+          (counsel--xdg-data-dirs))
   "Directories in which to search for applications (.desktop files)."
   :group 'ivy
   :type '(repeat directory))

@jellelicht jellelicht force-pushed the jellelicht:master branch from 6c5ced7 to ad19590 Dec 9, 2018

@jellelicht

This comment has been minimized.

Copy link
Author

commented Dec 9, 2018

Hmm, your proposal for using 'xdg seems nice! I'll extend it to also take XDG_DATA_HOME into account

@jellelicht jellelicht force-pushed the jellelicht:master branch from ad19590 to 0a3cd8e Dec 9, 2018

@jellelicht

This comment has been minimized.

Copy link
Author

commented Dec 9, 2018

@basil-conto thanks for all of your pointers!

counsel.el Outdated
"Compatibility shim for `xdg-data-home'.")

(defalias 'counsel--xdg-data-dirs
(if (and (require 'xdg nil t)

This comment has been minimized.

Copy link
@basil-conto

basil-conto Dec 9, 2018

Collaborator

Either the call to require can be extracted to top-level:

(require 'xdg nil t)

(defalias 'counsel--xdg-data-home
  (if (fboundp 'xdg-data-home) ...))

(defalias 'counsel--xdg-data-dirs
  (if (fboundp 'xdg-data-dirs) ...))

or the two shims can be combined:

diff --git a/counsel.el b/counsel.el
index 559d34c..0d701fb 100644
--- a/counsel.el
+++ b/counsel.el
@@ -4655,32 +4655,29 @@ counsel-rhythmbox
             :caller 'counsel-rhythmbox))
 
 ;;** `counsel-linux-app'
-(defalias 'counsel--xdg-data-home
-  (if (and (require 'xdg nil t)
-           (fboundp 'xdg-data-home))
-      #'xdg-data-home
-    (lambda ()
-      (let ((directory (getenv "XDG_DATA_HOME")))
-        (if (and directory (not (string= directory "")))
-            directory
-          "~/.local/share"))))
-  "Compatibility shim for `xdg-data-home'.")
+(declare-function xdg-data-home "xdg")
+(declare-function xdg-data-dirs "xdg")
 
-(defalias 'counsel--xdg-data-dirs
-  (if (and (require 'xdg nil t)
-           (fboundp 'xdg-data-dirs))
-      #'xdg-data-dirs
+(defalias 'counsel--xdg-data-path
+  (if (require 'xdg nil t)
+      (lambda ()
+        (cons (xdg-data-home)
+              (xdg-data-dirs)))
     (lambda ()
-      (let ((path (getenv "XDG_DATA_DIRS")))
-        (if (and path (not (string= path "")))
-            (parse-colon-path path)
-          '("/usr/local/share/" "/usr/share/")))))
-  "Compatibility shim for `xdg-data-dirs'.")
+      (let ((home (getenv "XDG_DATA_HOME"))
+            (dirs (getenv "XDG_DATA_DIRS")))
+        (cons (if (and home (not (string= home "")))
+                  home
+                "~/.local/share")
+              (if (and dirs (not (string= dirs "")))
+                  (parse-colon-path dirs)
+                '("/usr/local/share/" "/usr/share/"))))))
+  "Return the XDG data directory search path as a list.
+This includes the base directory for user-specific data files.")
 
 (defcustom counsel-linux-apps-directories
   (mapcar (lambda (dir) (expand-file-name "applications" dir))
-          (cons (counsel--xdg-data-home)
-                (counsel--xdg-data-dirs)))
+          (counsel--xdg-data-path))
   "Directories in which to search for applications (.desktop files)."
   :group 'ivy
   :type '(repeat directory))

This comment has been minimized.

Copy link
@jellelicht

jellelicht Dec 9, 2018

Author

I went with the first approach, and took the function definitions from xdg.el

Jelle Licht
counsel.el (counsel-linux-apps-directories): Add XDG defaults.
* counsel.el (counsel-linux-apps-directories): Use XDG Base Directory
  Specification to determine directories to search.
(counsel--xdg-data-home): New compatibility shim.
(counsel--xdg-data-dirs): New compatibility shim.

@jellelicht jellelicht force-pushed the jellelicht:master branch from 0a3cd8e to 2413e7a Dec 9, 2018

@abo-abo abo-abo closed this in 1cc1a60 Dec 12, 2018

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Dec 12, 2018

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.