-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Add feature to merge .bib files into current bib #13320
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
base: main
Are you sure you want to change the base?
Conversation
this.shouldMergeDuplicateEntries.set(shouldMergeDuplicateEntries); | ||
} | ||
|
||
public boolean getShouldMergeSameKeyEntries() { |
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.
Method name violates Java bean naming conventions. Boolean getter methods should start with 'is' rather than 'get' for better readability and convention compliance.
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.
in this case i would opt to leave the whole get or is away. we do that in other preferences too.
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.
Hi! Thanks for the feedback, we fixed that
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.
Isn't this a bit of overkill, for two checkboxes a new preferences tab and preferences object?
I think this can be merged into another tab and another prefs object.
Think of the user. Where would the user look for these preferences options?
If there are similar preferences options in other tabs and preferences objects, your welcome to propose a new plan and a new prefs tab to collect all these similar option.
And the name is just horrible.
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.
Hi! Thank you for the feedback!
We'll have another look at the Preferences menu to see where they fit better.
Regarding the name, do you have any suggestions?
This feature allows for users to merge .bib files in a chosen directory into their current bib. If an imported entry is equal to an existent one, it is silently ignored. If it is a duplicate or has the same citation key, it can either be silently ignored or the entries are merged (users can configure their preference in the Preferences menu in the "Merge other bib files into current bib" tab). Users can also undo/redo this command. Fixes JabRef#12290 Co-authored-by: Guilherme Ribeiro Pereira <guilherme.ribeiro.pereira@tecnico.ulisboa.pt>
- Replace 'bib files' to standardized 'BibTeX' in StandardActions - Remove redundant Javadoc in MergeBibFilesIntoCurrentBibAction - Rename boolean getters - Delete obsolete test comments - Correct Changelog issue link
43bcbb0
to
e415652
Compare
- Replace 'bib' to standardized 'BibTeX' in StandardActions - Delete obsolete test comments - Corrected MergeBibFilesIntoCurrentBib string in JabRef_en.properties
- Replace 'bib' to standardized 'BibTeX' in all strings related to the merge libraries action - Corrected jabRef_en.properties according to the changes above
} catch (IOException e) { | ||
LOGGER.error("Error finding .bib files in '{}'", directory.getFileName(), e); | ||
} |
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.
I think the exception is thrown if the Files.find method cannot open or read the path, so I think there should be check for if the path is valid or not to have correct and helpful feedback to the user (including a message).
BibEntryTypesManager entryTypesManager = new BibEntryTypesManager(); | ||
DuplicateCheck dupCheck = new DuplicateCheck(entryTypesManager); | ||
|
||
for (Path p : getAllBibFiles(directory, databasePath.orElseGet(() -> Path.of("")))) { |
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.
Please avoid abbreviations
|
||
for (BibEntry toMergeEntry : result.getDatabase().getEntries()) { | ||
boolean validNewEntry = true; | ||
for (BibEntry e : database.getEntries()) { |
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.
Please avoid abbreviations
} | ||
} | ||
} | ||
NamedCompound ce = new NamedCompound(Localization.lang("Merge BibTeX files into current library")); |
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.
Please avoid abbreviations
BibDatabase database = context.getDatabase(); | ||
Optional<Path> databasePath = context.getDatabasePath(); | ||
|
||
BibEntryTypesManager entryTypesManager = new BibEntryTypesManager(); |
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.
Inject
return List.of(); | ||
} | ||
|
||
public ParserResult loadDatabase(Path file) { |
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.
Why don't you reuse existing methods? Looks like code duplication to me.
If not, put some comment on it.
void setUp() throws IOException { | ||
MockitoAnnotations.openMocks(this); | ||
|
||
FileSystem fs = Jimfs.newFileSystem(Configuration.unix()); |
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.
Why aren't you using TempDir? Are there features neccessary provided by JimFS?
https://junit.org/junit5/docs/current/api/org.junit.jupiter.api/org/junit/jupiter/api/io/TempDir.html
We already use it for testing in other classes.
Please avoid introducing new dependencies if not really needed.
selectedEntries = new ArrayList<>(); | ||
selectedEntries.add(toMergeEntry); | ||
selectedEntries.add(e); | ||
stateManager.setSelectedEntries(selectedEntries); |
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.
changes to the ui should only happen at the very end of the action.
.../main/java/org/jabref/gui/mergebibfilesintocurrentbib/MergeBibFilesIntoCurrentBibAction.java
Show resolved
Hide resolved
Thank you for the feedback! We will fix the problems mentioned |
- Tests now use JUnit TempDir
…references tab - Removed abbreviations in MergeBibFilesIntoCurrentBibAction - Added a check for the chosen directory's path validity - Merges are performed at the end of the action - Refactored MergeBibFilesIntoCurrentBib method - Preferences related to the action moved to the General tab
- Applied OpenRewrite automated code improvements - Removed unnecessary test field in duplicate tests
@@ -250,14 +250,12 @@ public void sameCitationKeyNoMergePreferenceTest() { | |||
@Test | |||
public void duplicateMergeTest() { | |||
BibEntry currentEntry = new BibEntry(StandardEntryType.Article) | |||
.withCitationKey("DIFFERENTCITATIONKEY") | |||
.withCitationKey("DIFFERENT CITATION KEY") |
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.
I don't think bibtex or biblatex allow multiple words as citation keys. The documentation does not say anything about this, but there are hints. I was looking into the source of biber but was not able to quickly locate the parser for citation keys (https://github.com/plk/biber/blob/dev/lib/Biber.pm).
But anyways, please remove whitespaces from the citation key, just to make sure.
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.
Its possible with BibLaTeX: https://tex.stackexchange.com/a/37646/9075
However, it is very seldomly used and we have no proper UI support in JabRef for it.
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.
Neither spaces nor commans are allowed in the key references: https://tex.stackexchange.com/a/271520/9075
- Fix error message - Remove whitespaces from citation keys
Hi @calixtus, when you have some time, could you check out our changes? |
return List.of(); | ||
} | ||
|
||
private boolean checkPathValidity(Path directory) { |
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.
Naming nitpick:
"checkPathValidity
" sounds like a function that doesn't return anything.
A better name would be isValidPath
so that it can be used in conditionals as if(isValidPath(...))
rather than if(checkPathValidity(...))
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.
Already changed it! Thanks!
EasyBind.listen(mergeBibFilesIntoCurrentBibPreferences.shouldMergeSameKeyEntriesProperty(), (obs, oldValue, newValue) -> putBoolean(MERGE_SAME_KEY_ENTRIES, newValue)); | ||
EasyBind.listen(mergeBibFilesIntoCurrentBibPreferences.shouldMergeDuplicateEntriesProperty(), (obs, oldValue, newValue) -> putBoolean(MERGE_DUPLICATE_ENTRIES, newValue)); |
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.
EasyBind.listen(mergeBibFilesIntoCurrentBibPreferences.shouldMergeSameKeyEntriesProperty(), (obs, oldValue, newValue) -> putBoolean(MERGE_SAME_KEY_ENTRIES, newValue)); | |
EasyBind.listen(mergeBibFilesIntoCurrentBibPreferences.shouldMergeDuplicateEntriesProperty(), (obs, oldValue, newValue) -> putBoolean(MERGE_DUPLICATE_ENTRIES, newValue)); | |
EasyBind.listen(mergeBibFilesIntoCurrentBibPreferences.shouldMergeSameKeyEntriesProperty(), (_, _, newValue) -> putBoolean(MERGE_SAME_KEY_ENTRIES, newValue)); | |
EasyBind.listen(mergeBibFilesIntoCurrentBibPreferences.shouldMergeDuplicateEntriesProperty(), (_, _, newValue) -> putBoolean(MERGE_DUPLICATE_ENTRIES, newValue)); |
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.
Done!
@trag-bot didn't find any issues in the code! ✅✨ |
Closes #12290
This PR introduces a feature that allows for users to merge .bib files in a chosen directory into their current bib.
If an imported entry is equal to an existent one, it is silently ignored. If it is a duplicate (according to JabRef's duplication algorithm) or has the same citation key, it can either be silently ignored or the entries are merged, using JabRef's merge dialog (users can configure their preference in the Preferences menu in the "Merge other bib files into current bib" tab).
Users can undo/redo this command.
Co-authored-by: Guilherme Ribeiro Pereira guilherme.ribeiro.pereira@tecnico.ulisboa.pt
Steps to test
Feature_Demonstration.webm
Automatic tests were also created for this PR.
Mandatory checks
CHANGELOG.md
described in a way that is understandable for the average user (if change is visible to the user)