Package: elpa;
Reported by: Stefan Monnier <monnier <at> iro.umontreal.ca>
Date: Sat, 3 Feb 2024 22:32:01 UTC
Severity: normal
Message #32 received at 68915 <at> debbugs.gnu.org (full text, mbox):
From: Philip Kaludercic <philipk <at> posteo.net> To: Rahguzar <rahguzar <at> zohomail.eu> Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 68915 <at> debbugs.gnu.org, Stefan Kangas <stefankangas <at> gmail.com> Subject: Re: bug#68915: Request to include a couple of packages in GNU ELPA Date: Mon, 05 Feb 2024 07:06:00 +0000
Rahguzar <rahguzar <at> zohomail.eu> 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 <philipk <at> posteo.net> writes:
>
>> Stefan Monnier <monnier <at> iro.umontreal.ca> 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 <rahguzar <at> zohomail.eu>
>> ;; Maintainer: rahguzar <rahguzar <at> zohomail.eu>
>> ;; 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
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.