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

Copy cite command should respect preferences #10615

Closed
koppor opened this issue Nov 2, 2023 · 25 comments · Fixed by #10707
Closed

Copy cite command should respect preferences #10615

koppor opened this issue Nov 2, 2023 · 25 comments · Fixed by #10707
Labels
bug Confirmed bugs or reports that are very likely to be bugs good first issue An issue intended for project-newcomers. Varies in difficulty. preferences
Milestone

Comments

@koppor
Copy link
Member

koppor commented Nov 2, 2023

#10303 introduced a new cite command:

image

It is parsed at org.jabref.gui.push.AbstractPushToApplication#dissectCiteCommand.

However, in org.jabref.gui.edit.CopyMoreAction#copyCiteKey, this pattern is not used.

This leads to \cite{key1,key2}{Khalaf2007} for key Khalaf2007.

284869839-992b6afe-6efc-4a6d-811f-0641c392a50d

Implementation

  1. Rename org.jabref.logic.citationkeypattern.CitationKeyPatternPreferences to CitationKeyGenerationPreferences.
  2. Modify org.jabref.preferences.ExternalApplicationsPreferences#getCiteCommand to return a record having prefix, delimiter, suffix as string. There, the value is constructed based on the code at org.jabref.gui.push.AbstractPushToApplication#dissectCiteCommand. - No need to introduce caching there. That could be done as next step. (This code does not run for a long time, so no performance issues expected). Name the record CitationCommandStringPreferences.
  3. Add test cases testing (wrong) preference \cite. It should work nevertheless. It should use , as delimiter and { as prefix, and } as suffix (as fallback if nothing was provided). Adapt the code of CitationKeyGenerationPreferences accordingly. If this is successfully done, The command "Push entries to external application" adds a "null" prefix and suffix to an existing Citationkey in LaTeX. #10568 is fixed.
  4. Adapt getCitePrefix(), getDelimiter(), and getCiteSuffix() to use the introduced CitationStringPreferences.
  5. Remove org.jabref.gui.push.AbstractPushToApplication#dissectCiteCommand.
  6. Remove org.jabref.gui.push.PushToApplication#getDelimiter
    • Remove method
    • Change parameter at org.jabref.gui.push.PushToApplicationCommand#pushEntries to use CitationCommandStringPreferencess delimiter This is wrong; since application's delimiter has higher precedence than the one of the citaton command.
  7. Adapt org.jabref.gui.edit.CopyMoreAction#copyCiteKey to use the new preference
@koppor koppor added the good first issue An issue intended for project-newcomers. Varies in difficulty. label Nov 2, 2023
@github-project-automation github-project-automation bot moved this to Free to take in Good First Issues Nov 2, 2023
@ExpPhysU
Copy link

ExpPhysU commented Nov 4, 2023

I confirm this bug, making version 5.1.1 not usable. Revert to 5.10 and hoping, that it is corrected soon

@koppor koppor added the bug Confirmed bugs or reports that are very likely to be bugs label Nov 5, 2023
@OvercloudX
Copy link

Hello koppor, hope everything's well. This is my first time taking a issue and I wonder if I can take this one? Thanks

@keshav2025
Copy link

Hello koppor, I hope you're doing well. This is my first time addressing an issue, and I'm wondering if I can handle this one. Thanks!

@ThiloteE
Copy link
Member

Sorry for the late reply. I will assign @OvercloudX, because of first come first serve.

@keshav2025 Please choose another issue. We usually only assign one issue per student to avoid trouble further down the road. I am sorry.

@ThiloteE ThiloteE moved this from Free to take to Reserved in Candidates for University Projects Nov 11, 2023
@ThiloteE ThiloteE moved this from Free to take to Reserved in Good First Issues Nov 11, 2023
@OvercloudX
Copy link

Sorry for the late reply. I will assign @OvercloudX, because of first come first serve.

@keshav2025 Please choose another issue. We usually only assign one issue per student to avoid trouble further down the road. I am sorry.

Thanks u so much, im gonna start up in it

@OvercloudX
Copy link

@ExpPhysU @koppor @ThiloteE
Hello everyone, I hope you are doing well.
Could you please tell me how to reproduce the problem? It seems there was a missing image before the implementation part.
屏幕截图 2023-11-22 101153

@ThiloteE
Copy link
Member

ThiloteE commented Nov 22, 2023

Indeed. Maybe your browser has issues? Could you try with another one? I use Mozilla Firefox and I can see the image. What happens, if you click on that image link?

@OvercloudX
Copy link

Indeed. Maybe your browser has issues? Could you try with another one? I use Mozilla Firefox and I can see the image. What happens, if you click on that image link?

I use edge, and I tried Firefox but it had the same problem. When I click the image link it shows 404 not found.

@ThiloteE
Copy link
Member

ThiloteE commented Nov 22, 2023

Ok, I think I fixed it. Apparently the links to those images were copied from a private protected GitHub repository. I uploaded new images, which you now should be able to see. Could you please confirm, if you can see them now?

@OvercloudX
Copy link

Ok, I think I fixed it. Apparently the links to those images were copied from a private protected GitHub repository. I uploaded new images, which you now should be able to see. Could you please confirm, if you can see them now?

yes, it works, thank you for your help

@koppor
Copy link
Member Author

koppor commented Nov 23, 2023

Could you please tell me how to reproduce the problem?

Can you now reproduce the issue?

You can open the example BibTeX file https://github.com/JabRef/jabref/blob/main/src/test/resources/testbib/jabref-authors.bib.

And then you can go to an entry and try to do "Copy \cite{citationkey}". I think, even this context menu item text needs to be adapted based on the preferences.

@OvercloudX
Copy link

Could you please tell me how to reproduce the problem?

Can you now reproduce the issue?

You can open the example BibTeX file https://github.com/JabRef/jabref/blob/main/src/test/resources/testbib/jabref-authors.bib.

And then you can go to an entry and try to do "Copy \cite{citationkey}". I think, even this context menu item text needs to be adapted based on the preferences.

Yes, I've succeeded reproduce the issue I think. I saw that it was different when I imported to TEX Studio using two methods.

Thank you for the help.

@OvercloudX
Copy link

@koppor @ThiloteE Hello I have a question about the step3. It is said that the default prefix should be "{" but when I check the defaultCiteCommand in the java/org/jabref/preferences/ExternalApplicationsPreferences.java, I found it was "\cite{key1,key2}" so I wonder if the default prefix is "\cite{" or "{" ?

@OvercloudX
Copy link

@koppor @ThiloteE Hello I have a question about the step3. It is said that the default prefix should be "{" but when I check the defaultCiteCommand in the java/org/jabref/preferences/ExternalApplicationsPreferences.java, I found it was "\cite{key1,key2}" so I wonder if the default prefix is "\cite{" or "{" ?

Please forgive me if I made any silly mistake because I'm new in code.

@koppor
Copy link
Member Author

koppor commented Dec 4, 2023

@koppor @ThiloteE Hello I have a question about the step3. It is said that the default prefix should be "{" but when I check the defaultCiteCommand in the java/org/jabref/preferences/ExternalApplicationsPreferences.java, I found it was "\cite{key1,key2}" so I wonder if the default prefix is "\cite{" or "{" ?

You can reproduce by

  1. Unisntalling JabRef
  2. Isntalling JabRef 5.10
  3. Reset preferences in JabRef 5.10
  4. Close JabRef
  5. Start JabRef from maikn
  6. Check the preference

Now, you should see \cite instead of \cite{key1,key2}.

See https://github.com/JabRef/jabref/pull/10303/files#diff-42a07fbc47ff359873a26e31184338ddf82ddcecf188dac16da1ef103eae3833R703 for the change introduced in the PR

Please note that I linked the PR in the first words of the issue description

You need to click on "Files changed"

grafik

Then scroll down to src/main/java/org/jabref/preferences/JabRefPreferences.java. You can also click a the "file explorer"

grafik

Is it now more clear?

@koppor
Copy link
Member Author

koppor commented Dec 4, 2023

@OvercloudX Did you do step 2 - and modify org.jabref.preferences.ExternalApplicationsPreferences#getCiteCommand?

Can we see it a draft pull request?

@OvercloudX
Copy link

@OvercloudX Did you do step 2 - and modify org.jabref.preferences.ExternalApplicationsPreferences#getCiteCommand?

Can we see it a draft pull request?

Yes, I think I may successfully finished the part 2 and I'd like to make a draft PR to you. I changed the method and created a record(CitationCommandStringPreferences).
Now I'm stuck in testing, I used JUnit for the test.

@Siedlerchr
Copy link
Member

@OvercloudX You probably have to mock the preferences with prefs = mock(ExternalApplicationsPreferences.class) and then using when(prefs.getCiteCommand).thenReturn...

@OvercloudX
Copy link

@OvercloudX You probably have to mock the preferences with prefs = mock(ExternalApplicationsPreferences.class) and then using when(prefs.getCiteCommand).thenReturn...

Thank you for the help, gonna that right now

@OvercloudX
Copy link

@koppor @ThiloteE Hello I have a question about the step3. It is said that the default prefix should be "{" but when I check the defaultCiteCommand in the java/org/jabref/preferences/ExternalApplicationsPreferences.java, I found it was "\cite{key1,key2}" so I wonder if the default prefix is "\cite{" or "{" ?

You can reproduce by

  1. Unisntalling JabRef
  2. Isntalling JabRef 5.10
  3. Reset preferences in JabRef 5.10
  4. Close JabRef
  5. Start JabRef from maikn
  6. Check the preference

Now, you should see \cite instead of \cite{key1,key2}.

See https://github.com/JabRef/jabref/pull/10303/files#diff-42a07fbc47ff359873a26e31184338ddf82ddcecf188dac16da1ef103eae3833R703 for the change introduced in the PR

Please note that I linked the PR in the first words of the issue description

You need to click on "Files changed"

grafik

Then scroll down to src/main/java/org/jabref/preferences/JabRefPreferences.java. You can also click a the "file explorer"

grafik

Is it now more clear?

Thanks, that's more clear, but I still confused with these things:

  1. These procedures are for me to have defaultCiteCommand as "\cite"
  2. So it was \cite before the issue Push to external application config #10303, and after that merged PR it becomes \cite{key1,key2}

Thank you again for taking the time and effort to help me

@koppor
Copy link
Member Author

koppor commented Dec 5, 2023

Thanks, that's more clear, but I still confused with these things:

Software Engineering is also about clear requirements. It is good that you try to understand them. I would ask you to read and try to understand existing material. Please take the time to read. No rush. I assume you worked with Microsoft Word and not with LaTeX and JabRef. Then, it takes even more time to understand the whole setting.

In the statements you gave me, I can just write "yes" as answer. Do you ask about the why? From #10303, there is #10133 linked. I try to copy and paste from that.

1. These procedures are for me to have defaultCiteCommand as "\cite"

Yes. JabRef 5.10.

2. So it was \cite before the issue [Push to external application config #10303](https://github.com/JabRef/jabref/pull/10303), and after that merged PR it becomes \cite{key1,key2}

Yes. JabRef 5.10 to 5.11.

Quoting: #10133

I use Pandoc for processing Markdown manuscripts to ODT format. Markdown cite command is [@key] instead of LaTeX \cite{key}. In case of multiple citations this is [@key1; @key2]. Currently the preference for cite command does not include the { and } characters, making it impossible to change it to another cite command that does not use curly brackets. Also, the separating character (,) cannot be modified.

Quoting #10303 with a highlight.

grafik

Screenshot of today's JabRef:

grafik

Did you click on the question mark?

It leads to https://docs.jabref.org/cite/pushtoapplications

I highlight the important part. It is still worth to read the complete page.

grafik


Now click on "Copy \cite{citaton key}"

grafik

Result is:

\cite{key1,key2}{lala}

Which is wrong. The issue is about to fix THAT behavior.

@OvercloudX
Copy link

@koppor Thanks for your advice, I'll read them again.

@koppor
Copy link
Member Author

koppor commented Dec 18, 2023

@OvercloudX We did not see any activity on your side. Do you intend to continue?

@OvercloudX
Copy link

@OvercloudX We did not see any activity on your side. Do you intend to continue?

Hi there, recently I don't have much time to do it. But I still have strong willing to continue after several days.

But if there's anyone who want to take this issue now, I don't mind if you give it to someone else, because I don't want to take up public resources either.

@ThiloteE
Copy link
Member

Take your time, we will be happy, if you come back to this :-)

github-merge-queue bot pushed a commit that referenced this issue Dec 22, 2023
* Fix Copy cite command should respect preferences

Fixes #10615

* add changelog
fix checsktyle

* Fix CHANGELOG.md

* Add missing "citation" to "key"

* Add debug message at error in logic in CopyMoreAction

* Add fallback if preference could not be parsed

* Fix key binding setting

* More relaxed parsing

* Streamline code in AbstractPushToApplication

* Add more test cases for configured citation command

* Streamline code for copy action

* Update src/main/java/org/jabref/gui/preferences/external/ExternalTabViewModel.java

* Fix test

---------

Co-authored-by: Oliver Kopp <[email protected]>
@github-project-automation github-project-automation bot moved this from Reserved to Done in Good First Issues Dec 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bugs or reports that are very likely to be bugs good first issue An issue intended for project-newcomers. Varies in difficulty. preferences
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants