-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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 to option #12374
Copy to option #12374
Conversation
…lusion and save user preference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your code currently does not meet JabRef's code guidelines.
We use Checkstyle to identify issues.
Please carefully follow the setup guide for the codestyle.
Afterwards, please run checkstyle locally and fix the issues.
In case of issues with the import order, double check that you activated Auto Import.
You can trigger fixing imports by pressing Ctrl+Alt+O to trigger Optimize Imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your code currently does not meet JabRef's code guidelines.
We use Checkstyle to identify issues.
Please carefully follow the setup guide for the codestyle.
Afterwards, please run checkstyle locally and fix the issues.
In case of issues with the import order, double check that you activated Auto Import.
You can trigger fixing imports by pressing Ctrl+Alt+O to trigger Optimize Imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your code currently does not meet JabRef's code guidelines.
We use Checkstyle to identify issues.
Please carefully follow the setup guide for the codestyle.
Afterwards, please run checkstyle locally and fix the issues.
In case of issues with the import order, double check that you activated Auto Import.
You can trigger fixing imports by pressing Ctrl+Alt+O to trigger Optimize Imports.
Please review it and provide any suggestions. |
We will do, please keep in mind that all maintainers work at jabref in
their leisure time despite having a full time job. I will try to look into
it tomorrow.
Priyanshu Gupta ***@***.***> schrieb am Sa., 11. Jan. 2025,
20:38:
… Please review it and provide any suggestions.
—
Reply to this email directly, view it on GitHub
<#12374 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACOFZA3TI5CFOYLSVHGPHL2KFXKDAVCNFSM6AAAAABU5GXCISVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKOBVGM4DMMJUGQ>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
Apologies, and thanks for the update! I understand, and I really appreciate your time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments on first look.
Please also fix the Checkstyle, Openrewrite and Unit tests. The FAQ page will help you. |
I have fixed the Checkstyle, OpenRewrite, and Unit tests errors and made changes based on the reviews. |
What should I do next? Is this PR now eligible for merging? |
You can refer to Houssem's replies and ask him for further steps if any: #12374 (comment) |
@priyanshu16095 We received no response from the assignee of #12339 about progress, so it's yours, please comment there so we can assign you. About the next steps, since you have already tackled #12339 and (partially) #12341 you might as well include #12340 in this PR. It was a mistake anyway to split the tasks in the first place, that is my bad. |
@HoussemNasri I’ve already included the code for #12340 in the pull request. This PR implements the following tasks: #12339: Implemented a 'Copy to' context menu that displays the names of currently opened libraries as checkable menu items. #12340: Added a dialog that appears after selecting the libraries, allowing users to include or exclude cross-references. #12341: Implemented functionality to save user preferences for cross-reference inclusion or exclusion, ensuring a personalized experience for future use. |
Have I understood correctly? |
@priyanshu16095 why are the menu items checkable? We need to copy the bib entry to the library that the user chose in the menu. Also, I see no code that does the copying (neither normal copying nor cross-ref copying). Please re-read the issue descriptions. |
Yeah, my bad. I thought it was only for creating the UI. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your code currently does not meet JabRef's code guidelines.
We use OpenRewrite to ensure "modern" Java coding practices.
The issues found can be automatically fixed.
Please execute the gradle task rewriteRun
, check the results, commit, and push.
You can check the detailed error output by navigating to your pull request, selecting the tab "Checks", section "Tests" (on the left), subsection "OpenRewrite".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JUnit tests are failing. In the area "Some checks were not successful", locate "Tests / Unit tests (pull_request)" and click on "Details". This brings you to the test output.
You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide.
Recording.2025-01-24.mp4The feature is working fine; it copies the entry to the selected library. If an entry has a cross-referenced entry, it shows dialog, allowing the user to choose whether to include or exclude the cross-referenced entry. Please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good from my end. Just a few nitpicks. Houssem and others will take a more thorough look.
Also, @priyanshu16095, please add a changelog entry, as well as unit tests as described in the issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JUnit tests are failing. In the area "Some checks were not successful", locate "Tests / Unit tests (pull_request)" and click on "Details". This brings you to the test output.
You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide.
@subhramit @Siedlerchr I mistakenly clicked the re-request review button. Please ignore. |
I took the liberty to test and improve the code a little bit with streams. works fine so far |
Fixes #12339, #12340 and #12341
This PR introduces a 'Copy to' context menu option with features for cross-reference inclusion/exclusion, as well as the ability to save user preferences.
THE CHANGES MADE
Mandatory checks
CHANGELOG.md
described in a way that is understandable for the average user (if change is visible to the user)