Re: Adding new package org-menu to nongnu elpa

[Philip Kaludercic]

Jan Rehders <jan@sheijk.net> writes:

> Hello,

Hi,

> I’d like to offer my package org-menu to be included in nongnu elpa. I 
> believe it has enough users to warrant making it easier to install

Popularity is no precondition for a package to be added to NonGNU ELPA.
We can add the package, just consider the following comments and
suggestions:

diff --git a/org-menu.el b/org-menu.el
index df82c18..f4f5066 100644
--- a/org-menu.el
+++ b/org-menu.el
@@ -1,6 +1,6 @@
-;;; org-menu.el --- A discoverable menu for org-mode using transient -*- 
lexical-binding: t; coding: utf-8 -*-
+;;; org-menu.el --- A discoverable menu for org-mode using transient -*- 
lexical-binding: t -*-
 ;;
-;; Copyright 2021 Jan Rehders
+;; Copyright 2021  Jan Rehders
 ;;
 ;; Author: Jan Rehders <nospam@sheijk.net>
 ;; Version: 0.1alpha
@@ -23,19 +23,19 @@
 ;; Boston, MA 02111-1307, USA.
 ;;
 ;;; Commentary:
-;;
-;; Usage:
-;;
+
+;;;; Usage:
+
 ;; Add this to your ~/.emacs to bind the menu to `C-c m':
 ;;
 ;; (with-eval-after-load 'org
 ;;   (require 'org-menu) ;; not needed if installing by package manager
-;;   (define-key org-mode-map (kbd "C-c m") 'org-menu))
+;;   (define-key org-mode-map (kbd "C-c m") #'org-menu))
 ;;
 ;; The menu should be pretty self-explanatory.  It is context dependent and
 ;; offers different commands for headlines, tables, timestamps, etc.
 ;; The task menu provides entry points for task that work from anywhere.
-;;
+
 ;;; Code:
 
 (require 'org)
@@ -52,27 +52,23 @@
 
 Use this if you prefer to be consistent with magit.  It will also
 change some other bindings to use Q instead of q."
-  :group 'org-menu
   :type 'boolean)
 
 (defcustom org-menu-global-toc-depth 10
   "The number of heading levels to show when displaying the global content."
-  :group 'org-menu
-  :type 'integer)
+  :type 'natnum)
 
-(defcustom org-menu-expand-snippet-function 'org-menu-expand-snippet-default
+(defcustom org-menu-expand-snippet-function #'org-menu-expand-snippet-default
   "The function used to expand a snippet.
 
 See `org-menu-expand-snippet-default' for a list of snippet ids
 which need to be supported.  `org-menu-expand-snippet-yasnippet'
 shows how to invoke snippets."
-  :group 'org-menu
   :type 'function)
 
 (defun org-menu-show-columns-view-options-p ()
   "Return whether `org-columns' mode is active."
-  (and (boundp 'org-columns-overlays)
-       (not (null org-columns-overlays))))
+  (bound-and-true-p org-columns-overlays))
 
 (defun org-menu-show-heading-options-p ()
   "Whether to show commands operating on headings."
@@ -109,8 +105,7 @@ Conditions have been adapted from `org-insert-link'"
   (unless (org-menu-show-columns-view-options-p)
     (or
      ;; Use variable from org-compat to support Emacs 26
-     ;; this produces a warning in newer Emacs which we can't avoid
-     (org-in-regexp org-bracket-link-regexp 1)
+     (org-in-regexp (symbol-value 'org-bracket-link-regexp) 1)
      (when (boundp 'org-link-angle-re)
        (org-in-regexp org-link-angle-re))
      (when (boundp 'org-link-plain-re)
@@ -135,7 +130,7 @@ true the items will only be added if on a heading.  
`CYCLE-FUNCTION' is the
 function to be used to cycle visibility of current element."
   (setq cycle-function (or cycle-function #'org-cycle))
   `(["Navigate"
-     ,@(when check-for-heading '(:if org-menu-show-heading-options-p))
+     ,@(and check-for-heading '(:if org-menu-show-heading-options-p))
      ("p" "prev" org-previous-visible-heading :transient t)
      ("n" "next" org-next-visible-heading :transient t)
      ("c" "cycle" ,cycle-function :transient t)
@@ -164,30 +159,30 @@ function to be used to cycle visibility of current 
element."
 #+attr_org: :width 400px
 [[file:plot.svg]]
 "))
-    (_ (insert (format "unknown snippet type %s" snippet-id)))))
+    (_ (insert (format "unknown snippet type %s" snippet-id))))) ;or use 
`pcase-exhaustive'?
 
-(autoload 'yas-expand-snippet "yasnippet")
-(autoload 'yas-expand-from-trigger-key "yasnippet")
+;; You are require'ing yasnippet anyway, so there is no need to mess with 
autoloads
+(declare-function yas-expand-snippet "yasnippet" (snippet &optional start end 
expand-env))
+(declare-function yas-expand-from-trigger-key "yasnippet" (&optional field))
 
 (defun org-menu-expand-snippet-yasnippet (snippet-id)
   "Expand a yasnippet for each `SNIPPET-ID'."
-  (if (not (require 'yasnippet nil 'noerror))
-      (message "error: yasnippet not installed, could not expand %s" 
snippet-id)
-
-    (pcase snippet-id
-      ('block
-          (insert "beg")
-        (yas-expand-from-trigger-key))
-      ('option
-       (insert "opt")
-       (yas-expand-from-trigger-key))
-      ('subscript
-       (yas-expand-snippet "${1:text}_{${2:sub}}"))
-      ('superscript
-       (yas-expand-snippet "${1:text}^{${2:super}}"))
-      ('plot
-       (yas-expand-snippet
-        "#+plot: type:${1:2d} file:\"${2:plot.svg}\"
+  (unless (require 'yasnippet nil 'noerror)
+    (error "Yasnippet not installed, could not expand %s" snippet-id))
+  (pcase snippet-id
+    ('block
+     (insert "beg")
+     (yas-expand-from-trigger-key))
+    ('option
+     (insert "opt")
+     (yas-expand-from-trigger-key))
+    ('subscript
+     (yas-expand-snippet "${1:text}_{${2:sub}}"))
+    ('superscript
+     (yas-expand-snippet "${1:text}^{${2:super}}"))
+    ('plot
+     (yas-expand-snippet
+      "#+plot: type:${1:2d} file:\"${2:plot.svg}\"
 | A |  B |
 |---+----|
 | 1 | 10 |
@@ -197,8 +192,8 @@ function to be used to cycle visibility of current element."
 #+attr_org: :width ${3:400px}
 [[file:$2]]
 "))
-      (_
-       (insert (format "unknown snippet type %s" snippet-id))))))
+    (_
+     (insert (format "unknown snippet type %s" snippet-id)))))
 
 ;; If yasnippet gets loaded it will be used automatically
 (with-eval-after-load 'yasnippet
@@ -214,7 +209,7 @@ function to be used to cycle visibility of current element."
   (interactive)
   (save-excursion
     (outline-hide-subtree)
-    (org-show-children 4)
+    (org-show-children 4)              ;there is an obsoletion warning here!
     (org-goto-first-child)
     (org-reveal '(4))))
 
@@ -541,7 +536,6 @@ Named `NAME' with `DEFINITION'."
 If region is active it will be surrounded by `LEFT' and `RIGHT' and
 the point will be at end of region.  Will add spaces before/after text if
 `SURROUND-WHITESPACE' is true and it's needed."
-
   (let ((start (point))
         (end (point)))
     (when (region-active-p)
@@ -549,7 +543,7 @@ the point will be at end of region.  Will add spaces 
before/after text if
             end (region-end))
       (deactivate-mark))
     (when (> start end)
-      ;; swap variables w/o importing cl-lib
+      ;; swap variables w/o importing cl-lib (this wouldn't be that much of an 
issue, as org already uses cl-lib...)
       (setq start (prog1 end (setq end start))))
 
     (goto-char start)
@@ -582,14 +576,17 @@ the point will be at end of region.  Will add spaces 
before/after text if
 
 (defun org-menu-toggle-nbspace ()
   "Will remove non-breaking space before/after point or insert it if none 
found."
+  ;;           ^
+  ;;           Non-breaking or zero-width space?  \u200b is zero-width (used 
below)
   (interactive)
-  (cond
-   ((looking-back "​")
-    (backward-delete-char 1))
-   ((looking-at "​")
-    (delete-char 1))
-   (t
-    (insert "\u200b"))))
+  (let ((zww (eval-when-compile (string (char-from-name "ZERO WIDTH SPACE")))))
+    (save-excursion
+      ;; By moving back first, we will avoid just removing the space
+      ;; before /or/ after as handled by the cond expression.
+      (skip-chars-backward zww)
+      (if (looking-at (rx (+ (literal zww))))
+         (replace-match "")
+       (insert zww)))))
 
 (defun org-menu-text-format-items (check-for-table)
   "Items to format text.
@@ -704,12 +701,13 @@ Will add an ':if org-menu-show-text-options-p' criteria if
   (interactive)
   (org-columns t))
 
+;; Can you explain why you are copying code org-colview.el?  It is not
+;; clear to me why you don't try to call it directly.
+(declare-function org-agenda-do-context-action ())
 (defun org-menu-columns-next ()
   "Move into the next row when org-columns is active.
 
-Code copied from lambda in org-colview.el after
-  (org-defkey org-columns-map [down]
-"
+Code copied from lambda in org-colview.el from `org-columns-map'."
   (interactive)
   (let ((col (current-column)))
     (beginning-of-line 2)
@@ -805,7 +803,7 @@ Code copied from lambda in org-colview.el after
 
 (transient-insert-suffix 'org-menu-archive (list 0)
   `["Archive"
-    ,@(org-menu-heading-navigate-items nil #'org-force-cycle-archived)
+    ,@(org-menu-heading-navigate-items nil #'org-force-cycle-archived) 
;obsoletion warning!
     ["Archive to"
      ("t" "tree" org-archive-subtree :transient t)
      ("s" "sibling" org-archive-to-archive-sibling :transient t)

-- 
        Philip Kaludercic on siskin