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

Customize initial input #130

Merged
merged 6 commits into from
May 11, 2021

Conversation

publicimageltd
Copy link
Contributor

@publicimageltd publicimageltd commented May 9, 2021

Here's the PR for the customizable initial input. AFAIU, all other occurences of strings like "has:pdf" in the source code are used for actually inserting these strings in the candidates, so they should not be affected by that change.

Closes #129.

Copy link
Contributor

@bdarcus bdarcus left a comment

Choose a reason for hiding this comment

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

I added one minor suggestion (which you can commit by just clicking to accept it), and added a link to the issue to your post.

But the bigger question is I'm not clear on the use case, given my response to #129.

Can you clarify?

Comment on lines 101 to 104
'((pdf . "has:pdf")
(note . "has:note")
(link . "has:link")
(source . "has:link\\|has:pdf"))

This comment was marked as off-topic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Space is added in bibtex-actions-read, which see.

As to the use case, I thought the idea is to just make it customizable anyways. It's Emacs, after all. 😄

For me, at least, there is actually a good use case: I use bibtex-action-open-notes with the fallback mechanism provided by bibtex-completion, which creates a note file if there is none. So the initial input is useless, since for me, bibtex-action-open-notes is supposed to open or create a note with one command (which is very handy, btw). Another use case would be the ability to define what "source" actually means (as discussed in #121 ).

But TBH, I cannot imagine many more use cases right now. 🤷 So it's up to the maintainer!

Copy link
Contributor

@bdarcus bdarcus May 9, 2021

Choose a reason for hiding this comment

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

Space is added in bibtex-actions-read, which see.

Oops! I just hid the comment then.

As to the use case, I thought the idea is to just make it customizable anyways. It's Emacs, after all.

Well, no; customization for the sake of it would be overkill.

But when there's need, that's another matter.

For me, at least, there is actually a good use case: I use bibtex-action-open-notes with the fallback mechanism provided by bibtex-completion, which creates a note file if there is none.

Ooh, that's actually one I care about too; I totally forgot about that!

This does raise a larger design question, which is what @apc and I were going back-and-forth on in #118.

We settled on the initial-input approach, because the goal was filtering, but to make it easy for the user to effectively turn off or modify the filtering mid-stream; in this case to delete the initial-input.

This is not only because of cases like this, but also because with embark, you may open your library with one action in mind, but change your mind, and so need access to a wider list of candidates.

So the big question: did we make the right decision? And if yes, is this the right addition to this?

I'm inclined to say yes on both counts, but curious what you and @apc think.

So the initial input is useless, since for me, bibtex-action-open-notes is supposed to open or create a note with one command (which is very handy, btw). Another use case would be the ability to define what "source" actually means (as discussed in #121 ).

But TBH, I cannot imagine many more use cases right now. So it's up to the maintainer!

No, that's a good enough reason, per above.

@bdarcus
Copy link
Contributor

bdarcus commented May 10, 2021

It does occur to me, though, that default value for notes should be nil.

This is largely because they are different than the others, for the reason you note.

@publicimageltd
Copy link
Contributor Author

So you approve the PR, right? Nice! Only thing I forgot, as usual, is the readme.

As to the default value for notes, I would not set it to nil, as that value requires extra configuration of bibtex-completions and might be confusing otherwise. So in my imagined case, the user notes that it is cumbersome to delete the initial input and then begins to look for customization possibilities.

Regarding #118, I also think that the initial input solution is quite elegant to solve the problem of filtering: it can be easily changed and since it is visible, it is understood immediately that some kind of filtering is active. No need to remember some keys.

@bdarcus
Copy link
Contributor

bdarcus commented May 10, 2021

Let's see if any feedback comes in from others. I know @apc is in the US, for example.

Your solution is the most general, but if I had known about this use case upfront, I probably would have suggested starting with a boolean specific to open-notes, so users can just do something like:

(setq bibtex-actions-prefilter-notes nil)

What would, for example, the README addition be for this solution, for example, for the user that doesn't use the defcustom UI?

Edit: the user would have to redefine the entire alist; right? So for our preference, would have to add this to the init?

(setq bibtex-actions-initial-inputs
  '((pdf    . "has:pdf")
    (note   . nil)
    (link   . "has:link")
    (source . "has:link\\|has:pdf"))

@apc
Copy link
Contributor

apc commented May 10, 2021

I'm inclined to agree with @publicimageltd that setting the default value of 'notes' to nil might be confusing. I think it'd be best to have all bibtex-actions-open- commands treated the same way.

I think it might be helpful to emphasize that the user may want to set a 'default' entry point to her list of references (I essentially treat bibtex-actions-insert-citations that way, and it's the one thing I've bound in my global keymap so far). Depending on use-case, this may be bibtex-actions-open-pdf or bibtex-actions-open-entry etc., but it would make sense to have one's main entry point something that gives you access to one's entire list of references. For then the user can pull the list of references from her .bib files and, using embark, call bibtex-actions-open-notes if the user wants to just add a note to an entry without any notes, say. In other words, I think it'd be good to think of the commands bibtex-actions-open- as functioning slightly differently depending on the context. When the user calls bibtex-actions-open-, it makes sense to be presented with a list of things for which the open action makes sense. When the user calls the 'default' entry point, where plausibly she will know that the entry in question does not have a note associated with it, and then calls bibtex-actions-open-, the prefilters aren't relevant anymore.

So ideally, you would have a single command for each of pdf, notes, and link, that would (1) add a corresponding pdf, note, or even link to KEY when called with input KEY and (2) list entries with associated pdf, notes, or link, when called without any input key. (This makes me wonder whether there's a way of merging the functionality of bibtex-actions-add-pdf-to-library into that of bibtex-actions-open-pdf...)

@bdarcus
Copy link
Contributor

bdarcus commented May 10, 2021

I'm inclined to agree with @publicimageltd that setting the default value of 'notes' to nil might be confusing. I think it'd be best to have all bibtex-actions-open- commands treated the same way.

To be clear, we all agree on this, for the reasons @publicimageltd outlined.

So ideally, you would have a single command for each of pdf, notes, and link, that would (1) add a corresponding pdf, note, or even link to KEY when called with input KEY and (2) list entries with associated pdf, notes, or link, when called without any input key.

Before I wrap my head around this, I just want to make sure we're all on the same page WRT to notes.

Are you aware of this @apc?

https://github.com/tmalsburg/helm-bibtex#notes

This is what @publicimageltd is referring to.

I actually don't remember whether the "add new notes if none exist" functionality is built in to bibtex-completion, or it's automatically added when you use org-roam-bibtex.

But in any case, the config a user has to do it minimal.

I do agree it's not on by default, though, which is why it makes sense to filter by default.

I think our question for this PR is twofold:

  1. which initial-value options need to be configurable. Clearly notes, but the others?
  2. how should they be configured; alist with strings vs simple boolean, etc.

@apc
Copy link
Contributor

apc commented May 10, 2021

I'm inclined to agree with @publicimageltd that setting the default value of 'notes' to nil might be confusing. I think it'd be best to have all bibtex-actions-open- commands treated the same way.

To be clear, we all agree on this, for the reasons @publicimageltd outlined.

So ideally, you would have a single command for each of pdf, notes, and link, that would (1) add a corresponding pdf, note, or even link to KEY when called with input KEY and (2) list entries with associated pdf, notes, or link, when called without any input key.

Before I wrap my head around this, I just want to make sure we're all on the same page WRT to notes.

Are you aware of this @apc?

https://github.com/tmalsburg/helm-bibtex#notes

This is what @publicimageltd is referring to.

I'm not super clear on what you mean by 'this' here. Is it the option to work with a single note file or with multiple notes files? Or is it the functionality to create a note if none exists? I was aware of both, but I took @publicimageltd to be mostly talking about the latter. But maybe I'm misunderstanding something here?

I actually don't remember whether the "add new notes if none exist" functionality is built in to bibtex-completion, or it's automatically added when you use org-roam-bibtex.

No, it is built into bibtex-completion. See here.

But in any case, the config a user has to do it minimal.

I do agree it's not on by default, though, which is why it makes sense to filter by default.

What is not on by default? Sorry, I think I'm only half following here. Maybe I should be looking more carefully at the PR...

I think our question for this PR is twofold:

  1. which initial-value options need to be configurable. Clearly notes, but the others?
  2. how should they be configured; alist with strings vs simple boolean, etc.

I see little harm in having the most general functionality available to users—let anyone customize what, if anything, they want as filters for the relevant actions, but give 'sane' defaults. I cannot think of a reason to use anything other than `nil' or one of the original values as initial filters, but I imagine most people will not need to tweak the defaults, so I say go for a "if you want to mess with these defaults, here, you can do whatever you want" approach. ;)

@bdarcus
Copy link
Contributor

bdarcus commented May 10, 2021

But in any case, the config a user has to do it minimal.
I do agree it's not on by default, though, which is why it makes sense to filter by default.

What is not on by default? Sorry, I think I'm only half following here. Maybe I should be looking more carefully at the PR...

Sorry. I complain about non-specific referents with my students all the time!

By "this" I mean automatic note creation.

No, it is built into bibtex-completion. See here.

So not only is it included, but it's on my default?

If that's the case, then I go back to my earlier point: I don't think we should filter by default.

In any case, I think we should base whether we filter by notes or not based on what's default.

If automatic note creation is default, then initial-value should be nil by default.

I think our question for this PR is twofold:

  1. which initial-value options need to be configurable. Clearly notes, but the others?
  2. how should they be configured; alist with strings vs simple boolean, etc.

I see little harm in having the most general functionality available to users—let anyone customize what, if anything, they want as filters for the relevant actions, but give 'sane' defaults. I cannot think of a reason to use anything other than `nil' or one of the original values as initial filters, but I imagine most people will not need to tweak the defaults, so I say go for a "if you want to mess with these defaults, here, you can do whatever you want" approach. ;)

OK, then. Thanks.

Any further thoughts @publicimageltd?

@apc
Copy link
Contributor

apc commented May 10, 2021

So ideally, you would have a single command for each of pdf, notes, and link, that would (1) add a corresponding pdf, note, or even link to KEY when called with input KEY and (2) list entries with associated pdf, notes, or link, when called without any input key.

Before I wrap my head around this, I just want to make sure we're all on the same page WRT to notes.

Oh, and @bdarcus I didn't mean to suggest any changes. I was just thinking that there were two options for how to think of the filtering etc.

  1. You could have two commands for each of pdf, note, and link: one that adds and one that opens. This would essentially amount to adding an analogue of bibtex-actions-add-pdf-to-library for each of note and link. Filtering would make sense by default for the open command and not for the add command.
  2. Have a single 'open or edit/add if none exist' command for each of note, link, and pdf, and think of two different uses for each of them: when called with a KEY argument (what you effectively do, AFAIU, when you call these commands using embark from a selection in the minibuffer), the filtering mechanism is irrelevant and thus need not be modified; when called without a KEY argument, the filtering makes sense, and thus should be left as is (by default).

I was suggesting that the better approach is 2. (At first, I thought this could, but need not, involve making bibtex-actions-add-pdf-to-library obsolete, and having bibtex-actions-open-pdf have a similar 'adding-if-none-present' fallback mechanism. But I think that's a mistake, since you may want more than one pdf file associated with a given entry, and that possibility is already taken into account by bibtex-completion.)

@apc
Copy link
Contributor

apc commented May 10, 2021

But in any case, the config a user has to do it minimal.
I do agree it's not on by default, though, which is why it makes sense to filter by default.

What is not on by default? Sorry, I think I'm only half following here. Maybe I should be looking more carefully at the PR...

Sorry. I complain about non-specific referents with my students all the time!

😆

So not only is it included, but it's on my default?

I think so, yes.

If that's the case, then I go back to my earlier point: I don't think we should filter by default.

I ultimately think it's fine either way. But, FWIW, even taking the bibtex-completion defaults into account, I think once you add reasonable filtering defaults to some, it makes sense to add them to all. It's simpler and you don't need to explain why you're handling notes differently. Especially since you already have a way of viewing your entire list of entries, without filtering, with bibtex-actions-insert-citations or -open-entry, and depending on how you end up handling #121, you will also have the option of having bibtex-actions-open give you the entire list of entries.

That said: perhaps my mistake here is thinking that how you get to the list is essentially irrelevant because of embark, and not properly keeping in mind that someone may just want to create a note by default from the list of entries by simply pressing RET rather than having to do so via embark...

@bdarcus
Copy link
Contributor

bdarcus commented May 10, 2021

Let me just explain why I initially added has:pdf and such to begin with, which is what all of this builds on.

I was consistently annoyed when wanting to open a document (pdf or link) that I didn't know which entries that action would return a result on.

Aside: helm-bibtex and ivy-bibtex don't really have this issue because of their different design.

So I wanted to filter on presence of different related items, and also for the UI to reflect that.

With that, I would no longer be surprised.

But you, @apc, pointed out the next step was to incorporate this into the relevant commands.

However, bibtex-actions-open-notes will never return nil, so it seems wrong to me to filter those, when the user can do that manually; in my case at least simply by doing :n. That's more efficient, than to have to delete the initial input to get the full library.

@apc
Copy link
Contributor

apc commented May 10, 2021

I see. That makes sense.

I was probably thinking about things differently. Since you already have indicators that will tell you which entries have pdfs, say, it never occurred to me you would call bibtex-actions-open-pdf on an entry you know doesn't have a pdf. The way I was thinking about it, it's only if you knew about the fallback mechanism that it made sense to call bibtex-actions-open-notes on an entry which the list of candidates tells you has no note to open.

I guess there are two ways of thinking about what's best to do as a default: have it so that there's as little friction as possible for helm-bibtex and ivy-bibtex users to transition to using bibtex-actions or have it so that there's as little friction as possible for new users. It is of course hard to put oneself in the position of a new user here, but my hunch was that a new user would think there was something wrong with the package if she gets filtering with -open-pdf and -open-link but not with -open-notes. But I don't feel very confident about any of this, so... ¯\_(ツ)_/¯

@publicimageltd
Copy link
Contributor Author

I'm kind of lost in this discussion now, to be honest. So what about utmost explicitness?

bibtex-actions-open-or-create-notes with no initial input and explicit reliance on the fallback mechanism;

bibtex-actions-open-xxxx with prefiltering and predefined action (open pdf, open notes file, scratch your head, whatever);

bibtex-actions-open-source with no filtering and a predefined action what to do with the result, but no creation of new files whatsoever.

@bdarcus
Copy link
Contributor

bdarcus commented May 10, 2021

I'm kind of lost in this discussion now, to be honest.

Sorry. This does end up somewhat complicated, in part because our goal (or at least mine) is simple and clear for users.

I can make decisions and commit them on this branch for final review, hopefully later today, but in the meantime I think I only have one question for you, @publicimageltd:

Why would someone want to change the value of the pdf initial-value, rather than just turn it off? So substantively, why is the value of the alist entries a string, rather than a boolean?

Is it we don't really have a reason ATM, but it's a more general solution so gives more flexibility in the future?

I do think we need to turn off note filtering by default, but I think I'll just need to decide on that.

In any case, I can include the README content that explains all this, hopefully simply and clearly, and you all can see what you think.

So what about utmost explicitness?

I don't think we need to go so far as to add commands :-)

@bdarcus
Copy link
Contributor

bdarcus commented May 10, 2021

Actually, here's a start of possible README content:


By default, bibtex-actions will, assuming you are using orderless or prescient to filter candidates, pre-filter entries for the following commands.

  1. bibtex-actions-open: pre-narrows the list to those which have associated pdf or links
  2. bibtex-actions-open-link: pre-narrows the list to those which have associated links
  3. bibtex-actions-open-pdf: -pre-narrows the list to those which have associated pdf(s)

That is, upon running the command, an initial-input value will be inserted to narrow the results. You can also delete that if you prefer to see the full list of candidates.

By default, pre-filtering of bibtex-actions-open-notes is off, because the command by default will create a new note if none is available, and therefore it makes sense to have access to your full library. But you can customize this to pre-filter if you prefer.

If you want to modify those values, or remove them entirely, you can set bibtex-actions-initial-inputs like so; in this case turning off pre-filtering for bibtex-actions-open-pdf:

(setq bibtex-actions-initial-inputs
  '((pdf    . nil)
    (note   . nil)
    (link   . "has:link")
    (source . "has:link\\|has:pdf"))

@publicimageltd
Copy link
Contributor Author

Why would someone want to change the value of the pdf initial-value, rather than just turn it off? So substantively, why is the
value of the alist entries a string, rather than a boolean?

Actually I think that one advantage of the current PR is that the options are much more explicit than a simple nil or t. Nil is nil, ok, but if there is a string, one can immediately see what it is about -- and use one's knowledge about how such an input behaves. So even if it is true that, in the real world, we could simply replace the string with a t, the proposed structure is much more explicit and easier to understand, IMHO.

don't think we need to go so far as to add commands :-)

But it could solve the slight inconsistency we are struggling with, which is, at least in one view, basically the fact that there is one exception (notes) which forces us to add customization for every other open command. 🤷‍♂️

In any case, the README proposal is clear and understandable, go for it!

@bdarcus
Copy link
Contributor

bdarcus commented May 10, 2021

But it could solve the slight inconsistency we are struggling with ...

This is partly another consequence of me trying to simplify naming compared to bibtex-completion.

That function there is called bibtex-completion-edit-notes. But I noted it just as likely that one simply wants to view the notes. So "open" applies to viewing and editing/creating.

OTOH, in hindsight, you could argue that opening PDFs or links is a view--only option, so could have had command names like bibtex-actions-view-pdf instead.

Hmm ... I guess there's some confusion around these verbs: open, view, edit (and add) in general.

bibtex-actions.el Outdated Show resolved Hide resolved
@bdarcus bdarcus merged commit d994b7e into emacs-citar:main May 11, 2021
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

Successfully merging this pull request may close these issues.

Make initial value for the different actions customizable
3 participants