Re: [PATCH] New "project" option for org-link-file-path-type

[Kyle Meyer <kyle@kyleam.com>]

Jack Kamm writes:

> The attached patch adds a "project" option for org-link-file-path-type.
>
> When this is set, links to files under the current project root will be
> relative, while links elsewhere are absolute.

Thanks, that sounds useful.

> It relies on project.el, which appears to have been added in emacs 25. I
> used fboundp to check whether the functionality is available, and to
> silence compiler warnings. I'm not sure if this is the correct way to do
> it.

Functionally I think your current patch would only support Emacs's
unreleased master, unless the user installed a new project.el via ELPA.
More on that below.

> Subject: [PATCH] ol.el: New option "project" for org-link-file-path-type
[...]
> @@ -212,13 +212,17 @@ (defcustom org-link-file-path-type 'adaptive
>  absolute  Absolute path, if possible with ~ for home directory.
>  noabbrev  Absolute path, no abbreviation of home directory.
>  adaptive  Use relative path for files in the current directory and sub-
> -          directories of it.  For other files, use an absolute path."
> +          directories of it.  For other files, use an absolute path.
> +project   Use relative path for files in the current project and sub-
> +          directories of it. For other files, usue an absolute path.

s/usue/use/

> +          If project.el is not available, behave as adaptive."
>    :group 'org-link
>    :type '(choice
>  	  (const relative)
>  	  (const absolute)
>  	  (const noabbrev)
> -	  (const adaptive))
> +	  (const adaptive)
> +	  (const project))
>    :safe #'symbolp)

The :package-version keyword should be added to signal the change in
value.

>  (defcustom org-link-abbrev-alist nil
> @@ -1876,6 +1880,15 @@ (defun org-insert-link (&optional complete-file link-location description)
>  	    (setq path (expand-file-name path)))
>  	   ((eq org-link-file-path-type 'relative)
>  	    (setq path (file-relative-name path)))
> +	   ((and (fboundp 'project-current)
> +		 (fboundp 'project-root)
> +		 (project-current)
> +		 (eq org-link-file-path-type 'project))

Minor: the org-link-file-path-type check would be better positioned at
the top, or at least before the project-current call, to avoid
needlessly finding the project when the option is at its default value
of adaptive.

Also, I think it'd be good to let-bind the project-current result to
avoid a repeated call.

> +	    (if (string-prefix-p (expand-file-name (project-root
> +						    (project-current)))
> +				 (expand-file-name path))

It looks like project-root isn't available until 5044c19001 (project.el:
A project has only one main root now, 2020-05-23), which isn't yet part
of an Emacs release.  Before that, it'd be (car (project-roots ...), I
think.  Do you think it's worth adding a compatibility kludge here?

As a projectile user, I'm tempted to suggest that, instead of the adding
the `project' value, org-insert-link could learn to call
org-link-file-path-type if it is a function and, if that returns
non-nil, do the prefix check.  Then projectile users could set it to
projectile-project-root.  It seems project.el doesn't have a similar
function that could be called without any arguments, but I guess we
could add a simple ol- wrapper.  I'm not sure that's a good idea,
though.

> +		(setq path (file-relative-name path))
> +	      (setq path (abbreviate-file-name (expand-file-name path)))))

A cosmetic preference that you can take or leave: the condition can be
moved inside the setq form:

  (setq path (if ...))

And another: let-binding (expand-file-name path) would avoid a repeating
the expand-file-name in the abbreviate-file-name case.