Subject: Re: bug#68915: Request to include a couple of packages in GNU ELPA

From: Rahguzar <[email-masked]>
Date: Sun, 04 Feb 2024 21:57:44 +0100

Hi Philip,
  Thanks again for these.

I will make most of the stylistic changes but I would like to not mix
tabs and spaces for the code I want to maintain. Other than that some
responses to your questions are below:

Philip Kaludercic <[email-masked]> writes:

> Stefan Monnier <[email-masked]> writes:
>
>>>> 2) filechooser.el: https://codeberg.org/rahguzar/filechooser
>>>> It implements the backend D-bus methods for xdg filechooser and can be
>>>> used to provide an Emacs based file selection interface for applications
>>>> such as web browsers which support using xdg desktop portals for this
>>>> purpose.
>
> I don't really understand what this package is doing, so my comments are
> more superficial, hope they are nevertheless at least a bit useful:
>
> diff --git a/filechooser.el b/filechooser.el
> index cc96386..e08f4b1 100644
> --- a/filechooser.el
> +++ b/filechooser.el
> @@ -1,26 +1,27 @@
>  ;;; filechooser.el --- An xdg-desktop-portal filechooser -*- lexical-binding: t; -*-
> -;;
> -;; Copyright (C) 2023 azeem
> -;;
> +
> +;; Copyright (C) 2023 azeem <-- is this you?
> +

Yes, sorry will change that.

>  ;; Author: rahguzar <[email-masked]>
>  ;; Maintainer: rahguzar <[email-masked]>
>  ;; Created: May 20, 2023
> -;; Modified: May 20, 2023
> +;; Modified: May 20, 2023 <-- either use time-stamp or remove this please
>  ;; Version: 0.0.1
>  ;; Keywords: convenience files tools unix
>  ;; Homepage: https://codeberg.org/rahguzar/filechooser
>  ;; Package-Requires: ((emacs "28.1") (compat "29.1"))
> -;;
> +
>  ;; This file is not part of GNU Emacs.
> -;;
> +
>  ;;; Commentary:
> -;; An implementation of xdg-desktop-portal filechooser in Emacs. This allows
> +
> +;; An implementation of xdg-desktop-portal filechooser in Emacs.  This allows
>  ;; for choosing files in applications like firefox (with GTK_USE_PORTAL set)
> -;; from an Emacs frame. The default is to use `read-file-name' for choosing
> +;; from an Emacs frame.  The default is to use `read-file-name' for choosing
>  ;; a single file and a pair of Dired buffers for choosing multiple files.
> -;;
> -;;
> +
>  ;;; Code:
> +
>  (require 'compat)
>  (require 'dbus)
>  (require 'xdg)
> @@ -49,25 +50,27 @@
>
>  (defvar filechooser-multiple-selection-map
>    (let ((map (make-sparse-keymap)))
> +    (set-keymap-parent map filechooser-mininuffer-map)
>      (define-key map (kbd "M-TAB") #'filechooser-multiple-continue)
>      (define-key map (kbd "M-RET") #'filechooser-multiple-finalize-current-selection)
>      (define-key map (kbd "M-a") #'filechooser-multiple-select-all)
> -    (make-composed-keymap map filechooser-mininuffer-map)))
> +    map))
>
>  ;;;; Custom variables
> -(defgroup filechooser nil
> +(defgroup filechooser nil		;this is a duplicate!

Is the other definition in some package? I didn't see any filechooser
group is options for `customize-group` without this package loaded.

>    "An xdg-desktop-portal filechooser."
>    :link '(url-link :tag "Homepage" "https://codeberg.org/rahguzar/filechooser")
>    :group 'files)
>
>  (defcustom filechooser-save-existing-files 'uniquify
>    "Determines behavior when attempting to save an existing file FILENAME.
> -If it is symbol `yes-or-no-p', `yes-or-no-p' is used to confirm if the file
> -should be overwritten. If it is the symbol `y-or-n-p', `y-or-n-p' is used to
> -prompt. In both cases if a the answer is negative, the file selection is started
> -again. If it is the symbol `uniquify', the FILENAME is made unique by appedning
> --N to it where N is a positive number. If it is a function, it is called with
> -FILENAME and the return value is used as the filename."
> +If it is symbol `yes-or-no-p', `yes-or-no-p' is used to confirm if the
> +file should be overwritten.  If it is the symbol `y-or-n-p', `y-or-n-p'
> +is used to prompt.  In both cases if a the answer is negative, the file
> +selection is started again.  If it is the symbol `uniquify', the
> +FILENAME is made unique by appedning -N to it where N is a positive
> +number.  If it is a function, it is called with FILENAME and the return
> +value is used as the filename."
>    :type '(choice
>            (const :tag "Uniquify" uniquify)
>            (const :tag "Prompt Yes/No" yes-or-no-p)
> @@ -80,8 +83,8 @@ If it is nil the selected frame is used instead."
>    :type 'boolean)
>
>  (defcustom filechooser-filters `(("Directories" filechooser-file-directory-p . t)
> -                              ("Elisp files" ,(rx ".el" eos))
> -                              ("Not dot files" ,(rx bos (not ?.))))
> +				 ("Elisp files" ,(rx ".el" eos))
> +				 ("Not dot files" ,(rx bos (not ?.))))
>    "An alist of (NAME FILTER . BOOL).
>  NAME should describe the filter which can either be a regexp
>  or else a predicate function which takes a filename as argument.
> @@ -89,8 +92,8 @@ If BOOL is non-nil filter is active by default otherwise it is inactive."
>    :type '(alist :key-type (string :tag "Name")
>                  :value-type
>                  (cons :tag "Value:"
> -                  (choice :tag "Filter" regexp function)
> -                  (boolean :tag "Default"))))
> +                      (choice :tag "Filter" regexp function)
> +                      (boolean :tag "Default"))))
>
>  (defcustom filechooser-choose-file #'filechooser-read-file-name
>    "Function used to choose a single file.
> @@ -114,7 +117,7 @@ It should have the same calling convention as
>    "The key that is used to exit minibuffer to do completion.
>  I.e. the key that binds the equivalent of `exit-minibuffer' for the completion
>  UI of choice: usually RET."
> -  :type 'key)
> +  :type 'key)				;this is not available for Emacs 27

Will (if (> emacs-major-version 27) 'key 'string) be fine here?

>  ;;;; Others
>  (defvar filechooser-current-operation nil
> @@ -122,12 +125,12 @@ UI of choice: usually RET."
>
>  ;;;; Internal Variables
>  (defvar filechooser--filters nil)
> -(defvar filechooser--selection (list (make-temp-file "filechooser-selection-" t)))
> +(defvar filechooser--selection (list (make-temp-file "filechooser-selection-" t))) ;do you really want to evaluate a side effect here?

This is a hack to get a default-directory I can use for a fake dired
buffer. I think it is enough to create such a directory once when the
package is loaded. I will try changing it so that the existence of this
directory is checked at the point of use and if not it is created.

>  (defvar filechooser--multiple-selection nil)
>
>  ;;; Filters
>  (defun filechooser--filters-group-fn (cand transform)
> -  "Group function for selecting filters. CAND TRANSFORM."
> +  "Group function for selecting filters.  CAND TRANSFORM." ;please document CAND and TRANSFORM

Will do.

>    (if transform
>        cand
>      (if (cdr (alist-get cand filechooser--filters nil nil #'equal))

Thanks,
Rahguzar