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

file indicator incorrectly indicating #776

Open
stefangroha opened this issue Apr 12, 2023 · 19 comments
Open

file indicator incorrectly indicating #776

stefangroha opened this issue Apr 12, 2023 · 19 comments
Labels
bug Something isn't working

Comments

@stefangroha
Copy link

Describe the bug
I use Spacemacs and Mendeley and I am trying to directly access the pdfs of files through citar, however when I try to set

(setq citar-file-parser-functions
  '(citar-file-parser-default
    citar-file-parser-triplet))

as described in the documentation, I get the following error:

citar--check-configuration: ‘citar-file-parser-functions’ should be a list of functions: '(citar-file-parser-default citar-file-parser-triplet)

If I remove this snippet I do not see the link to the file when clicking on citar links, however when trying to insert links it does show me that a file is there.

To Reproduce
Steps to reproduce the behavior:
My citar configuration is (except for the above snippet) is

;; citar
(use-package citar
    :no-require
    :custom
        (org-cite-global-bibliography '("~/library.bib"))
        (org-cite-insert-processor 'citar)
        (org-cite-follow-processor 'citar)
        (org-cite-activate-processor 'citar)
        (citar-bibliography org-cite-global-bibliography)
    :bind
         (:map org-mode-map :package org ("C-c b" . #'org-cite-insert)))

(use-package citar-org-roam
        :after (citar org-roam)
        :config (citar-org-roam-mode))

(use-package citar-embark
       :after citar embark
       :no-require
       :config (citar-embark-mode))
  1. insert citation
  2. click citation
  3. error message

Expected behavior
A buffer should open with the choice for weblinks, notes and direct links.

Emacs version:
28.2

@stefangroha stefangroha added the bug Something isn't working label Apr 12, 2023
@bdarcus
Copy link
Contributor

bdarcus commented Apr 12, 2023

Something is weird. What you're trying to set is the default value?

@stefangroha
Copy link
Author

Maybe I am confused by the documentation, I want to mainly set the triplet value, but I just copy and pasted this in case I want to use zotero as well.

@bdarcus
Copy link
Contributor

bdarcus commented Apr 12, 2023

You shouldn't have to set that; I never do, and just use the default.

The way the parsing code works is it runs through the list of functions until it gets a result. So there's no harm in having both default.

I mostly use Doom, but have a small experimental init over here, where it should all work correctly (see init-biblio.el in particular).

https://gitlab.com/bdarcus/emacs-elp

@stefangroha
Copy link
Author

Without setting the variable I can see that there is a file associated with the bibliography (when trying to insert), however when clicking the citation I only see the weblink and option to create a note and no option to open the pdf. As the file field in the bib-file is set in Mendeley format (:/path/to/pdf:pdf), I thought this part of the documentation would help.

@bdarcus
Copy link
Contributor

bdarcus commented Apr 12, 2023

Let's start from the beginning ...

OK, I see a problem now :-)

So the parser code and at-point functionality is probably confusing the issue.

This is what I see:

If I do M-x citar-open, and then filter the candidates to those that should have files associated with them, I see:

image

If I select the first, I see this:

image

I think that's what you're seeing also?

So somehow a bug crept in somewhere.

Two possibilities, which I guess aren't mutually exclusive:

  1. the indicators are wrong, incorrectly indicating there are files, when there are not
  2. the functions that list the files in the second step is wrong, incorrectly omitting them

You are saying it's 2, but we'd want to confirm it is, and it's not 1, because it's different code.

@bdarcus
Copy link
Contributor

bdarcus commented Apr 12, 2023

Looking at the biblatex file for that entry, it has this as the value of file:

/home/bruce/Zotero/storage/WZB4FH9E/1405ba54-7f4e-11ea-8013-1b6da0e4a2b7_story.html

The reason that file is not showing up is because the file doesn't actually exist (long story)! So the problem there is 1 above.

But now I need to figure out why the indicator is wrong.

@stefangroha
Copy link
Author

stefangroha commented Apr 12, 2023

That is exactly what I am seeing, however for me in the biblatex file I do see a link to a pdf:

file = {:Users/smg/Library/Application Support/Mendeley Desktop/Downloaded/Heinz et al. - 2022 - Liver Colonization by Colorectal Cancer Metastases Requires YAP-Controlled Plasticity at the Micrometastatic Stage.pdf:pdf}

which indeed exists at this location.

@bdarcus
Copy link
Contributor

bdarcus commented Apr 12, 2023

Can you confirm the file is actually there, though, with that precise path? The code won't list it if it can't find it.

Not sure what OS you're on (Mac OS?), but this is what I see on Linux from the console:

ls /home/bruce/Zotero/storage/WZB4FH9E/1405ba54-7f4e-11ea-8013-1b6da0e4a2b7_story.html
ls: cannot access '/home/bruce/Zotero/storage/WZB4FH9E/1405ba54-7f4e-11ea-8013-1b6da0e4a2b7_story.html': No such file or directory

@bdarcus bdarcus changed the title citar-file-parser-functions not working file indicator incorrectly is incorrect Apr 12, 2023
@stefangroha
Copy link
Author

stefangroha commented Apr 12, 2023

Ah, thank you so much, the file path from the Mendeley export did not have the initial "/' (before Users), which was of course needed. It does work now, if I change the bib file to the correct link.

@bdarcus
Copy link
Contributor

bdarcus commented Apr 12, 2023

I'm going to reopen this, then, as in that case, the UI should not indicate it as present.

@bdarcus bdarcus reopened this Apr 12, 2023
@bdarcus
Copy link
Contributor

bdarcus commented Apr 12, 2023

@roshanshariff - before I dig into fixing this, anything I should know?

Basically, right now, citar-has-files (well, really citar-file--has-file-field) does not check if a file exists, but the open commands do.

So one can get an the indicator for an entry say it has a file, but it actually doesn't.

Is that perhaps for performance reasons, or is this an actual bug/oversight?

I can modify that function something like this:

  (when citar-file-variable
    (lambda (citekey)
      (and (citar-file--parse-file-field
            (or (citar-get-value citar-file-variable citekey) "") nil) t))))

... but that would also require changes in citar-file--parse-file-field.

@bdarcus bdarcus changed the title file indicator incorrectly is incorrect file indicator incorrectly indicating Apr 13, 2023
@roshanshariff
Copy link
Collaborator

roshanshariff commented Apr 13, 2023

@bdarcus, this was an intentional change for performance reasons, so that showing the indicator doesn't require parsing the file fields, checking if files exist, and so on. Since this function is called for every bibliography item every time you call any citar command, that would be prohibitively slow.

Instead, it just checks if each entry has a file field. I remember that we discussed this in the relevant PR, but can't find the link right now. I think we also agreed that it's a reasonable user experience, since the item is "supposed" to have an attached file, and if you get an error when you open it you can investigate why it's not there.

@bdarcus
Copy link
Contributor

bdarcus commented Apr 13, 2023

I thought that might be the case; sounds familiar.

Except the file isn't listed in the related resources UI, so you don't get the error; just a discrepancy between the two UI views.

So maybe we should list them even if in error?

And do not the files in directories get checked?

@roshanshariff
Copy link
Collaborator

So maybe we should list them even if in error?

Yes, I think all the files in the file field should be listed even if they don't exist. This is going to take some thinking, because currently the file field parser functions use the non-existence of the file to filter out bad parses (e.g. treating a triplet-style file field as a plain filename). But perhaps we can refine the parsing functions so we don't have to do this.

And do not the files in directories get checked?

Yes, for files that are found in the library path (i.e. not listed in the file field in a bibliography) the indicator does reflect whether the file actually exists.

@bdarcus
Copy link
Contributor

bdarcus commented Apr 13, 2023

Yes, for files that are found in the library path (i.e. not listed in the file field in a bibliography) the indicator does reflect whether the file actually exists.

Is that faster than splitting a string and checking the path(s)?

I guess the confusing part is also some files are checked differently than other files; that the indicator means different things depending.

@roshanshariff
Copy link
Collaborator

The difference is that, for library files, we can just scan the directory once and figure out which keys have files, because each filename starts with the key. Not having a file for a key is not an error; it just means you don't have an attached file. On the other hand, explicitly listing a non-existent file in the bibliography seems like a data error. Perhaps we could handle it by having a linting function that checks for non-existent files and offers to remove them, which you would run when needed?

From a performance point of view, the files listed in file fields needn't be in any particular location, so we can't scan a directory beforehand. So we take the existence of a file field as a signal that the item has attached files.

@bdarcus
Copy link
Contributor

bdarcus commented Apr 13, 2023

On the other hand, explicitly listing a non-existent file in the bibliography seems like a data error. Perhaps we could handle it by having a linting function that checks for non-existent files and offers to remove them, which you would run when needed?

Makes sense.

I was also wondering about using faces to distinguish files that are confirmed to exist, and those that are confirmed to not exist, or unconfirmed?

But maybe that gets too complicated.

@roshanshariff
Copy link
Collaborator

If I remember correctly, annotation and affixation functions are only called for visible candidates, not all candidates. So you could do (relatively) expensive work in those functions. But they can only change the visible representation of each candidate (like icons and other prefixes/suffixes, but not faces). They can't modify the actual candidate text like the searchable has:files tag.

And you're right, doing all this adds complexity which we'd have to weigh against the usefulness of the feature.

@bdarcus
Copy link
Contributor

bdarcus commented Apr 13, 2023

Yes, that also makes sense. Annotations or affixations might be appropriate for that case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants