Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature Request] Add hooks for pre-processing buffer-file-name and buffer-file-truename. #673

Closed
noncog opened this issue Oct 20, 2023 · 4 comments
Assignees
Labels
enhancement New feature or request

Comments

@noncog
Copy link

noncog commented Oct 20, 2023

Is your feature request related to a problem? Please describe.

When using Denote, Org Roam, or other note taking modes, metadata is often stored in the file name itself. Usually in the form of numbers. Although useful for file searching, it's not very appealing in the modeline, especially with many numbers (Denote, see below). It would be nice to be able to pre-process the file name to reformat or remove the metadata (usually a timestamp) before the built-in file name styles are applied.

Describe the solution you'd like

I would like to add a hook(s) in the function: doom-modeline-buffer-file-name in the let* binding which allows the user to add custom processing to both buffer-file-name and buffer-file-truename. This will allow the user to easily make their customization needs to just the file name itself without the need to redefine the formater function doom-modeline-buffer-file-name such as somehow extending the cases in its pcase doom-modeline-buffer-file-name-style or using poorly created advices. The user can then supply simple functions which transform the file name which would then continue on to be further formatted by the built-in styles. Adding an area at this level would maintain complete compatibility with the rest of the package's functionality with basically zero cost while providing the user this freedom.

Describe alternatives you've considered

I don't believe the following alternatives are as nice of solutions but could be preferable to the package maintainer somehow:

  • Move the binding of buffer-file-name and buffer-file-truename out of doom-modeline-buffer-file-name and instead pass them in as arguments. This would require the package to now generate those variables using some method, which the user could override, or pass in their own variables, either directly or by using advice.
  • Since buffer-file-name and buffer-file-truename are local variables, instead of using the following let* binding in doom-modeline-buffer-file-name, which uses some fallback logic, perhaps the user can make their modifications to those local variables and locally setq them, then the function doom-modeline-buffer-file-name could simply load the local variable without redefining it.
 (defun doom-modeline-buffer-file-name ()
  "Propertize file name based on `doom-modeline-buffer-file-name-style'."
  (let* ((buffer-file-name (file-local-name (or (buffer-file-name (buffer-base-buffer)) "")))
         (buffer-file-truename (file-local-name
                                (or buffer-file-truename (file-truename buffer-file-name) "")))

A variant of the previous bullet point is what I actually started to write, then I realized that this would be a trivial addition upstream instead of a cumbersome hack. Which, really doesn't even work (yet) since in the above snippet the buffer-file-* variables are being redefined (bound) in this context. Using the following snippet I have been able to reformat and set the buffer file names using advice but I believe that I can't get around the fact that they're being bound again in the let* from above.

Here's a simple proof of concept of that in Doom: (Notice: I use Denote's file naming scheme with Org Roam, which I'm trying to integrate)

(defadvice! my/doom-modeline--buffer-file-name-roam-aware-a (orig-fun)
  :around #'doom-modeline-buffer-file-name
  ;; Only modifies file names for Org-Roam/Denote files.
  (if (and (s-contains-p org-roam-directory (or buffer-file-name ""))
           (string-match denote-title-regexp buffer-file-name))
      (progn
        (my/reformat-metadata buffer-file-name)
        (my/reformat-metadata buffer-file-truename))
    (funcall orig-fun)))
  • Lastly, as shown in the above snippet, there is potential use for "contexts" under which select modifications to the filename would happen. For me, that's when viewing Denote/Org-Roam files with metadata in the file name. However, perhaps this idea can be further implemented to enable users to show custom file names, using custom functions, in contexts of their choosing. I would leave that implementation up to the author as my elisp-fu isn't the greatest. Perhaps an alist could be used to provide a "checker" (predicate) function to decide if reformatting should be done and if it yields true, then its cdr could be ran to apply the associated formatting.

Additional context

  • Denote's file naming convention/metadata (Prot calls it front-matter) looks like this:
DATE--TITLE__KEYWORDS.EXT
|-- 20210303T120534--this-is-a-test__conference.txt
|-- 20220303T120534--another-sample__meeting.md
`-- 20220620T181255--the-third-test__keyword.org
  • By enabling this feature the modeline could contain something like this:
notes/this-is-a-test.txt
notes/another-sample.md
notes/the-third-tes.org
  • Denote provides great regexps for selecting each section of the metadata, this makes it trivial to use something like replace-regexp-in-string to select the relevant parts and transform them to your liking.
  • For now, I wanted to remove all but the note title and extension, then use the styles from Doom Modeline to show its folder context, however this feature could even enable custom rendering however the user wants, potentially even on a per-file basis.
  • Additionally, the function doom-modeline-buffer-file-name could also benefit from a similar extension to its pcase doom-modeline-buffer-file-name which could be made into some sort of list the user could append custom styles to and enable even further customization.

Gratitude
Thank you for all of your hard work as well as reading and considering these features! I think they would be simple to implement and highly appreciated by the community.

@noncog noncog added the enhancement New feature or request label Oct 20, 2023
@noncog
Copy link
Author

noncog commented Oct 20, 2023

I was able to make a minimal version of the feature to meet my needs. I redefined the let* bind in doom-modeline-buffer-file-name to use my own functions which provide the buffer file name variables.

This:

(defun doom-modeline-buffer-file-name ()
  "Propertize file name based on `doom-modeline-buffer-file-name-style'."
  (let* ((buffer-file-name (file-local-name (or (buffer-file-name (buffer-base-buffer)) "")))
         (buffer-file-truename (file-local-name
                                (or buffer-file-truename (file-truename buffer-file-name) "")))

Turned into this:

(defun doom-modeline-buffer-file-name ()
  "Propertize file name based on `doom-modeline-buffer-file-name-style'."
  (let* ((buffer-file-name (my/doom-modeline-buffer-file-name))
         (buffer-file-truename (my/doom-modeline-buffer-file-truename))

And these are responsible for checking if the file name should be formatted or returned like normal:

(defun my/org-roam-note-has-denote-title-p (file-name)
  "Return t if Org Roam note file name should be reformatted to display in Doom Modeline."
  (when (and (s-contains-p (expand-file-name org-roam-directory) (expand-file-name file-name))
             (string-match denote-title-regexp file-name)) t))

(defun my/org-roam-note-reformat-file-name (file-name)
  "Remove Denote ID and keywords from Org Roam file names."
  (replace-regexp-in-string (concat denote-id-regexp "--") "" (replace-regexp-in-string denote-keywords-regexp "" file-name)))

(defun my/doom-modeline-buffer-file-name ()
  "Return buffer-file-name filtered if necessary to display in Doom Modeline."
  (let ((file-name (file-local-name (or (buffer-file-name (buffer-base-buffer)) ""))))
    (if (my/org-roam-note-has-denote-title-p file-name)
        (my/org-roam-note-reformat-file-name file-name) file-name)))

(defun my/doom-modeline-buffer-file-truename ()
  "Return buffer-file-truename filtered if necessary to display in Doom Modeline."
  (let ((file-truename (file-local-name (or buffer-file-truename (file-truename buffer-file-name) ""))))
    (if (my/org-roam-note-has-denote-title-p file-truename)
        (my/org-roam-note-reformat-file-name file-truename) file-truename)))

So far, it works well and I get to benefit from the built-in styles too. Perhaps something like this can eventually become an official feature or extended further.

@seagle0128
Copy link
Owner

seagle0128 commented Oct 21, 2023

Your proposal will make the package more extensible. Adding hooks should be able to implement it, but I prefer to add new options to handle the buffer names, as you did in the snippets.

For example,

(defcustom doom-modeline-file-name-function #'doom-modeline-format-file-name
  "The function to format the file name."
  :type 'function
  :group 'doom-modeline)

(defcustom doom-modeline-file-truename-function #'doom-modeline-format-file-truename
  "The function to format the file truename."
  :type 'function
  :group 'doom-modeline)

(defun doom-modeline-buffer-file-name ()
  "Propertize file name based on `doom-modeline-buffer-file-name-style'."
  (let* ((buffer-file-name (funcall #'doom-modeline-format-file-name))
         (buffer-file-truename (funcall #'doom-modeline-format-file-truename)))
    ;;...
  ))

Thoughts?

@noncog
Copy link
Author

noncog commented Oct 21, 2023

My primary concern with implementing it in this way is the fact that there is some care needed when selecting the buffer names. For example, in the original function, you used the following logic:

(defun doom-modeline-buffer-file-name ()
  "Propertize file name based on `doom-modeline-buffer-file-name-style'."
  (let* ((buffer-file-name (file-local-name (or (buffer-file-name (buffer-base-buffer)) "")))
         (buffer-file-truename (file-local-name
                                (or buffer-file-truename (file-truename buffer-file-name) "")))

This appears to be able to handle remote filenames. It seems like you've put some care into ensuring the buffer file names are returned correctly in different scenarios.

If you were to remove this, then I feel that it would be possible for the user to lose this functionality or other future ones like it by simply calling to buffer-file-name and buffer-file-truename. I'm not sure about those additional context/requirements or "what is best" regarding this situation. I'm just concerned with the above structure putting that consideration onto the user whereas a simple filtering function doesn't. Perhaps I'm mistaken but it seems like they would have to handle all aspects of getting the buffer file name and filtering it based on its context.

Further thoughts:

In my opinion, I lean toward preferring a list of simple filter functions run on the buffer file name acquired by the package. I don't have any real opinions on hooks or other implementation, just about structure. So, I think that it should go:

Doom-Modeline gets buffer file names > Filter according to user function(s) > Continue to built-in styles.

Your previous example also appears to run a single function which the user would have to include all of their filtering logic into. Perhaps it could be used like a hook to run a list of filtering functions, which makes sense, run one filter function to return the formatted file name. I do think that I lean toward allowing the user to have filters separated into a list functions to easily add/remove logic. This would make individual filters more sharable and allow combinations of them to run in different contexts if desired. I doubt there is a need for many filters though so maybe that's not actually a concern.

The above could be restructured to include the acquisition of the initial buffer file names in the functions and somehow handle a list of functions from the user. To me, that sounds like the job of a hook, but could be implemented equivalently using customizations. At minimum, I think that the package should get the buffer file name for the user and then be filtered by a list of user function and then be styled.

What do you think? I'm not very experienced with hooks or package development. Is there some reason to prefer defcustoms over hooks? Also is there some form of like a no-op for the empty filter the package would ship with? I don't know how that would work. Either way, the only real concern is the acquisition of the buffer file name and then running a filter which can be structured equivalently in many ways, whether it's a single function, hook, list of functions, or whatever, that's all up to your preference.

@seagle0128
Copy link
Owner

seagle0128 commented Oct 23, 2023

Doom-Modeline gets buffer file names > Filter according to user function(s) > Continue to built-in styles.

The steps are correct. The only issue is the return value of run-hook-with-args is unspecified. In my env (Windows & Linux, Emacs 29.1 & 28.1), the snippet below returns "Return value: nil". Can you please try?

(defvar my-hook nil
  "My custom hook")

(defun my-hook-function (arg1 arg2)
  "A function to be executed by the hook"
  (message "Argument 1: %s" arg1)
  (message "Argument 2: %s" arg2)
  (+ arg1 arg2))

(add-hook 'my-hook 'my-hook-function)

(let ((result (run-hook-with-args 'my-hook 2 3)))
  (message "Return value: %s" result))

So I implemented with functions but keep the original handles and styles. The default value is identity, and the users can implement a function to handle the names as what the users want. I think it meets the most of requirements, right?

Of course if I find a better solution, I shall update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants