bug#41890: bug#42210: bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Eli Zaretskii <eliz@gnu.org>]

> Date: Thu, 23 Jul 2020 19:04:13 -0700
> Cc: "Basil L. Contovounesios" <contovob@tcd.ie>,
>  "Philip K." <philip@warpmail.net>, 41890@debbugs.gnu.org,
>  42210@debbugs.gnu.org
> 
> -(defun project-switch-to-buffer ()
> +(defun project-switch-to-buffer (&optional switching-function)
>    "Switch to another buffer belonging to the current project.
>  This function prompts for another buffer, offering as candidates
>  buffers that belong to the same project as the current buffer.
>  Two buffers belong to the same project if their project instances,
> -as reported by `project-current' in each buffer, are identical."
> +as reported by `project-current' in each buffer, are identical.
> +
> +Optional argument SWITCHING-FUNCTION is the function used to
> +switch the buffer.  It defaults to `switch-to-buffer'."
>    (interactive)

This interface strikes me as unusual and even unexpected for a command
that switches to another buffer.  I would expect it to have an API
similar to that of switch-to-buffer: that it should accept the buffer
to switch to as an argument, and set up that argument in the
'interactive' spec according to the preferences of this command
(offering buffers in the same project etc.).  The API you propose
makes it awkward, to say the least, to invoke this command from Lisp.
Granted, the original API doesn't allow such invocation, either, but
as long as we are changing this API, let's try fixing that, okay?

> +(defun project-display-buffer ()
> +  "Display a buffer of the current project without selecting it.

This doesn't say where that buffer will be displayed.  Please add that
important detail to the doc string.

> +(defun project-display-buffer-other-frame ()
> +  "Display a buffer of the current project, preferably in another frame.
> +
> +See `project-switch-to-buffer' for what it means for a buffer to
> +belong to the current project."

The "preferably" part needs to be explained some more, perhaps by
pointing to display-buffer-other-frame for the details.  Otherwise it
leaves some of the command's MO a mystery.

> +(defvar project-other-window-map
> +  (let ((map (make-sparse-keymap)))
> +    (define-key map "\C-o" #'project-display-buffer)
> +    map)
> +  "Keymap for additional project commands to display in other windows.")

Do you mean "commands _that_ display in other windows"?  If so, please
use "that" instead of "to".  Also, I think the doc string should
explain what do those commands display in those other windows.

> +(defvar project-other-frame-map
> +  (let ((map (make-sparse-keymap)))
> +    (define-key map "\C-o" #'project-display-buffer-other-frame)
> +    map)
> +  "Keymap for additional project commands to display in other frames.")

Same here.

> +;;;###autoload
> +(defun project-other-window-command ()
> +  "Invoke a project command but display its buffer in another window.
                              ^
Comma is missing there.

More importantly, the "its" part is ambiguous.  What does it allude
to?

> +(defun project-other-frame-command ()
> +  "Invoke a project command but display its buffer in another frame.

Same here.

> +(defun project-other-tab-command ()
> +  "Invoke a project command but display its buffer in another tab.

Same here, except that in this case even the idea of "displaying in a
tab" is unclear.  This needs rewording.

Thanks.