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

Why the order is not respected ? #32

Open
Cletip opened this issue Jan 5, 2024 · 6 comments
Open

Why the order is not respected ? #32

Cletip opened this issue Jan 5, 2024 · 6 comments

Comments

@Cletip
Copy link

Cletip commented Jan 5, 2024

Hi there!

Related to #28 : the proposed solution does not work :(

When I try to do this :

(setq consult-org-roam-buffer-enabled nil)
(setq org-roam-node-default-sort 'file-atime)

I always get the same order, regardless of whether the consult-org-roam-buffer-enabled variable is nil or t by the way.

I don't understand why I have this behaviour. The desired behaviour would be when I don't activate consult-org-roam (I use vertico-prescient, and I simply have a sort by date of node visited, which is very good) but I no longer have the preview (which is disturbing, because I really love that one).

Is there a solution? Have I misunderstood something?

Thank you in advance for your reply.

@Cletip
Copy link
Author

Cletip commented May 14, 2024

I think I've found the solution:
remove / comment the line :
:sort nil ;; cands are already sorted
from the consult-org-roam-node-read function, in the call to the consult--read function.

@jgru
Copy link
Owner

jgru commented May 14, 2024

Hi @Cletip ,

thanks for the update. Really nice that you found it. Do you think this should be changed or maybe exposed to the user to customize that?

Best,
jgru

@Cletip
Copy link
Author

Cletip commented May 14, 2024

Hello @jgru !

I think you should delete the line: if it's nil, then I don't think it's useful (because it's nil, but the order was modified because of the nil : it's not the behavior that we want, we want to not touch the order !). On the other hand, I don't know how the underlying of consult, so you'd have to check that it doesn't interfere with other things (and explain that the consult-org-roam-buffer-enabled variable just enables org-roam buffers in consult-buffer, and not doesn't change the order, for example).

So yes, to sum up, delete the line (and maybe explain the consult-org-roam-buffer-enabled variable in more detail in the readme ?).

Best too,
Cletip

@akashpal-21
Copy link

akashpal-21 commented May 20, 2024

I was looking for this too - I realised this was broken, when trying to test a cache to the #'org-roam-node-read--completions
It was turned off here 3c2701e
I am unsure how org-roam's sorting would clash with it -- if user sets a custom sort function. Will consult's sort override it ?

It was turned off in the first place because user complained that org-roam's sort was failing and being overriden -- if I am not incorrect?

If I may ask why is the history commented out?

Thanks.

@akashpal-21
Copy link

akashpal-21 commented May 20, 2024

Also I want to note that I think org-roam's atime implementation is broken. the atime is stored in a node -- but nodes are not updated unless some change is pushed -- so the atime sort is bound to fail. atime and mtime are functionally the same.

For test,

select files.atime from files where files.title like "%filetitle%";

(org-roam-db-query
 [:select [files:atime]
	  :from files
	  :where (like files:title "%filetitle%")])

replace filetitle with a title of a org file in your system tracked by org roam

Youll notice that the atime doesn't change if you simply visit the file. So its broken because the atime sort is broken in org-roam and users notice this as a fault of consult-org-roam because it relies on sort passed by org-roam -- my guess.

Please correct me if you have different result in your query. But if this is true -- it should default to nil - and force the user to either roll out their own sort-fn or get the sort function of their minibuffer by setting :sort to t -- which will override org-roam's.

@akashpal-21
Copy link

akashpal-21 commented May 20, 2024

My hunch was correct - it is indeed calling vertico-sort when :sort is set to t --

Debugger entered--entering a function:
* vertico-sort-history-length-alpha((#("             20001                                ..." 0 13 (node #s(org-roam-node :file "/home/akash/Desktop/test2/test/20001.org" :file-title "20001" :file-hash nil :file-atime (26185 49027 710064 549000) :file-mtime (26185 49027 710064 549000) :id "20240519T142944.642943" :level 0 :point 1 :todo nil :priority nil :scheduled nil :deadline nil :title "20001" :properties (("CATEGORY" . "20001") ("ID" . "20240519T142944.642943") ("BLOCKED" . "") ("FILE" . "/home/akash/Desktop/test2/test/20001.org") ("PRIORITY" . "B")) :olp nil :tags nil :aliases nil :refs nil)) 13 18 (node #s(org-roam-node :file "/home/akash/Desktop/test2/test/20001.org" :file-title "20001" :file-hash nil :file-atime (26185 49027 710064 549000) :file-mtime (26185 49027 710064 549000) :id "20240519T142944.642943" :level 0 :point 1 :todo nil :priority nil :scheduled nil :deadline nil :title "20001" :properties (("CATEGORY" . "20001") ("ID" . "20240519T142944.642943") ("BLOCKED" . "") ("FILE" . "/home/akash/Desktop/test2/test/20001.org") ("PRIORITY" . "B")) :olp nil :tags nil :aliases nil :refs nil) face bold) 18 130 (node #s(org-roam-node :file "/home/akash/Desktop/test2/test/20001.org" :file-title "20001" :file-hash nil :file-atime (26185 49027 710064 549000) :file-mtime (26185 49027 710064 549000) :id "20240519T142944.642943" :level 0 :point 1 :todo nil :priority nil :scheduled nil :deadline nil :title "20001" :properties (("CATEGORY" . "20001") ("ID" . "20240519T142944.642943") ("BLOCKED" . "") ("FILE" . "/home/akash/Desktop/test2/test/20001.org") ("PRIORITY" . "B")) :olp nil :tags nil :aliases nil :refs nil))) ... ... ... ... ... ... ... ... ... ... ... ... ... ...))
  vertico--recompute(0 "")
  vertico--update()
  vertico--prepare()
 
  consult--read((... ... ... ... ... ... ... ... ... ... ... ... ... ... ...) :prompt "Node: " :initial nil :sort t :require-match nil :category org-roam-node :history org-roam-node-history :state ...)
  (let* ((nodes (org-roam-node-read--completions filter-fn sort-fn)) (prompt (or prompt "Node: ")) (state-func (if nodes (progn (consult-org-roam--node-preview)))) (node (consult--read nodes :prompt prompt :initial initial-input :sort t :require-match require-match :category 'org-roam-node :history 'org-roam-node-history :state state-func :annotate #'(lambda (title) (funcall org-roam-node-annotation-function (get-text-property 0 ... title))) :lookup #'(lambda (selected candidates input narrow) (alist-get selected candidates input nil #'equal))))) (if (progn (and (memq (type-of node) cl-struct-org-roam-node-tags) t)) (progn node) (progn (record 'org-roam-node nil nil nil nil nil nil nil nil nil nil nil nil node nil nil nil nil nil))))
  consult-org-roam-node-read(nil (lambda (node) (custom/org-roam-node--filter-by-entries node nil org-roam-filter-by-entries)) nil)
  apply(consult-org-roam-node-read (nil (lambda (node) (custom/org-roam-node--filter-by-entries node nil org-roam-filter-by-entries)) nil))
  org-roam-node-read(nil (lambda (node) (custom/org-roam-node--filter-by-entries node nil org-roam-filter-by-entries)) nil)

Should clarify this issue in the visible place to clear confusion,

And also let users configure it through a defcustom maybe if possible.

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

No branches or pull requests

3 participants