Skip to content

(feat): interactive functions for managing aliases and tags#1183

Merged
jethrokuan merged 1 commit intoorg-roam:masterfrom
d12frosted:feature/tags-and-alias-management
Oct 12, 2020
Merged

(feat): interactive functions for managing aliases and tags#1183
jethrokuan merged 1 commit intoorg-roam:masterfrom
d12frosted:feature/tags-and-alias-management

Conversation

@d12frosted
Copy link
Copy Markdown
Contributor

@d12frosted d12frosted commented Oct 11, 2020

Motivation for this change

See my comment in #1149 (comment)

See it in action:

org-roam-alias-tag-test

Comment thread org-roam.el
index)))

;;;; dealing with file-wide properties
(defun org-roam--set-global-prop (name value)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually have an idea how we can remove this function in the future. Since 9.4 is out, and files also have properties, we can gradually migrate to storing ROAM_xxx as properties (aka :ROAM_xxx:) instead of 'settings' (aka #+ROAM_xxx).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is fine, I think we can file this as a feature request (supporting fetching from property drawers).

Comment thread org-roam.el
Comment thread org-roam.el
Comment thread org-roam.el
"ROAM_ALIAS"
(combine-and-quote-strings (delete alias aliases)))
(org-roam-db--update-file (buffer-file-name (buffer-base-buffer))))
(user-error "No aliases to delete")))
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I just want to avoid empty completion list. Plus required-match...

Comment thread org-roam.el Outdated
Return added tag."
(interactive)
(unless org-roam-mode (org-roam-mode))
(let* ((all-tags
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I just want to provide completion from existing tags.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(org-roam-db--get-tags) gives you what you want, and also applies the distinct op via SQL, which should be faster.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow, didn't know about this. Really works great and simplifies implementation 🙇

Comment thread org-roam.el
Comment thread org-roam.el
index)))

;;;; dealing with file-wide properties
(defun org-roam--set-global-prop (name value)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is fine, I think we can file this as a feature request (supporting fetching from property drawers).

Comment thread org-roam.el
Comment thread org-roam.el
Comment thread org-roam.el Outdated
Comment on lines +884 to +890
(let ((found))
(while (not (or found (eobp)))
(beginning-of-line)
(if (or (looking-at "^#")
(looking-at "^:"))
(line-move 1 t)
(setq found t)))
Copy link
Copy Markdown
Member

@jethrokuan jethrokuan Oct 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
(let ((found))
(while (not (or found (eobp)))
(beginning-of-line)
(if (or (looking-at "^#")
(looking-at "^:"))
(line-move 1 t)
(setq found t)))
(while (and (not (eobp))
(looking-at "^[#:]"))
(if (save-excursion (end-of-line) (eobp))
(progn
(end-of-line)
(insert "\n"))
(forward-line)
(beginning-of-line)))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for explain this diff:

  1. line-move runs a lot of things, and depends on some user config options (like whether to create new lines). forward-line is the simplest way to go one line down

  2. We need to explicitly check the boundary condition so we know when to create a new line, since forward-line does not error

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense! Thank you.

Comment thread org-roam.el
Comment thread org-roam.el
(org-roam--set-global-prop
"ROAM_ALIAS"
(combine-and-quote-strings (delete alias aliases)))
(org-roam-db--update-file (buffer-file-name (buffer-base-buffer))))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can leave out the argument, it defaults to the current buffer.

Comment thread org-roam.el Outdated
Return added tag."
(interactive)
(unless org-roam-mode (org-roam-mode))
(let* ((all-tags
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(org-roam-db--get-tags) gives you what you want, and also applies the distinct op via SQL, which should be faster.

@d12frosted d12frosted force-pushed the feature/tags-and-alias-management branch from 0d609fb to b273bdf Compare October 12, 2020 06:11
@d12frosted
Copy link
Copy Markdown
Contributor Author

Updated PR with suggestions @jethrokuan :)

@jethrokuan jethrokuan merged commit 5348654 into org-roam:master Oct 12, 2020
@d12frosted d12frosted deleted the feature/tags-and-alias-management branch October 12, 2020 06:56
@d12frosted d12frosted mentioned this pull request Oct 12, 2020
1 task
@TimQuelch
Copy link
Copy Markdown
Contributor

TimQuelch commented Nov 20, 2020

I've noticed these functions use uppercase ROAM_TAGS and ROAM_ALIAS. Should these instead be lowercase to be more consistent (#769)?

I've got a branch on my fork where this is the case, I can submit a quick PR if you like. (there's also a patch in there to maintain the original case of the property, so people who already have upper case ones won't really be affected)

@d12frosted
Copy link
Copy Markdown
Contributor Author

Oh wow, didn't know that the new convention is to use lowercase. Will send PR to follow this convention in new functions.

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Jun 7, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants