protesilaos / denote

Simple notes for Emacs with an efficient file-naming scheme
https://protesilaos.com/emacs/denote
GNU General Public License v3.0
530 stars 55 forks source link

Suggestion a denote-keyword-append function. #405

Closed MirkoHernandez closed 1 month ago

MirkoHernandez commented 3 months ago

I recently updated to denote 3 and I'm not getting used to the newdenote-keyword-addbehavior . I understand that it is much faster for adding and removing multiple keywords, but it is less reliable (a typing error can remove or rename a previous keyword).

And there are also problems with some other packages. I'm using citar-denote and the citar-denote-add-citekey command is broken since it can't append a keyword now, It calls denote-keyword-add with the intention of appending a keyword.

Providing an append function would trivially allow backward compatibility with other packages. Checking the denote version would allow anyone to set the proper function for appending keywords.

MirkoHernandez commented 3 months ago

I had some trouble creating an append (and remove) function similar to the previous denote-keywords-add. I'm not sure how to easily tell denote to use the current values, is there a way to do this ?, I could not find a way searching the denote 3 code, I had to manually fetch and pass the values.

(defun denote-keywords-append (&optional keywords)
  (interactive)
  (let* ((file (denote--rename-dired-file-or-current-file-or-prompt))
     (denote-prompts '())
     (cur-values (denote--rename-get-file-info-from-prompts-or-existing file))
     (cur-title (cl-first cur-values))
     (cur-keywords (cl-second cur-values))
     (cur-signature (cl-third cur-values))
     (new-keywords (seq-uniq (append cur-keywords
                     (or keywords (denote-keywords-prompt))))))
    (funcall-interactively 'denote-rename-file
               file 
               cur-title 
               new-keywords
               cur-signature
               nil)))
protesilaos commented 2 months ago

From: Mirko Hernández @.***> Date: Sat, 3 Aug 2024 13:34:05 -0700

I recently updated to denote 3 and I'm not getting used to the new denote-keyword-addbehavior . I understand that it is much faster for adding and removing multiple keywords, but it is less reliable (a typing error can remove or rename a previous keyword).

A typing error is indeed a concern, though this is true for all functions we provide.

And there are also problems with some other packages. I'm using citar-denote and the citar-denote-add-citekey command is broken since it can't append a keyword now, It calls denote-keyword-add with the intention of appending a keyword.

Oh, this is undesirable! I believe our old commands were declared as "interactive-only", though I think the byte compiler does not actually complain where such functions are used in Lisp.

Providing an append function would trivially allow backward compatibility with other packages. Checking the denote version would allow anyone to set the proper function for appending keywords.

Does this work for you?

(defun denote-rename-file-keywords-add (&optional keywords)
  (interactive)
  (let* ((file (denote--rename-dired-file-or-current-file-or-prompt))
         (file-type (denote-filetype-heuristics file))
         (title (denote-retrieve-title-or-filename file file-type))
         (old-keywords (denote-extract-keywords-from-path file))
         (new-keywords (or keywords (denote-keywords-prompt)))
         (signature (denote-retrieve-filename-signature file))
         (date (denote-retrieve-filename-identifier file))
         (keywords (seq-union new-keywords old-keywords #'equal)))
    (denote--rename-file file title keywords signature date)))

-- Protesilaos Stavrou https://protesilaos.com

MirkoHernandez commented 2 months ago

Thanks!, I was not sure which functions to use for creating that rename command. I'm used to creating custom commands so I forgot that it is proper to use a private function when creating an official denote command.

protesilaos commented 2 months ago

From: Mirko Hernández @.***> Date: Mon, 5 Aug 2024 09:59:57 -0700

Thanks!

You are welcome!

I was not sure which functions to use for creating that rename command. I'm used to creating custom commands so I forgot that it is proper to use a private function when creating an official denote command.

This is fine, no worries!

How do we proceed now? Does citar-denote need to have this or should we add it to denote.el? Do you need complementary functionality? Other issues?

-- Protesilaos Stavrou https://protesilaos.com

MirkoHernandez commented 2 months ago

How do we proceed now? Does citar-denote need to have this or should we add it to denote.el?

Yes, I think that it should be added to denote. Packages that append keywords would benefit from this function, and also from an user's perspective I think that it improves usability (I like the new rename function but I would use an append command more often).

Do you need complementary functionality? Other issues?

I think a denote-keywords-remove like the previous one.

jeanphilippegg commented 1 month ago

There are 2 two things with this issue.

Functions to be used by packages

I understand the need for packages to modify an existing note. This is true of all components, not only keywords. Packages may want to modify (rename) a note to change the title, add/remove keywords, change the signature or many other use cases (maybe a combination of those?).

The command denote-rename-file accepts the necessary parameters and is intended to be used for that. Since package authors already know elisp and we already have all the necessary functions to retrieve a note's components as well as denote-rename-file, I would not add new functions for this.

The command denote-rename-file-keywords-add given above by Prot makes use of those retrieval functions combined with denote-rename-file to handle this specific use case of adding a keyword. It is a good starting example for package authors to create the commands they need.

Interactive commands to add/remove keywords

As for an interactive command, we have denote-rename-file-keywords that prompts for keywords and the prompt contains existing keywords. If you want to add a keyword, you just type it. If you want to remove a keyword, you remove it.

If you are used to calling 2 distinct commands for adding and removing keywords, maybe aliases could do the trick, like this:

(defalias 'denote-rename-file-keywords-add `denote-rename-file-keywords)
(defalias 'denote-rename-file-keywords-remove `denote-rename-file-keywords)

If you really need to not see the current keywords in your prompt, you can add to your config the command denote-rename-file-keywords-add suggested by Prot above. For denote-rename-file-keywords-remove, just replace (seq-union new-keywords old-keywords...) with (seq-difference old-keywords new-keywords...).

MirkoHernandez commented 1 month ago

The command denote-rename-file accepts the necessary parameters and is intended to be used for that. Since package authors already know elisp and we already have all the necessary functions to retrieve a note's components as well as denote-rename-file, I would not add new functions for this.

I understand this point, I was coming from the point of view of an user. It was confusing to me the process of creating this particular renaming command. The usual method for extending denote ( denote, denote-prompts and denote-user-enforced-denote-directory) is really elegant and intuitive, so I was surprised having to fetch and pass all the required parameters explicitly.

Maybe, denote-rename-file could work more like denote. My initial attempts looked something like this.

(defun denote-rename-file-keywords-add (&optional keywords)
      (interactive)
      (let* ((file (denote--rename-dired-file-or-current-file-or-prompt))
             (old-keywords (denote-extract-keywords-from-path file))
             (keywords (seq-union new-keywords old-keywords #'equal)))
        (denote-rename-file nil nil keywords)))

After a few errors I realized that I had to call the interactive version and also pass the rest of the parameters.

jeanphilippegg commented 1 month ago

Ok, I think I understand what you are trying to do. Let me know if I got it right.

First, denote-rename-file actually works similarly to denote. In both cases, denote-prompts is respected. For example, the command denote-rename-file-keywords is just like denote-rename-file, but with denote-prompts set with keywords only. denote-rename-file-keywords is just a convenience command like denote-signature is.

Also, both denote and denote-rename-file prompts for keywords using denote-keywords-prompt. This is the same function in both cases. In the case of denote, the prompt starts empty and you fill it with your keywords. In the case of denote-rename-file, the prompt starts prefilled with your existing keywords and you add or remove keywords. This part is not customizable. You want to change how the prompting is made (and also recombine the result with your existing keywords). In this case, you are right that you have to create your own denote-rename-file command like you attempted.

Here are the commands I think you want:

(defun denote-rename-file-keywords-add (file new-keywords)
  (interactive
   (list (denote--rename-dired-file-or-current-file-or-prompt)
         (denote-keywords-prompt)))
  (let* ((old-keywords (denote-extract-keywords-from-path file))
         (keywords (seq-union old-keywords new-keywords))
         (file-type (denote-filetype-heuristics file))
         (date (denote-retrieve-filename-identifier file))
         (title (or (denote-retrieve-title-or-filename file file-type) ""))
         (signature (or (denote-retrieve-filename-signature file) "")))
    (denote-rename-file file title keywords signature date)))

(defun denote-rename-file-keywords-remove (file keywords-to-remove)
  (interactive
   (list (denote--rename-dired-file-or-current-file-or-prompt)
         (denote-keywords-prompt)))
  (let* ((old-keywords (denote-extract-keywords-from-path file))
         (keywords (seq-difference old-keywords keywords-to-remove))
         (file-type (denote-filetype-heuristics file))
         (date (denote-retrieve-filename-identifier file))
         (title (or (denote-retrieve-title-or-filename file file-type) ""))
         (signature (or (denote-retrieve-filename-signature file) "")))
    (denote-rename-file file title keywords signature date)))

But note that the only difference between them and the provided denote-rename-file-keywords is that the keywords prompt leave you "in the dark", ie the existing keywords are not prefilled in the prompt. That is why I don't think that they should be included in Denote. We would end up with 3 almost identical commands.

You can try the commands above and let us know if they do what you want!


Some more details:

denote is more intuitive and elegant than denote-rename-file because creation is simpler than renaming. When you create a new note, the non-supplied optional paramater can only have one meaning: the corresponding parameter is absent from the new note. On the other hand, when you rename a note, a nil title may have 2 meanings: use the existing one or use an empty title.

The renaming commands have evolved a lot over the years and there were a lot of changes made about this last point. In the end, I believe that a nil title should be handled the same way as if a user supplied a nil title at the prompt: it means an empty title (and not to use the previous title). The previous title is prefilled in the prompt, ready to be selected again. This is documented in the docstring of denote-rename-file.

There were also some inconsistencies between how we handled each parameter. A nil title did not mean the same thing as nil keywords, for example.

I believe the renaming commands are now in a good state. denote-rename-file has parameters to rename a file. They are mandatory and used "as-is" for the corresponding component. This is also consistent with the input of the user when called interactively.

This has the consequence that package authors must supply all arguments to denote-rename-file. This is not a problem, as they know elisp and we provide the necessary retrieval functions. It might be a "big" function, but it is not complex.

As for users, they should not have to customize denote-rename-file in such a way. What you are trying to do is very specific: you want to change how the prompting works.

By the way, as explained above, the way you attempted to use denote-rename-file with nil arguments does not work. Your confusion is warranted because they are declared optional parameters. I could have interpreted them the same way. I will make a pull request to remove the optional declaration.

MirkoHernandez commented 1 month ago

That is why I don't think that they should be included in Denote. We would end up with 3 almost identical commands.

I agree, the new approach makes sense. I will use the provided functions;I have been using the previous one suggested by Prot without issue, I will try the other ones.

(defalias 'denote-keywords-add `denote-rename-file-keywords-add)

The advice works for citar-denote-add-citekey, thanks for the suggestion I was using a rewritten version.

On the other hand, when you rename a note, a nil title may have 2 meanings: use the existing one or use an empty title.

This does not make a lot of sense to me, wouldn't an empty title or signature be specified with an empty string?. Maybe a 'keep symbol or some other parameter could be used to provide a default value.

This has the consequence that package authors must supply all arguments to denote-rename-file.

Please consider the idea of adding the default values functionality to denote-rename-file, I'm guessing that most users will want to use it in the same way as the denote function.

jeanphilippegg commented 1 month ago

This does not make a lot of sense to me, wouldn't an empty title or signature be specified with an empty string?

I did not want to go too much in the details here, but you are absolutely right and an empty string should be used to mean an empty title. The docstring also states that a string is expected. However, Prot has made it so a nil value will also work.

Maybe a 'keep symbol or some other parameter could be used to provide a default value.

This would be the right approach! We cannot use nil to mean "keep current title", because it currently means "use an empty title". For the keywords parameter, nil is actually a valid value as it is the empty list of keywords. We cannot use it to mean "keep current keywords". If we want to add this "keep current component" feature, I think your suggestion would be ideal.

Please consider the idea of adding the default values functionality to denote-rename-file

I have pushed this in pull request #429! From lisp, you can now use keep-current for any parameter that you do not want to modify. You can test this by evaluating the internal denote--rename-file (which is used by the public denote-rename-file) in my PR. Package authors and users that want to use denote-rename-file from lisp will be able to use this. Thanks for the suggestion!

MirkoHernandez commented 1 month ago

Thanks!, I appreciate the effort of keeping things simple and convenient, specially for the user facing commands.

protesilaos commented 1 month ago

Just merged the PR from @jeanphilippegg. Thanks @MirkoHernandez.