emacs-helm / helm-org

53 stars 9 forks source link

Make `helm-org-insert-link-to-heading-at-marker` insert id when the selected heading has one #25

Open rodrigomorales1 opened 3 years ago

rodrigomorales1 commented 3 years ago

Given that org-id.el is a native package and some users use this feature for creating a permanent link to a heading, I think it would be useful that helm-org-insert-link-to-heading-at-marker consider the ID property for referencing a selected heading. The current behavior is to use the heading title.

rodrigomorales1 commented 3 years ago

I guess it would be better if there were a defcustom that would allow the user to define whether helm-org-insert-link-to-headint-at-marker use ids or not, so that helm would create the ID in the scenario that the selected heading doesn't have one.

alphapapa commented 3 years ago

I haven't looked at the code in question, but we should probably be using Org's built-in link-creating functions (i.e. org-store-link), which should take the existing option org-id-link-to-org-use-id into account. We probably shouldn't be doing this manually and adding a Helm-specific option.

rodrigomorales1 commented 3 years ago

@alphapapa I agree with you, using org-store-link (your idea) would be better than creating a defcustom (my idea) for this.

For those interested, here's the relevant part of the source code:

The function helm-org-headings-actions contains helm-org-insert-link-to-heading-at-marker.

https://github.com/emacs-helm/helm-org/blob/d67186d3a64e610c03a5f3d583488f018fb032e4/helm-org.el#L98-L105

The function helm-org-insert-link-to-heading-at-marker uses org-insert-link.

https://github.com/emacs-helm/helm-org/blob/d67186d3a64e610c03a5f3d583488f018fb032e4/helm-org.el#L376-L384

alphapapa commented 3 years ago

Thanks. I don't use this feature, myself, so I'll let someone else propose a patch. Please feel free to tag my username to get my attention on this again; notifications on repos I don't own tend to get lost.

rodrigomorales1 commented 3 years ago

@alphapapa I've created a pull request: #26