[WIP] Batch Insert entries #5691

abepolk · 2019-12-02T01:03:28Z

This is a work in progress, and I finally got it to compile. It's not tested, and I still have to turn EntryAddedEvent into EntriesAddedEvent and refactor the code to let it accept that. I also may have to change the SQL in DBMSProcessor. See #5659.

Change in CHANGELOG.md described (if applicable)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for bigger UI changes)
Checked documentation: Is the information available and up to date? If not: Issue created at /~https://github.com/JabRef/user-documentation/issues.

abepolk · 2019-12-23T23:23:08Z

It appears the failing databaseTest was simply due to a bug in my ininsertEntries method, which I solved with a one-line fix.

abepolk · 2019-12-24T00:27:31Z

Made the requested changes, and fixed the databaseTests. Is there anything else to do before I manually test it?

tobiasdiez

Thanks for your Christmas present 🎄. The code looks really good and I have only a few very minor remarks (mostly concerning naming conventions and the like).

It would be nice if you could implement these minor things, and cleanup the code a bit (removing obsolete todo comments for example). Then we can merge.

tobiasdiez · 2019-12-24T09:00:22Z

src/main/java/org/jabref/gui/EntryTypeViewModel.java

@@ -161,7 +161,7 @@ public void runFetcherWorker() {
                } else {
                    // Regenerate CiteKey of imported BibEntry
                    new BibtexKeyGenerator(basePanel.getBibDatabaseContext(), prefs.getBibtexKeyPatternPreferences()).generateAndSetKey(entry);
-                    basePanel.insertEntry(entry);
+                    basePanel.insertEntry(entry); // Should this be basePanel.getDatabase().insertEntry(entry);?


I think basePanel.insertEntry is indeed the better choice, because of the extra work it does. BibDatabase.insertEntry is just the plain insertion of the entry in the database.

tobiasdiez · 2019-12-24T09:05:37Z

src/main/java/org/jabref/gui/maintable/MainTable.java

@@ -217,17 +216,12 @@ public void paste() {
        List<BibEntry> entriesToAdd = Globals.clipboardManager.extractData();

        if (!entriesToAdd.isEmpty()) {
-            // Add new entries
-            NamedCompound ce = new NamedCompound((entriesToAdd.size() > 1 ? Localization.lang("paste entries") : Localization.lang("paste entry")));
+            database.getDatabase().insertEntries(entriesToAdd);


Please also check if the UpdateField.setAutomaticFields call is then still required or if it's part of basePanel.insertEntries.

tobiasdiez · 2019-12-24T09:07:50Z

src/main/java/org/jabref/gui/undo/UndoableInsertEntries.java

+public class UndoableInsertEntries extends AbstractUndoableJabRefEdit {
+
+    private static final Logger LOGGER = LoggerFactory.getLogger(UndoableInsertEntries.class);
+    private final BibDatabase base;


Please rename base to database.

tobiasdiez · 2019-12-24T09:16:30Z

src/main/java/org/jabref/logic/auxparser/DefaultAuxParser.java

-        BibEntry clonedEntry = (BibEntry) entry.clone();
-        result.getGeneratedBibDatabase().insertEntry(clonedEntry);
+    private void insertEntries(List<BibEntry> entries, AuxParserResult result) {
+        List<BibEntry> clonedEntries = new ArrayList<>();


I don´t think the clone is actually necessary, can you please try to remove it. Thanks

tobiasdiez · 2019-12-24T09:17:52Z

src/main/java/org/jabref/model/database/BibDatabases.java

     * @return BibDatabase that contains the entries
     */
-    public static BibDatabase createDatabase(Collection<BibEntry> bibentries) {
+    public static BibDatabase createDatabase(Collection<BibEntry> bibEntries) {


I think entries as a name is already sufficient.

I'm curious why you don't think the clone is necessary. It is my understanding that the AuxParserResult should have a separate database with separate BibEntrys, and you can't have the same entry objects in the same database, so you have to clone them.

The purpose of the clone method is too create a copy with the same data in such a way that you can modify the clone without modifying the original entry. For example for copy/pasting, you do not want to have a literal copy of the same entry as then modification to the pasted entry would transfer to the copied one.

I thought the AuxParser creates a bunch of new entries, and then the clone is not necessary. But you are right the clone might be necessary because the entries are not created but only copied. My bad! Thanks for pointing it out.

tobiasdiez · 2019-12-24T09:18:36Z

src/main/java/org/jabref/model/database/BibDatabases.java

        BibDatabase database = new BibDatabase();

-        for (BibEntry entry : bibentries) {
+        for (BibEntry entry : bibEntries) {


I might be mistaken but I think insertEntries already takes care of the id generation.

insertEntries add existing IDs to the database, but does not set them.

However, I just noticed that BibDatabases.createDatabase is only called outside of test in ParserResult. Also, all BibEntry constructors assign an ID to each BibEntry. Is it possible that the IDs are being changed for something that relates to parsing?

No, IDs should be fixed upon creation and are never changed. Moreover, they are only used in the connection with shared databases to identify remote entries with local ones. Thus, I guess this method can be removed.

It looks like this method is only used in ParserResult (and test). Perhaps we can remove the method entirely. In addition, based on what you said about IDs not changing, it might be worth it to remove the setId method from BibEntry, which is only called in BibDatabases.createDatabase and test. Maybe that could be in another PR. Thoughts?

Sounds good! Do you want to work on this, too? Then I let you decide if you want to implement it as part of this PR or a new one.

Let's do it as a new PR once I've finished this one and the DBMS synchronization.

…n BasePanel

abepolk · 2019-12-24T21:01:39Z

todo

On the TODO in UndoableInsertEntries, it seems that we don't have to keep track of the entry editors that are open because if we remove them in an undo, an entriesRemovedEvent will be fired, and EntryRemovedListener (which I will rename EntriesRemovedListener) in BasePanel will close the entry editor with the listen() method. This would have to be tested.

abepolk · 2019-12-24T21:05:59Z

Would it still be worth my manually testing everything before we merge?

tobiasdiez · 2019-12-25T11:05:53Z

On the TODO in UndoableInsertEntries, it seems that we don't have to keep track of the entry editors that are open because if we remove them in an undo, an entriesRemovedEvent will be fired, and EntryRemovedListener (which I will rename EntriesRemovedListener) in BasePanel will close the entry editor with the listen() method. This would have to be tested.

Sounds plausible!

Would it still be worth my manually testing everything before we merge?

It would be good if you could test the major changes, or where you are unsure that your changes had indeed the correct effects. Extensive testing is not necessary. To be honest: developer time is precious and we usually rely on our users to report issues that might be introduced due to recent changes. That works (surprisingly?) well for us as we have a lot of active users that try the recent development versions.

abepolk · 2019-12-30T13:07:59Z

I tested the most important changes, and a few things don't work, but those things also don't work on master. In other words, it doesn't appear that I broke anything that worked before. Should we merge?

abepolk · 2019-12-30T15:43:26Z

Also, I'm working on the DBMSProcessor (which will be in a separate PR) and am having trouble running databaseTest. For each failed test, I get the same error. See below. Any ideas how to troubleshoot it?

  org.postgresql.util.PSQLException: Connection to localhost:5432 refused. Check that the hostname and port are correct and that the postmaster is accepting TCP/IP connections.
      at org.jabref.logic.shared.DBMSProcessorTest.setup(DBMSProcessorTest.java:45)
  Caused by: java.net.ConnectException: Connection refused
      at org.jabref.logic.shared.DBMSProcessorTest.setup(DBMSProcessorTest.java:45)

Siedlerchr · 2019-12-30T16:05:52Z

@NorwayMaple The database test runs fine on github actions. If you run it locally you have to ensure at least postgres runnning at localhost.

tobiasdiez · 2019-12-30T16:40:46Z

Thanks again for your contribution!!

koppor · 2019-12-31T12:57:21Z

@NorwayMaple What do you mean with "a few things don't work"? Could you open an issue for that so that we can track it?

@NorwayMaple Do you have a local PostgreSQL instance running? The string Connection to localhost:5432 indicates that no local PostgreSQL is running on port 5432 on your local machine.

abepolk · 2019-12-31T14:26:37Z

I had trouble copying entries from one database into another database. The copy appeared to work, but the paste didn't. I can make an issue later.

I got the PostgreSQL and databaseTest to work.

* upstream/master: Add select all buttons to change dialog (#5803) Squashed 'src/main/resources/csl-locales/' changes from 9785a6e358..a3e8843f75 Squashed 'src/main/resources/csl-styles/' changes from 49a1841..b2fbe15 Fix CSL update Add new authors Remove ACM Fetcher in one more check Adapt WebFetchersTest Disable ACM Fetcher Try to fix branch name on cleanup pr action Remove obsolete class (#5801) Try to use right ref Run tests only once at pull requets Add ignored dependency Fix "&" on previews (#5786) [WIP] Batch Insert entries (#5691)

abepolk added 30 commits October 7, 2019 20:06

Initial changes to remove entry functionality

80e8377

Fix logic error in KeyChangeListener

48cb7fd

Fix logic error in BasePanel

923b40e

Changes to tests

a0ffcb5

Fix typo

b08ae88

Move remove entry calls to batch version

3a830a8

Un-propagate for loop in KeyChangeListener

4e57a46

Finalize changes to SharedEntriesNotPresentEvent

b990e0d

Fix bug

3122e67

Fix other compile errors

aac840c

Fix bug and update tests

90c27f1

Fix tests

a0cafc4

Merge master

868deed

Fix test omission

2afb868

Update l10n

460f44a

For loop to citationStyle

f11389e

Add comment for method not working

f535a0b

Clarify var name

77ff450

Allow single entry for undo

b0805cc

Replace compound edit undo with normal undo in BasePanel

7ac7a1c

Typo

8255ad2

Simplify loop in DBMSSynchronizer

3b8dfea

Use if instead of stream

5f1b811

Pluralize Javadoc

c41dc17

EntryEvent -> EntriesEvent in Javadoc, comments, and var names

bb88484

Make imports explicit in BasePanel

4c97c64

Merge master

30d8d51

Batch delete to SQL

125d33a

Final EntriesRemovedEvent fixes

5cf5965

Fix checkStyleMain

1df2869

koppor removed their assignment Dec 20, 2019

abepolk added 5 commits December 23, 2019 18:23

Fix database tests by adding entries after firing EntryAddedEvent

772f8f6

Remove redundant DBMS connection closure from DBMSSychronizerTest

454ccd0

Merge test resources from master

2fb860a

Merge master

039e223

Remove firstEntry hack

790e7f2

tobiasdiez requested changes Dec 24, 2019

View reviewed changes

abepolk added 5 commits December 24, 2019 14:46

Remove duplicate code from paste() by and pluralize insertEntries() i…

76845c2

…n BasePanel

Comment fix

30fc8e5

Clarify BasePanel insertEntries JavaDoc

a192c2f

Rename vars

a74381f

Rename vars

f2a517f

Remove unnecessary TODO and rename EntriesRemovedListener

3f78aa9

abepolk added 4 commits December 29, 2019 20:37

Merge master

d42890e

Merge fixes

85ff37b

Remove old comment

659db97

checkStyle fixes

a2a619c

tobiasdiez merged commit 371a863 into JabRef:master Dec 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Batch Insert entries #5691

[WIP] Batch Insert entries #5691

abepolk commented Dec 2, 2019

abepolk commented Dec 23, 2019

abepolk commented Dec 24, 2019

tobiasdiez left a comment

tobiasdiez Dec 24, 2019

tobiasdiez Dec 24, 2019

tobiasdiez Dec 24, 2019

tobiasdiez Dec 24, 2019

tobiasdiez Dec 24, 2019

abepolk Dec 24, 2019

tobiasdiez Dec 25, 2019

tobiasdiez Dec 24, 2019

abepolk Dec 24, 2019

abepolk Dec 24, 2019

tobiasdiez Dec 25, 2019

abepolk Dec 30, 2019

tobiasdiez Dec 30, 2019

abepolk Dec 30, 2019

abepolk commented Dec 24, 2019

abepolk commented Dec 24, 2019

tobiasdiez commented Dec 25, 2019

abepolk commented Dec 30, 2019

abepolk commented Dec 30, 2019

Siedlerchr commented Dec 30, 2019

tobiasdiez commented Dec 30, 2019

koppor commented Dec 31, 2019

abepolk commented Dec 31, 2019

[WIP] Batch Insert entries #5691

[WIP] Batch Insert entries #5691

Conversation

abepolk commented Dec 2, 2019

abepolk commented Dec 23, 2019

abepolk commented Dec 24, 2019

tobiasdiez 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

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

Choose a reason for hiding this comment

abepolk commented Dec 24, 2019

abepolk commented Dec 24, 2019

tobiasdiez commented Dec 25, 2019

abepolk commented Dec 30, 2019

abepolk commented Dec 30, 2019

Siedlerchr commented Dec 30, 2019

tobiasdiez commented Dec 30, 2019

koppor commented Dec 31, 2019

abepolk commented Dec 31, 2019