Add MergeLibraries functionality #9292

leonzolati · 2022-10-24T05:37:16Z

Added UI element to create user friendly way of interacting with the feature
Added a command that merges any libraries within a given directory in to the current active library according to the rules specifies in the issue so that users with many .lib files that wish to compile them into one .lib file may do so.

Fixes https://github.com/koppor/jabref/issues/295

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 developer's documentation](https://devdocs.jabref.org/): Is the information available and up to date? If not, I outlined it in this pull request.
Checked documentation](https://docs.jabref.org/): Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

Apologies for the image being in a separate post, github was not cooperating on my computer for whatever reason. Thank you.

Added test case for MergeCommand

… match in the test case

crazycatseven · 2022-10-24T05:40:05Z

calixtus

Hi, thanks for your interest in JabRef and your great contribution. After a very quick first look I noticed some issues I would like you to address. We will also take a deeper look into your code as soon we have some time in the next days.

calixtus · 2022-10-31T06:55:08Z

build.gradle

@@ -221,6 +225,8 @@ dependencies {
    // xjc needs the runtime as well for the ant task, otherwise it fails
    xjc group: 'org.glassfish.jaxb', name: 'jaxb-xjc', version: '3.0.2'
    xjc group: 'org.glassfish.jaxb', name: 'jaxb-runtime', version: '3.0.2'
+
+


Please remove the empty lines

calixtus · 2022-10-31T06:56:47Z

build.gradle

+    implementation 'com.google.jimfs:jimfs:1.2'
+    implementation 'com.google.jimfs:jimfs-parent:1.2'
+    implementation 'org.junit.jupiter:junit-jupiter:5.8.1'
+    implementation 'org.junit.jupiter:junit-jupiter:5.8.1'


Shouldn't this be testImplementation, otherwise I think blink will break?

calixtus · 2022-10-31T06:58:42Z

src/main/java/org/jabref/gui/mergeLibraries/MergeCommand.java

+import java.io.File;
+import java.io.IOException;
+import java.lang.module.Configuration;
+import java.nio.file.*;


No wildcard imports please

calixtus · 2022-10-31T06:59:32Z

src/main/java/org/jabref/gui/mergeLibraries/MergeCommand.java

+    public MergeCommand(JabRefFrame frame,PreferencesService preferences, StateManager stateManager) {
+
+        this.preferences = preferences;
+        this.frame = frame;


Try to avoid reverse dependencies

calixtus · 2022-10-31T07:00:01Z

src/main/java/org/jabref/gui/mergeLibraries/MergeCommand.java

+
+    @Override
+    public void execute() {
+        //TODO: write the execute method


Artifact comments?

calixtus · 2022-10-31T07:00:49Z

src/main/java/org/jabref/gui/mergeLibraries/MergeCommand.java

+        //construct a configuration with the current directory
+        //(this really could have just been a constructor so think of it like that)
+        DirectoryDialogConfiguration.Builder config = new DirectoryDialogConfiguration.Builder().withInitialDirectory(preferences.getFilePreferences().getWorkingDirectory());
+        //open dialog box using the dialog service which will return a path to a directory


Pretty much self explaining the line above. Really need a comment for this?

calixtus · 2022-10-31T07:02:15Z

src/main/java/org/jabref/gui/mergeLibraries/MergeCommand.java

+     *             to merge
+     * @return all the .bib files in a given directory and subdirectories
+     */
+    private List<Path> getAllFiles(Path path) throws IOException {


getAllBibFiles

calixtus · 2022-10-31T07:03:14Z

src/main/java/org/jabref/gui/mergeLibraries/MergeCommand.java

+    }
+
+    /**
+     * This method is taken from the loadDatabase method in {@link OpenDatabaseAction}.


Please avoid code duplication

HoussemNasri

The feature seems to function correctly, however, I have a few comments about the implementation.

In general, this is a great feature and I'm sure users would appreciate it, but I don't believe it's complete. I suggest that we wait until 5.8 is released before merging it.

I can think of a few things missing:

When there are duplicates, the three-way merge dialog does not appear. For instance, if you copy all of the entries from one library and paste them into another, a merge dialog will appear to resolve duplicates. So, for consistency, I believe JabRef should include that when merging libraries as well.
It's not possible to undo merging a library into the current library.

HoussemNasri · 2022-11-01T18:02:20Z

src/main/java/org/jabref/gui/mergeLibraries/MergeCommand.java

+import java.util.*;
+
+
+public class MergeCommand extends SimpleCommand {


Can you please use a more descriptive name; maybe MergeLibrariesInFolderIntoCurrent?

HoussemNasri · 2022-11-01T18:09:37Z

src/main/java/org/jabref/gui/mergeLibraries/MergeCommand.java

+    }
+
+    public BibDatabase doMerge(Path path, BibDatabase database) throws IOException {
+        //find all the .lib files by crawling in doMerge and merge them


Just like what Carl said earlier; this comment is describing HOW the code works, but comments are better used to describe the WHY. In this case, there is no 'why' to answer, so it's better to remove the comment.

HoussemNasri · 2022-11-01T18:17:06Z

src/main/java/org/jabref/gui/mergeLibraries/MergeCommand.java

+        List<Path> paths = new ArrayList<>();
+
+        try (DirectoryStream<Path> stream = Files.newDirectoryStream(path)) {
+            for (Path entry : stream) {


This code can be replaced using Files#find.

HoussemNasri · 2022-11-01T18:25:27Z

src/main/java/org/jabref/gui/mergeLibraries/MergeCommand.java

+                        break;
+                    }
+
+                    if (new DuplicateCheck(entryTypesManager).isDuplicate(entry, dbEntry, BibDatabaseMode.BIBTEX)){


You could make one instance of DuplicateCheck and reuse it. It won't make much of a difference in terms of performance, but it will result in the garbage collector doing less work and thus shorter GC pauses.

HoussemNasri · 2022-11-01T18:26:44Z

src/main/java/org/jabref/gui/mergeLibraries/MergeCommand.java

+        database.insertEntries(toAdd.stream().toList());
+//
+        //then add the database to the StateManager's activeDatabase
+        return database;


Why do you need to return the database here?

HoussemNasri · 2022-11-01T18:29:22Z

src/test/java/org/jabref/logic/mergelibraries/MergeCommandTest.java

+
+        for (BibEntry dbEntry : db.getEntries()) {
+            boolean equalFlag = false;
+            System.out.println("dbEntry: " + dbEntry);


Please remove the System.out.println calls. If you think they are necessary use a logger.

calixtus · 2022-11-19T21:03:16Z

Whats the status here? Do you want to continue this PR?

Siedlerchr · 2023-01-16T20:11:17Z

Closing this PR due to inactivity 💤
Please reopen the PR if you intend to work on this agaiin.

MauraLxy and others added 9 commits October 13, 2022 16:19

Added code stumps for MergeCommand

79d0f4f

Added test case for MergeCommand

Add GUI and part of mergeCommand

16b0ea8

Add parser and another part of Merging.

bdb6652

Solve duplication issue.

e8878e5

Bug fixed.

d6771be

Test bug fixed.

df76f2b

Fix for test case to not be OS dependant and fixed the entry types to…

35ef2c7

… match in the test case

Additional testcases added

d9bd2fe

Update CHANGELOG.md

cd60dee

ThiloteE added ui duplicateFinder labels Oct 24, 2022

Siedlerchr requested a review from HoussemNasri October 26, 2022 20:13

calixtus requested changes Oct 31, 2022

View reviewed changes

HoussemNasri requested changes Nov 1, 2022

View reviewed changes

calixtus added the status: changes required Pull requests that are not yet complete label Nov 8, 2022

koppor added the status: devcall label Jan 2, 2023

Siedlerchr closed this Jan 16, 2023

Siedlerchr removed the status: devcall label Jan 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MergeLibraries functionality #9292

Add MergeLibraries functionality #9292

leonzolati commented Oct 24, 2022 •

edited

Loading

crazycatseven commented Oct 24, 2022

calixtus left a comment

calixtus Oct 31, 2022

calixtus Oct 31, 2022 •

edited

Loading

calixtus Oct 31, 2022

calixtus Oct 31, 2022

calixtus Oct 31, 2022

calixtus Oct 31, 2022

calixtus Oct 31, 2022

calixtus Oct 31, 2022

HoussemNasri left a comment

HoussemNasri Nov 1, 2022

HoussemNasri Nov 1, 2022

HoussemNasri Nov 1, 2022

HoussemNasri Nov 1, 2022

HoussemNasri Nov 1, 2022

HoussemNasri Nov 1, 2022

calixtus commented Nov 19, 2022

Siedlerchr commented Jan 16, 2023

		import java.util.*;


		public class MergeCommand extends SimpleCommand {

Add MergeLibraries functionality #9292

Add MergeLibraries functionality #9292

Conversation

leonzolati commented Oct 24, 2022 • edited Loading

crazycatseven commented Oct 24, 2022

calixtus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calixtus Oct 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HoussemNasri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calixtus commented Nov 19, 2022

Siedlerchr commented Jan 16, 2023

leonzolati commented Oct 24, 2022 •

edited

Loading

calixtus Oct 31, 2022 •

edited

Loading