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

Added option to copy the DOI to the copy sub-menu #7894

Merged
merged 6 commits into from
Jul 16, 2021
Merged

Added option to copy the DOI to the copy sub-menu #7894

merged 6 commits into from
Jul 16, 2021

Conversation

AidanM11
Copy link
Contributor

@AidanM11 AidanM11 commented Jul 10, 2021

Adds the feature requested in
Fixes #7826

  • Modelled off already existing "Copy title" and "Copy key" features
  • Adds method copyDOI to CopyMoreAction and binds it appropriately
  • Adds new item in RightClickMenu
  • Adds appropriate tests to CopyMoreActionTests
  • Adds appropriate localization entries
    Screenshot:
    CopyDOIButton

.

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

@calixtus
Copy link
Member

Thank you for your interest in this issue, especially for thinking of the tests.
Please do not forget to check the other checkboxes in the pr description too.

@AidanM11
Copy link
Contributor Author

I've completed the checklist, and everything seems right. The only problem is the checkstyle test failing, saying that there are 5 trailing spaces in CHANGELOG.md, but my change doesn't appear to add any trailing spaces. I'm not sure what to do to fix that.

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jul 12, 2021
@@ -40,6 +40,7 @@
private BibEntry entry;
private List<String> titles = new ArrayList<String>();
private List<String> keys = new ArrayList<String>();
private List<String> DOIs = new ArrayList<String>();
Copy link
Member

Choose a reason for hiding this comment

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

Variabl names should be all lowerCamelCase, e.g dois

Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

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

Overall looks good, just one minor nitpicking.

src/main/java/org/jabref/gui/edit/CopyMoreAction.java Outdated Show resolved Hide resolved
@Siedlerchr
Copy link
Member

Thanks for the quick follow up. Looks good from my side.
PS: For the next time, it's better to use a separate branch instead of main to avoid unnecessary conflicts. And use something like Fixes ...for autoclosing the issue.

Copy link
Member

@calixtus calixtus left a comment

Choose a reason for hiding this comment

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

Looks good to me too, one thing that could be improved in a future PR is making the executable binding dependant on if there is a doi present.

@calixtus calixtus merged commit c99d61c into JabRef:main Jul 16, 2021
@calixtus
Copy link
Member

Thanks for your contribution to JabRef!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add posibility to just copy the DOI
3 participants