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

From: Philip Kaludercic <[email-masked]>
Date: Mon, 05 Feb 2024 07:06:00 +0000

Rahguzar <[email-masked]> writes:

> 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:

That was my mistake, I just ran indent-region on the entire buffer.  If
you want to avoid issues with this in the future, I'd recommend
adding a .dir-locals.el file that disables indent-tabs-mode.

I usually have this in my packages,

--8<---------------cut here---------------start------------->8---
;;; Directory Local Variables
;;; For more information see (info "(emacs) Directory Variables")

((emacs-lisp-mode
  (indent-tabs-mode . nil)
  (show-trailing-whitespace . t)
  (sentence-end-double-space . t)
  (byte-compile-error-on-warn . t)))
--8<---------------cut here---------------end--------------->8---

though I know that some people think it is opinionated.

> 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.

I believe there was a duplicate definition a few lines above.

>>    "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?

I think 'key-sequence would be the more adequate fallback.

>>  ;;;; 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.

Right, that sounds more robust.  I don't know all the edge cases in
which a temporary directory can be removed, especially over different
operating systems, so I'd recommend not relying on it, especially over
longer periods.

>>  (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.

Thanks.

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