Proposal to add Popper to ELPA

Proposal to add Popper to ELPA

[Karthik Chikmagalur]

I would like to add my package Popper to GNU ELPA.

URL: https://github.com/karthink/popper

What is Popper?

Short for "Popup Buffer", it's a global minor mode that designates
buffers as "popups", which means their display can be toggled with a
single key-press.  This is useful for quickly displaying and dismissing
"auxiliary" buffers like shells, compilation, help/man, grep buffers
etc.  If you have used the guake/yakuake pull-down terminals on GNOME or
KDE, the experience should be similar.

- You can designate a buffer as a "popup" manually or automatically
  ahead of time via name/major-mode or any predicate of your choosing.

- You can cycle through your popups with one key.

- You can group your popups by any predicate: for example, by project so
  that you only see auxiliary buffers relevant to your current project.

There are other features, and a few video demos at the link.

There are a few other packages that do this kind of thing, but they are
one or more of:

- Unmaintained (Popwin).

- Too limited in scope.  For example, the shellpop and vterm-toggle
packages allows this kind of behavior but only for shells.  Popper is
generic.

- Too rigid/overarching in their display behavior.  In contrast, Popper
does not try to control how popups are displayed: your
display-buffer-alist rules are respected, so that it composes well with
your other packages or customizations involving buffer display.

This package is now 4+ years old and (I think) in a stable state.  It
has been installed 16,000+ times.

Copyright assignment: I have signed the FSF copyright assignment papers.
Popper has several other contributors, but each of their contributions
is limited to 1-2 lines, usually fixing a linting issue or typo.

Please let me know if need more information or have any concerns.

Karthik

Re: Proposal to add Popper to ELPA

[Philip Kaludercic]

[-- Attachment #1: Type: text/plain, Size: 260 bytes --]

Karthik Chikmagalur <karthikchikmagalur@gmail.com> writes:

> I would like to add my package Popper to GNU ELPA.
>
> URL: https://github.com/karthink/popper

Here are a few changes I would propose to make.  Haven't tested it, so
take it with a grain of salt:


[-- Attachment #2: Type: text/plain, Size: 14164 bytes --]

diff --git a/popper.el b/popper.el
index 2c392d6..563a655 100644
--- a/popper.el
+++ b/popper.el
@@ -26,55 +26,56 @@
 ;;; Commentary:
 
 ;; Popper is a minor-mode to tame the flood of ephemeral windows Emacs
-;; produces, while still keeping them within arm's reach. Designate any
-;; buffer to "popup" status, and it will stay out of your way. Disimss
-;; or summon it easily with one key. Cycle through all your "popups" or
-;; just the ones relevant to your current buffer. Useful for many
+;; produces, while still keeping them within arm's reach.  Designate any
+;; buffer to "popup" status, and it will stay out of your way.  Disimss
+;; or summon it easily with one key.  Cycle through all your "popups" or
+;; just the ones relevant to your current buffer.  Useful for many
 ;; things, including toggling display of REPLs, documentation,
 ;; compilation or shell output, etc.
 ;;
 ;; For a demo describing usage and customization see
 ;; https://www.youtube.com/watch?v=E-xUNlZi3rI
-;;
-;; COMMANDS:
-;;
-;; popper-mode          : Turn on popup management
-;; popper-toggle-latest : Toggle latest popup
-;; popper-cycle         : Cycle through all popups, or close all open popups
-;; popper-toggle-type   : Turn a regular window into a popup or vice-versa
-;; popper-kill-latest-popup : Kill latest open popup
-;;
-;; CUSTOMIZATION:
-;;
+
+;;;; Commands:
+
+;; `popper-mode': Turn on popup management
+;; `popper-toggle-latest': Toggle latest popup
+;; `popper-cycle': Cycle through all popups, or close all open popups
+;; `popper-toggle-type': Turn a regular window into a popup or vice-versa
+;; `popper-kill-latest-popup': Kill latest open popup
+
+;;;; Customization:
+
 ;; `popper-reference-buffers': A list of major modes or regexps whose
 ;; corresponding buffer major-modes or regexps (respectively) should be
 ;; treated as popups.
 ;;
 ;; `popper-mode-line': String or sexp to show in the mode-line of
-;; popper. Setting this to nil removes the mode-line entirely from
+;; popper.  Setting this to nil removes the mode-line entirely from
 ;; popper.
 ;;
 ;; `popper-group-function': Function that returns the context a popup
-;; should be shown in. The context is a string or symbol used to group
+;; should be shown in.  The context is a string or symbol used to group
 ;; together a set of buffers and their associated popups, such as the
-;; project root. Customize for available options.
+;; project root.  Customize for available options.
 ;;
 ;; `popper-display-control': This package summons windows defined by the
-;; user as popups by simply calling `display-buffer'. By default,
-;; it will display your popups in a non-obtrusive way. If you want
+;; user as popups by simply calling `display-buffer'.  By default,
+;; it will display your popups in a non-obtrusive way.  If you want
 ;; Popper to display popups according to window rules you specify in
 ;; `display-buffer-alist' (or through a package like Shackle), set this
 ;; variable to nil.
 ;;
 ;; There are other customization options, such as the ability to suppress
-;; certain popups and keep them from showing. Please customize the popper group
+;; certain popups and keep them from showing.  Please customize the popper group
 ;; for details.
 
 ;;; Code:
 
 (eval-when-compile
-  (require 'cl-lib)
   (require 'subr-x))
+(require 'cl-lib)
+(require 'seq)
 
 (declare-function project-root "project")
 (declare-function project-current "project")
@@ -84,9 +85,11 @@
 (defvar popper-mode)
 
 (defgroup popper nil
-  "Provide functions for easy access to popup windows"
+  "Provide functions for easy access to popup windows."
   :group 'convenience)
 
+;; If you are interested in depending on Compat, you could use
+;; `buffer-match-p' here.
 (defcustom popper-reference-buffers '("\\*Messages\\*$")
   "List of buffers to treat as popups.
 Each entry in the list can be a regexp (string) to match buffer
@@ -113,31 +116,31 @@ Output*, and all help and compilation buffers.
 will match against the Messages buffer, all help buffers and any
 buffer with major-mode derived from fundamental mode that has
 fewer than 10 lines at time of creation."
-  :type '(restricted-sexp :match-alternatives (stringp symbolp functionp consp))
-  :group 'popper)
+  :type '(repeat (string :tag "Regular Expression")
+		 (symbol :tag "Major Mode")
+		 (function :tag "Predicate Function")
+		 ;; What is the consp in (restricted-sexp :match-alternatives (stringp symbolp functionp consp))?
+		 ))
 
 (defcustom popper-mode-line '(:eval (propertize " POP" 'face 'mode-line-emphasis))
   "String or sexp to show in the mode-line of popper.
 
- Can be a quoted list or function. Setting this to nil removes
+ Can be a quoted list or function.  Setting this to nil removes
 the mode-line entirely from popper."
-  :group 'popper
   :type '(choice (const :tag "Off" nil)
                  (string :tag "Literal text")
                  (sexp :tag "General `mode-line-format' entry")))
 
 (defcustom popper-mode-line-position 0
   "Position in mode-line to place `popper-mode-line'."
-  :type 'integer
-  :group 'popper)
+  :type 'natnum)
 
 (defcustom popper-display-control t
   "Whether popper should control the placement of popup windows.
 Choices are:
-\\='user: The default. Only control placement of explicitly marked popups.
+\\='user: The default.  Only control placement of explicitly marked popups.
  nil : Do not control popup placement.
  t   : Control placement of all popups."
-  :group 'popper
   :type '(choice (const :tag "Explicitly set popups only" user)
                  (const :tag "All popups" t)
                  (const :tag "Never" nil)))
@@ -149,9 +152,8 @@ Choices are:
 `popper-display-control' is non-nil.
 
 This function accepts two arguments, a buffer and (optional) an
-action alist and displays the buffer. See (info \"(elisp) Buffer
+action alist and displays the buffer.  See (info \"(elisp) Buffer
 Display Action Alists\") for details on the alist."
-  :group 'popper
   :type 'function)
 
 (defcustom popper-group-function nil
@@ -160,7 +162,7 @@ Display Action Alists\") for details on the alist."
 When set to nil popups are not grouped by context.
 
 This function is called with no arguments and should return a
-string or symbol identifying a popup buffer's group. This
+string or symbol identifying a popup buffer's group.  This
 identifier is used to associate popups with regular buffers (such
 as by project, directory, or `major-mode') so that popup-cycling
 from a regular buffer is restricted to its associated group.
@@ -171,7 +173,6 @@ Built-in choices include
 `popper-group-by-project': Return project root using project.el.
 `popper-group-by-projectile': Return project root using projectile.
 `popper-group-by-perspective': Return perspective name."
-  :group 'popper
   :type '(choice
           (const :tag "Don't group popups" nil)
           (const :tag "Group by project (project.el)" popper-group-by-project)
@@ -185,7 +186,7 @@ Built-in choices include
 
 This can be a number representing the height in chars or a
 function that optionally takes one argument (the popup window)
-and returns the height in chars. This option is ignored when
+and returns the height in chars.  This option is ignored when
 `popper-display-control' is set to nil.
 
 Examples:
@@ -199,7 +200,6 @@ the frame height.
   (fit-window-to-buffer
     win
     (floor (frame-height) 3)))"
-  :group 'popper
   :type '(choice (integer :tag "Height in chars")
                  (function :tag "Height function")))
 
@@ -208,8 +208,7 @@ the frame height.
 
 Each function in the hook is called with the opened popup-buffer
 as current."
-  :type 'hook
-  :group 'popper)
+  :type 'hook)
 
 (defvar popper--reference-names nil
   "List of buffer names whose windows are treated as popups.")
@@ -243,7 +242,7 @@ grouped by the predicate `popper-group-function'.")
 
 (defvar-local popper-popup-status nil
   "Identifies a buffer as a popup by its buffer-local value.
-  Valid values are \\='popup, \\='raised, \\='user-popup or nil.
+Valid values are \\='popup, \\='raised, \\='user-popup or nil.
 
 \\='popup     : This is a popup buffer specified in `popper-reference-buffers'.
 \\='raised    : This is a POPUP buffer raised to regular status by the user.
@@ -257,15 +256,21 @@ grouped by the predicate `popper-group-function'.")
    (floor (frame-height) 6)))
 
 (defun popper-select-popup-at-bottom (buffer &optional alist)
-  "Display and switch to popup-buffer BUFFER at the bottom of the screen."
+  "Display and switch to popup-buffer BUFFER at the bottom of the screen.
+ALIST is an association list of action symbols and values.  See
+Info node `(elisp) Buffer Display Action Alists' for details of
+such alists."
   (let ((window (popper-display-popup-at-bottom buffer alist)))
     (select-window window)))
 
 (defun popper-display-popup-at-bottom (buffer &optional alist)
-  "Display popup-buffer BUFFER at the bottom of the screen."
+  "Display popup-buffer BUFFER at the bottom of the screen.
+ALIST is an association list of action symbols and values.  See
+Info node `(elisp) Buffer Display Action Alists' for details of
+such alists."
   (display-buffer-in-side-window
    buffer
-   (append alist 
+   (append alist
            `((window-height . ,popper-window-height)
              (side . bottom)
              (slot . 1)))))
@@ -306,9 +311,9 @@ directory as a fall back."
 (defun popper-group-by-project ()
   "Return an identifier (project root) to group popups."
   (unless (fboundp 'project-root)
-    (user-error "Cannot find project directory to group popups.
-  Please install `project' or customize
-  `popper-group-function'"))
+    (user-error "Cannot find project directory to group popups. \
+Please install `project' or customize \
+`popper-group-function'"))
   (when-let ((project (project-current)))
     (project-root project)))
 
@@ -317,8 +322,8 @@ directory as a fall back."
 
 This returns the project root found using the projectile package."
   (unless (fboundp 'projectile-project-root)
-    (user-error "Cannot find project directory to group popups.
-  Please install `projectile' or customize
+    (user-error "Cannot find project directory to group popups. \
+Please install `projectile' or customize
   `popper-group-function'"))
   (projectile-project-root))
 
@@ -327,9 +332,9 @@ This returns the project root found using the projectile package."
 
 This returns the name of the perspective."
   (unless (fboundp 'persp-current-name)
-    (user-error "Cannot find perspective name to group popups.
-  Please install `perspective' or customize
-  `popper-group-function'"))
+    (user-error "Cannot find perspective name to group popups. \
+Please install `perspective' or customize \
+`popper-group-function'"))
   (persp-current-name))
 
 (defun popper--find-popups (test-buffer-list)
@@ -519,7 +524,7 @@ next popup windows while keeping the current one (FIXME: This
 behavior can be inconsistent.)
 
 With a double prefix ARG \\[universal-argument]
-\\[universal-argument], toggle all popup-windows. Note that only
+\\[universal-argument], toggle all popup-windows.  Note that only
 one buffer can be show in one slot, so it will display as many
 windows as it can."
   (interactive "p")
@@ -620,9 +625,9 @@ If BUFFER is not specified act on the current buffer instead."
       (seq-some (lambda (pred) (funcall pred buf)) popper--suppressed-predicates)))
 
 (defun popper--suppress-popups ()
-  "Suppress open popups in the user-defined
-  `popper-suppress-buffers' list. This should run after
-  `popper--update-popups' in `window-configuration-change-hook'."
+  "Suppress open popups in the user-defined `popper-suppress-buffers' list.
+This should run after `popper--update-popups' in
+`window-configuration-change-hook'."
   ;; Check if popup-status for any open popup is 'suppressed. If yes, change
   ;; its popup-status to 'popup and hide it.
   (let ((configuration-changed-p))
@@ -644,7 +649,7 @@ If BUFFER is not specified act on the current buffer instead."
 (defun popper--set-reference-vars ()
   "Unpack `popper-reference-buffers' to set popper--reference- variables."
   (cl-labels ((popper--classify-type
-               (elm) (pcase elm
+                (elm) (pcase-exhaustive elm
                        ((pred stringp) 'name)
                        ((and (pred symbolp)
                              (guard (or (memq 'derived-mode-parent (symbol-plist elm))
@@ -654,12 +659,12 @@ If BUFFER is not specified act on the current buffer instead."
                        ((pred functionp) 'pred)
                        ((pred consp) 'cons)))
               (popper--insert-type
-               (elm) (pcase (popper--classify-type elm)
+                (elm) (pcase-exhaustive (popper--classify-type elm)
                        ('name (cl-pushnew elm popper--reference-names))
                        ('mode (cl-pushnew elm popper--reference-modes))
                        ('pred (cl-pushnew elm popper--reference-predicates))
                        ('cons (when (eq (cdr elm) 'hide)
-                                (pcase (popper--classify-type (car elm))
+                                (pcase-exhaustive (popper--classify-type (car elm))
                                   ('name (cl-pushnew (car elm) popper--suppressed-names))
                                   ('mode (cl-pushnew (car elm) popper--suppressed-modes))
                                   ('pred (cl-pushnew (car elm) popper--suppressed-predicates))))
@@ -669,10 +674,11 @@ If BUFFER is not specified act on the current buffer instead."
 
 ;;;###autoload
 (define-minor-mode popper-mode
-  "Toggle Popper mode. When enabled, treat certain buffer
-windows as popups, a class of window that can be summoned or
-dismissed with a command. See the customization options for
-details on how to designate buffer types as popups."
+  "Toggle Popper mode.
+When enabled, treat certain buffer windows as popups, a class of
+window that can be summoned or dismissed with a command.  See the
+customization options for details on how to designate buffer
+types as popups."
   :global t
   :version "0.4.5"
   :lighter ""

[-- Attachment #3: Type: text/plain, Size: 2622 bytes --]


Also, it seems adding a .elpaignore file would be nice to remove
unnecessary files from the tarball:  For now a file just containing
"images" should suffice.

Also also, by default the README file will be used to generate a package
description (as seen in C-h P).  I feel that the current file is just a
tad too long for this indent, and the description in the commentary
section might be preferable.  Would you be fine with using that instead?

> What is Popper?
>
> Short for "Popup Buffer", 

FWIW I don't think I would have understood this.  Perhaps it is just me,
but despite fearing a general discussion about package names, do you
think renaming the package to something like "popup-buffers" would be
imaginable.  If not, it is fine, just wanted to bring it up /briefly/.

>                           it's a global minor mode that designates
> buffers as "popups", which means their display can be toggled with a
> single key-press.  This is useful for quickly displaying and dismissing
> "auxiliary" buffers like shells, compilation, help/man, grep buffers
> etc.  If you have used the guake/yakuake pull-down terminals on GNOME or
> KDE, the experience should be similar.
>
> - You can designate a buffer as a "popup" manually or automatically
>   ahead of time via name/major-mode or any predicate of your choosing.
>
> - You can cycle through your popups with one key.
>
> - You can group your popups by any predicate: for example, by project so
>   that you only see auxiliary buffers relevant to your current project.
>
> There are other features, and a few video demos at the link.

Is the video mirrored on some other platform as well?

> There are a few other packages that do this kind of thing, but they are
> one or more of:
>
> - Unmaintained (Popwin).
>
> - Too limited in scope.  For example, the shellpop and vterm-toggle
> packages allows this kind of behavior but only for shells.  Popper is
> generic.
>
> - Too rigid/overarching in their display behavior.  In contrast, Popper
> does not try to control how popups are displayed: your
> display-buffer-alist rules are respected, so that it composes well with
> your other packages or customizations involving buffer display.
>
> This package is now 4+ years old and (I think) in a stable state.  It
> has been installed 16,000+ times.
>
> Copyright assignment: I have signed the FSF copyright assignment papers.
> Popper has several other contributors, but each of their contributions
> is limited to 1-2 lines, usually fixing a linting issue or typo.

Wunderbar.

> Please let me know if need more information or have any concerns.
>
> Karthik

Re: Proposal to add Popper to ELPA

[Karthik Chikmagalur]

Thank you for the review, especially for the changes to the defcustoms.
I'm not very familiar with the many options the customization interface
provides.  I have applied all changes except the one in the defcustom
for `popper-reference-buffers' (which see below).

In case you missed it during the review, the package has two files:
`popper.el' and `popper-echo.el'.  (The latter provides `popper-echo-mode',
which adds echo-area display of available popups and allows for quick
selection using a transient keymap.)

> +;; If you are interested in depending on Compat, you could use
> +;; `buffer-match-p' here.
>  (defcustom popper-reference-buffers '("\\*Messages\\*$")
>    "List of buffers to treat as popups.

I would like to avoid depending on Compat for one utility function.

> -  :type '(restricted-sexp :match-alternatives (stringp symbolp functionp consp))
> -  :group 'popper)
> +  :type '(repeat (string :tag "Regular Expression")
> +		 (symbol :tag "Major Mode")
> +		 (function :tag "Predicate Function")
> +		 ;; What is the consp in (restricted-sexp :match-alternatives (stringp symbolp functionp consp))?
> +		 ))

The `consp' here is for specifying additional behavior per popup.
Presently this is limited to requesting that (initial) display of the
popup be suppressed, but support for other special behavior could be
provided in the future.  Examples of this are in the README in the
section "Suppressing popups".

An example: `("\\*Compilation\\*" . hide)'

How do you suggest incorporating this into the defcustom?

> Also, it seems adding a .elpaignore file would be nice to remove
> unnecessary files from the tarball:  For now a file just containing
> "images" should suffice.

Done, thank you.

> Also also, by default the README file will be used to generate a package
> description (as seen in C-h P).  I feel that the current file is just a
> tad too long for this indent, and the description in the commentary
> section might be preferable.  Would you be fine with using that instead?

Yes, that should be fine.  The commentary section explains the purpose
of the package and the main functions.

>> What is Popper?
>>
>> Short for "Popup Buffer", 
>
> FWIW I don't think I would have understood this.  Perhaps it is just me,
> but despite fearing a general discussion about package names, do you
> think renaming the package to something like "popup-buffers" would be
> imaginable.  If not, it is fine, just wanted to bring it up /briefly/.

I took my cue for the name `popper' from Emacs' built-in `winner-mode'.
At 16K+ downloads, Popper is a reasonably well known package and I think
renaming it now will create more confusion than clarity.

The byline for the package is "Summon and dismiss buffers as popups",
which shows up in package listings and should help discoverability.

>> There are other features, and a few video demos at the link.
>
> Is the video mirrored on some other platform as well?

It's not.  Do you have a suggestion for where you'd like the videos to
be available?  I can make that happen.

Karthik

Re: Proposal to add Popper to ELPA

[Mauro Aranda]

On 5/9/23 14:38, Philip Kaludercic wrote:
 > Karthik Chikmagalur <karthikchikmagalur@gmail.com> writes:
 >
 >> I would like to add my package Popper to GNU ELPA.
 >>
 >> URL: https://github.com/karthink/popper
 >
 > Here are a few changes I would propose to make.  Haven't tested it, so
 > take it with a grain of salt:
 > -  :type '(restricted-sexp :match-alternatives (stringp symbolp 
functionp consp))
 > -  :group 'popper)
 > +  :type '(repeat (string :tag "Regular Expression")
 > +		 (symbol :tag "Major Mode")
 > +		 (function :tag "Predicate Function")
 > +		 ;; What is the consp in (restricted-sexp :match-alternatives 
(stringp symbolp functionp consp))?
 > +		 ))

The different alternatives should be inside a choice:
:type '(repeat (choice (string :tag "Regular Expression")
                        (symbol :tag "Major Mode")
                        (function :tag "Predicate Function"))

 >   (defcustom popper-mode-line-position 0
 >     "Position in mode-line to place `popper-mode-line'."
 > -  :type 'integer
 > -  :group 'popper)
 > +  :type 'natnum)

IIUC, the package requires Emacs 26.1, and the natnum widget wasn't
available back then.  Does Compat have it?

Re: Proposal to add Popper to ELPA

[Mauro Aranda]

On 5/9/23 17:12, Karthik Chikmagalur wrote:

 > In case you missed it during the review, the package has two files:
 > `popper.el' and `popper-echo.el'.  (The latter provides 
`popper-echo-mode',
 > which adds echo-area display of available popups and allows for quick
 > selection using a transient keymap.)

I took a look at the defcustoms in popper-echo.el.  Here are my proposed
changes:

diff --git a/popper-echo.el b/popper-echo.el
index 0369840..b19d7cb 100644
--- a/popper-echo.el
+++ b/popper-echo.el
@@ -58,7 +58,8 @@ This is called on buffer-names displayed by `popper-echo'.

  This function should accept a
    string (the buffer name) and return a transformed string."
-  :type 'function
+  :type '(choice (const :tag "Don't transform buffer-names" nil)
+                 function)
    :group 'popper)

  (defcustom popper-echo-lines 2
@@ -102,7 +103,7 @@ Examples:

  This variable has no effect when popper-echo-mode is turned
  off."
-  :type '(group character string)
+  :type '(repeat (choice character string))
    :group 'popper)

  (defface popper-echo-area-buried

Re: Proposal to add Popper to ELPA

[Karthik Chikmagalur]

>  > -  :type '(restricted-sexp :match-alternatives (stringp symbolp 
> functionp consp))
>  > -  :group 'popper)
>  > +  :type '(repeat (string :tag "Regular Expression")
>  > +		 (symbol :tag "Major Mode")
>  > +		 (function :tag "Predicate Function")
>  > +		 ;; What is the consp in (restricted-sexp :match-alternatives 
> (stringp symbolp functionp consp))?
>  > +		 ))
>
> The different alternatives should be inside a choice:
> :type '(repeat (choice (string :tag "Regular Expression")
>                         (symbol :tag "Major Mode")
>                         (function :tag "Predicate Function"))

In addition, cons cells of the form '(string-or-symbol . hide) are
allowed.  I'm not sure how to represent this in the customization types
list.  I'm okay with omitting this from the customize interface, since
it's a niche option that few users will need.

>  >   (defcustom popper-mode-line-position 0
>  >     "Position in mode-line to place `popper-mode-line'."
>  > -  :type 'integer
>  > -  :group 'popper)
>  > +  :type 'natnum)
>
> IIUC, the package requires Emacs 26.1, and the natnum widget wasn't
> available back then.  Does Compat have it?

Thanks for checking.  I will revert the type to 'integer for now.

Karthik

Re: Proposal to add Popper to ELPA

[Mauro Aranda]

On 5/9/23 17:49, Karthik Chikmagalur wrote:
 >>  > -  :type '(restricted-sexp :match-alternatives (stringp symbolp
 >> functionp consp))
 >>  > -  :group 'popper)
 >>  > +  :type '(repeat (string :tag "Regular Expression")
 >>  > +         (symbol :tag "Major Mode")
 >>  > +         (function :tag "Predicate Function")
 >>  > +         ;; What is the consp in (restricted-sexp 
:match-alternatives
 >> (stringp symbolp functionp consp))?
 >>  > +         ))
 >>
 >> The different alternatives should be inside a choice:
 >> :type '(repeat (choice (string :tag "Regular Expression")
 >>                         (symbol :tag "Major Mode")
 >>                         (function :tag "Predicate Function"))
 >
 > In addition, cons cells of the form '(string-or-symbol . hide) are
 > allowed.  I'm not sure how to represent this in the customization types
 > list.  I'm okay with omitting this from the customize interface, since
 > it's a niche option that few users will need.

I didn't notice the strings were really regular expressions, so I
adapted the type to use regexp.

The cons widget, as below, does the job but it looks UGLY.  I'm in a
hurry now, so I can't take a look at how to make it prettier.

:type '(repeat (choice regexp
                        (symbol :tag "Major Mode")
                        (function :tag "Predicate Function")
                        (cons (choice regexp
                                      (symbol :tag "Major Mode"))
                              (const :tag "Hide" hide))))

Re: Proposal to add Popper to ELPA

[Karthik Chikmagalur]

> :type '(repeat (choice regexp
>                         (symbol :tag "Major Mode")
>                         (function :tag "Predicate Function")
>                         (cons (choice regexp
>                                       (symbol :tag "Major Mode"))
>                               (const :tag "Hide" hide))))

Thanks, I improved the documentation for this user option and added this
customization type.  I think this addresses all the issues raised by
Philip.

Karthik

Re: Proposal to add Popper to ELPA

[Philip Kaludercic]

Karthik Chikmagalur <karthikchikmagalur@gmail.com> writes:

> Thank you for the review, especially for the changes to the defcustoms.
> I'm not very familiar with the many options the customization interface
> provides.  I have applied all changes except the one in the defcustom
> for `popper-reference-buffers' (which see below).
>
> In case you missed it during the review, the package has two files:
> `popper.el' and `popper-echo.el'.  (The latter provides `popper-echo-mode',
> which adds echo-area display of available popups and allows for quick
> selection using a transient keymap.)
>
>> +;; If you are interested in depending on Compat, you could use
>> +;; `buffer-match-p' here.
>>  (defcustom popper-reference-buffers '("\\*Messages\\*$")
>>    "List of buffers to treat as popups.
>
> I would like to avoid depending on Compat for one utility function.

That is understandable, I just wanted to bring it up.

>> -  :type '(restricted-sexp :match-alternatives (stringp symbolp functionp consp))
>> -  :group 'popper)
>> +  :type '(repeat (string :tag "Regular Expression")
>> +		 (symbol :tag "Major Mode")
>> +		 (function :tag "Predicate Function")
>> +		 ;; What is the consp in (restricted-sexp :match-alternatives (stringp symbolp functionp consp))?
>> +		 ))
>
> The `consp' here is for specifying additional behavior per popup.
> Presently this is limited to requesting that (initial) display of the
> popup be suppressed, but support for other special behavior could be
> provided in the future.  Examples of this are in the README in the
> section "Suppressing popups".
>
> An example: `("\\*Compilation\\*" . hide)'
>
> How do you suggest incorporating this into the defcustom?
>
>> Also, it seems adding a .elpaignore file would be nice to remove
>> unnecessary files from the tarball:  For now a file just containing
>> "images" should suffice.
>
> Done, thank you.
>
>> Also also, by default the README file will be used to generate a package
>> description (as seen in C-h P).  I feel that the current file is just a
>> tad too long for this indent, and the description in the commentary
>> section might be preferable.  Would you be fine with using that instead?
>
> Yes, that should be fine.  The commentary section explains the purpose
> of the package and the main functions.

1+

>>> What is Popper?
>>>
>>> Short for "Popup Buffer", 
>>
>> FWIW I don't think I would have understood this.  Perhaps it is just me,
>> but despite fearing a general discussion about package names, do you
>> think renaming the package to something like "popup-buffers" would be
>> imaginable.  If not, it is fine, just wanted to bring it up /briefly/.
>
> I took my cue for the name `popper' from Emacs' built-in `winner-mode'.
> At 16K+ downloads, Popper is a reasonably well known package and I think
> renaming it now will create more confusion than clarity.

For the record, I supposed you are referring to MELPA?

> The byline for the package is "Summon and dismiss buffers as popups",
> which shows up in package listings and should help discoverability.

Of course, which is why I am not insisting on anything, I just wanted to
point out that people like me with slight dyslexia have difficulties
reading and remembering names like these.  It was actually just
yesterday that I noticed the name is not "poppler".

>>> There are other features, and a few video demos at the link.
>>
>> Is the video mirrored on some other platform as well?
>
> It's not.  Do you have a suggestion for where you'd like the videos to
> be available?  I can make that happen.

I guess any peertube instance should be OK:
https://joinpeertube.org/publish-videos.  I haven't taken a look at the
video, but as long as there is nothing mentioned in the video that is
not mentioned elsewhere, everything should be fine.  While some people
prefer it, others find videos to be a information-sparse format.

> Karthik

Re: Proposal to add Popper to ELPA

[Karthik Chikmagalur]

>>>> What is Popper?
>>>>
>>>> Short for "Popup Buffer", 
>>>
>>> FWIW I don't think I would have understood this.  Perhaps it is just me,
>>> but despite fearing a general discussion about package names, do you
>>> think renaming the package to something like "popup-buffers" would be
>>> imaginable.  If not, it is fine, just wanted to bring it up /briefly/.
>>
>> I took my cue for the name `popper' from Emacs' built-in `winner-mode'.
>> At 16K+ downloads, Popper is a reasonably well known package and I think
>> renaming it now will create more confusion than clarity.
>
> For the record, I supposed you are referring to MELPA?

Yes, that number is from https://melpa.org/#/popper.

>> The byline for the package is "Summon and dismiss buffers as popups",
>> which shows up in package listings and should help discoverability.
>
> Of course, which is why I am not insisting on anything, I just wanted to
> point out that people like me with slight dyslexia have difficulties
> reading and remembering names like these.  It was actually just
> yesterday that I noticed the name is not "poppler".

I see.

>>>> There are other features, and a few video demos at the link.
>>>
>>> Is the video mirrored on some other platform as well?
>>
>> It's not.  Do you have a suggestion for where you'd like the videos to
>> be available?  I can make that happen.
>
> I guess any peertube instance should be OK:
> https://joinpeertube.org/publish-videos.  I haven't taken a look at the
> video, but as long as there is nothing mentioned in the video that is
> not mentioned elsewhere, everything should be fine.  While some people
> prefer it, others find videos to be a information-sparse format.

There is one 10+ min extended demo video on Youtube -- this is now quite
out of date and only useful to get a general overview.  Additionally
there are a few short 5-10 second gifs on the Github page that
illustrate specific features.  These only make sense in the context of
the README, so uploading them individually to peertube won't help
either.

I like the 5-10 second gifs as accompaniments to text, but am not a
fan of extended video demos either since they're not greppable.

I've applied the changes you suggested, and improved the documentation
in a couple of other places.

Karthik

Re: Proposal to add Popper to ELPA

[Philip Kaludercic]

Karthik Chikmagalur <karthikchikmagalur@gmail.com> writes:

>> :type '(repeat (choice regexp
>>                         (symbol :tag "Major Mode")
>>                         (function :tag "Predicate Function")
>>                         (cons (choice regexp
>>                                       (symbol :tag "Major Mode"))
>>                               (const :tag "Hide" hide))))
>
> Thanks, I improved the documentation for this user option and added this
> customization type.  I think this addresses all the issues raised by
> Philip.

One more point, the user option `popper-echo-dispatch-keys' seems to
have a broken type.  Shouldn't it be something like

(repeat (choice key-sequence character))

with proper tag annotations?

Other than that, I have added the package to the archive, thanks for
contributing your package!

> Karthik

Re: Proposal to add Popper to ELPA

[Karthik Chikmagalur]

> One more point, the user option `popper-echo-dispatch-keys' seems to
> have a broken type.  Shouldn't it be something like
>
> (repeat (choice key-sequence character))
>
> with proper tag annotations?

I'm not sure it makes sense to enter these keybindings one-by-one, or to
have tags attached to each entry.  The idea is that this is a list of
keys that will be bound to buffers.

I will change it from (group string character) to (group key-sequence character).

As an aside, how do I determine when a customization type was added to
Emacs?  I want to be sure that (for instance) the `key-binding' type is
available on Emacs 26.1.

> Other than that, I have added the package to the archive, thanks for
> contributing your package!

Thank you Philip.

Karthik

Re: Proposal to add Popper to ELPA

[Mauro Aranda]

On 8/9/23 18:52, Karthik Chikmagalur wrote:
 >> One more point, the user option `popper-echo-dispatch-keys' seems to
 >> have a broken type.  Shouldn't it be something like
 >>
 >> (repeat (choice key-sequence character))
 >>
 >> with proper tag annotations?
 >
 > I'm not sure it makes sense to enter these keybindings one-by-one, or to
 > have tags attached to each entry.  The idea is that this is a list of
 > keys that will be bound to buffers.
 >
 > I will change it from (group string character) to (group key-sequence 
character).
 >

Did you see my message about popper-echo.el? Here is a link to the
archives:
https://lists.gnu.org/archive/html/emacs-devel/2023-09/msg00438.html

I don't think is recommended to use the key-sequence type.  It was
declared obsolete in favor of a key type.  But the key type isn't
available on Emacs 26.1, so that's why I proposed to keep the character
and string types.

Re: Proposal to add Popper to ELPA

[Karthik Chikmagalur]

> Did you see my message about popper-echo.el? Here is a link to the
> archives:
> https://lists.gnu.org/archive/html/emacs-devel/2023-09/msg00438.html

I missed it, thank you.

> I don't think is recommended to use the key-sequence type.  It was
> declared obsolete in favor of a key type.  But the key type isn't
> available on Emacs 26.1, so that's why I proposed to keep the character
> and string types.

I've applied your suggested changes.

Karthik

Re: Proposal to add Popper to ELPA

[Philip Kaludercic]

Karthik Chikmagalur <karthikchikmagalur@gmail.com> writes:

>> One more point, the user option `popper-echo-dispatch-keys' seems to
>> have a broken type.  Shouldn't it be something like
>>
>> (repeat (choice key-sequence character))
>>
>> with proper tag annotations?
>
> I'm not sure it makes sense to enter these keybindings one-by-one, or to
> have tags attached to each entry.  The idea is that this is a list of
> keys that will be bound to buffers.
>
> I will change it from (group string character) to (group key-sequence character).

The issue here is that `group' is the wrong type.  See (elisp) Composite
Types,

‘(group ELEMENT-TYPES...)’
     This works like ‘list’ except for the formatting of text in the
     Custom buffer.

and

‘(list ELEMENT-TYPES...)’
     The value must be a list with exactly as many elements as the
     ELEMENT-TYPES given; and each element must fit the corresponding
     ELEMENT-TYPE.

So your type now will match

  (list (kbd "C-a") ?z)

but not

  (list (kbd "C-a"))
  (list ?z)
  (list)
  (list ?a ?z)
  (list (kbd "C-a") (kbd "C-a") ?z)

or the current default value.

> As an aside, how do I determine when a customization type was added to
> Emacs?  I want to be sure that (for instance) the `key-binding' type is
> available on Emacs 26.1.

I don't know if this is the best way, but I checked wid-edit.el and
using vc-annotate I could see that it was added in 2005.  It is also
documented in the ChangeLog files.

>> Other than that, I have added the package to the archive, thanks for
>> contributing your package!
>
> Thank you Philip.
>
> Karthik