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

Refactor EntryEvents - removal part #5410

Merged
merged 36 commits into from
Nov 11, 2019
Merged
Show file tree
Hide file tree
Changes from 31 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
80e8377
Initial changes to remove entry functionality
abepolk Oct 8, 2019
48cb7fd
Fix logic error in KeyChangeListener
abepolk Oct 8, 2019
923b40e
Fix logic error in BasePanel
abepolk Oct 8, 2019
a0ffcb5
Changes to tests
abepolk Oct 9, 2019
b08ae88
Fix typo
abepolk Oct 9, 2019
3a830a8
Move remove entry calls to batch version
abepolk Oct 11, 2019
4e57a46
Un-propagate for loop in KeyChangeListener
abepolk Oct 15, 2019
b990e0d
Finalize changes to SharedEntriesNotPresentEvent
abepolk Oct 15, 2019
3122e67
Fix bug
abepolk Oct 15, 2019
aac840c
Fix other compile errors
abepolk Oct 15, 2019
90c27f1
Fix bug and update tests
abepolk Oct 20, 2019
a0cafc4
Fix tests
abepolk Oct 20, 2019
868deed
Merge master
abepolk Oct 20, 2019
2afb868
Fix test omission
abepolk Oct 20, 2019
460f44a
Update l10n
abepolk Oct 20, 2019
f11389e
For loop to citationStyle
abepolk Oct 20, 2019
f535a0b
Add comment for method not working
abepolk Oct 23, 2019
77ff450
Clarify var name
abepolk Oct 23, 2019
b0805cc
Allow single entry for undo
abepolk Oct 23, 2019
7ac7a1c
Replace compound edit undo with normal undo in BasePanel
abepolk Oct 24, 2019
8255ad2
Typo
abepolk Oct 25, 2019
3b8dfea
Simplify loop in DBMSSynchronizer
abepolk Oct 27, 2019
5f1b811
Use if instead of stream
abepolk Oct 28, 2019
c41dc17
Pluralize Javadoc
abepolk Oct 30, 2019
bb88484
EntryEvent -> EntriesEvent in Javadoc, comments, and var names
abepolk Oct 30, 2019
4c97c64
Make imports explicit in BasePanel
abepolk Oct 30, 2019
30d8d51
Merge master
abepolk Oct 30, 2019
5cf5965
Final EntriesRemovedEvent fixes
abepolk Nov 1, 2019
1df2869
Fix checkStyleMain
abepolk Nov 3, 2019
0f5d76f
Merge branch 'master' into refactor_group_entryevents
abepolk Nov 3, 2019
13abf0c
More checkStyle fixes
abepolk Nov 3, 2019
17f04b0
More fixes
abepolk Nov 10, 2019
214e958
Move List coercion into DuplicateSearchResult method
abepolk Nov 10, 2019
a90c44e
Fix typo
abepolk Nov 11, 2019
648c5da
Fix pesky BibDatabaseTest error with setStrings
abepolk Nov 11, 2019
b0f585a
Fixed BibDatabase Javadoc
abepolk Nov 11, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 16 additions & 23 deletions src/main/java/org/jabref/gui/BasePanel.java
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@
import org.jabref.gui.undo.NamedCompound;
import org.jabref.gui.undo.UndoableFieldChange;
import org.jabref.gui.undo.UndoableInsertEntry;
import org.jabref.gui.undo.UndoableRemoveEntry;
import org.jabref.gui.undo.UndoableRemoveEntries;
import org.jabref.gui.util.DefaultTaskExecutor;
import org.jabref.gui.worker.SendAsEMailAction;
import org.jabref.logic.citationstyle.CitationStyleCache;
Expand All @@ -80,15 +80,15 @@
import org.jabref.model.database.KeyCollisionException;
import org.jabref.model.database.event.BibDatabaseContextChangedEvent;
import org.jabref.model.database.event.CoarseChangeFilter;
import org.jabref.model.database.event.EntriesRemovedEvent;
import org.jabref.model.database.event.EntryAddedEvent;
import org.jabref.model.database.event.EntryRemovedEvent;
import org.jabref.model.database.shared.DatabaseLocation;
import org.jabref.model.database.shared.DatabaseSynchronizer;
import org.jabref.model.entry.BibEntry;
import org.jabref.model.entry.FileFieldParser;
import org.jabref.model.entry.LinkedFile;
import org.jabref.model.entry.event.EntriesEventSource;
import org.jabref.model.entry.event.EntryChangedEvent;
import org.jabref.model.entry.event.EntryEventSource;
import org.jabref.model.entry.field.Field;
import org.jabref.model.entry.field.FieldFactory;
import org.jabref.model.entry.field.SpecialField;
Expand Down Expand Up @@ -408,19 +408,9 @@ private void delete(boolean cut, List<BibEntry> entries) {
return;
}

NamedCompound compound;
if (cut) {
compound = new NamedCompound((entries.size() > 1 ? Localization.lang("cut entries") : Localization.lang("cut entry")));
} else {
compound = new NamedCompound((entries.size() > 1 ? Localization.lang("delete entries") : Localization.lang("delete entry")));
}
for (BibEntry entry : entries) {
compound.addEdit(new UndoableRemoveEntry(bibDatabaseContext.getDatabase(), entry));
bibDatabaseContext.getDatabase().removeEntry(entry);
ensureNotShowingBottomPanel(entry);
}
compound.end();
getUndoManager().addEdit(compound);
getUndoManager().addEdit(new UndoableRemoveEntries(bibDatabaseContext.getDatabase(), entries, cut));
bibDatabaseContext.getDatabase().removeEntries(entries);
ensureNotShowingBottomPanel(entries);

markBaseChanged();
this.output(formatOutputMessage(cut ? Localization.lang("Cut") : Localization.lang("Deleted"), entries.size()));
Expand Down Expand Up @@ -893,10 +883,13 @@ public void entryEditorClosing() {
}

/**
* Closes the entry editor if it is showing the given entry.
* Closes the entry editor if it is showing any of the given entries.
*/
private void ensureNotShowingBottomPanel(BibEntry entry) {
if (((mode == BasePanelMode.SHOWING_EDITOR) && (entryEditor.getEntry() == entry))) {
private void ensureNotShowingBottomPanel(List<BibEntry> entriesToCheck) {

// This method is not able to close the bottom pane currently

if ((mode == BasePanelMode.SHOWING_EDITOR) && (entriesToCheck.contains(entryEditor.getEntry()))) {
closeBottomPane();
}
}
Expand Down Expand Up @@ -1129,7 +1122,7 @@ private class GroupTreeListener {
@Subscribe
public void listen(EntryAddedEvent addedEntryEvent) {
// if the added entry is an undo don't add it to the current group
if (addedEntryEvent.getEntryEventSource() == EntryEventSource.UNDO) {
if (addedEntryEvent.getEntriesEventSource() == EntriesEventSource.UNDO) {
return;
}

Expand All @@ -1145,8 +1138,8 @@ public void listen(EntryAddedEvent addedEntryEvent) {
private class EntryRemovedListener {

@Subscribe
public void listen(EntryRemovedEvent entryRemovedEvent) {
ensureNotShowingBottomPanel(entryRemovedEvent.getBibEntry());
public void listen(EntriesRemovedEvent entriesRemovedEvent) {
ensureNotShowingBottomPanel(entriesRemovedEvent.getBibEntries());
}
}

Expand Down Expand Up @@ -1184,7 +1177,7 @@ public void listen(EntryChangedEvent entryChangedEvent) {
}

@Subscribe
public void listen(EntryRemovedEvent removedEntryEvent) {
public void listen(EntriesRemovedEvent removedEntriesEvent) {
// IMO only used to update the status (found X entries)
DefaultTaskExecutor.runInJavaFXThread(() -> frame.getGlobalSearchBar().performSearch());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import org.jabref.JabRefGUI;
import org.jabref.gui.preview.PreviewViewer;
import org.jabref.gui.undo.NamedCompound;
import org.jabref.gui.undo.UndoableRemoveEntry;
import org.jabref.gui.undo.UndoableRemoveEntries;
import org.jabref.logic.bibtex.DuplicateCheck;
import org.jabref.logic.l10n.Localization;
import org.jabref.model.database.BibDatabaseContext;
Expand Down Expand Up @@ -40,7 +40,7 @@ public EntryDeleteChangeViewModel(BibEntry memEntry, BibEntry tmpEntry) {
@Override
public void makeChange(BibDatabaseContext database, NamedCompound undoEdit) {
database.getDatabase().removeEntry(memEntry);
undoEdit.addEdit(new UndoableRemoveEntry(database.getDatabase(), memEntry));
undoEdit.addEdit(new UndoableRemoveEntries(database.getDatabase(), memEntry));
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
import org.jabref.gui.duplicationFinder.DuplicateResolverDialog.DuplicateResolverType;
import org.jabref.gui.undo.NamedCompound;
import org.jabref.gui.undo.UndoableInsertEntry;
import org.jabref.gui.undo.UndoableRemoveEntry;
import org.jabref.gui.undo.UndoableRemoveEntries;
import org.jabref.gui.util.BackgroundTask;
import org.jabref.gui.util.DefaultTaskExecutor;
import org.jabref.logic.bibtex.DuplicateCheck;
Expand Down Expand Up @@ -162,10 +162,8 @@ private void handleDuplicates(DuplicateSearchResult result) {
final NamedCompound compoundEdit = new NamedCompound(Localization.lang("duplicate removal"));
// Now, do the actual removal:
if (!result.getToRemove().isEmpty()) {
for (BibEntry entry : result.getToRemove()) {
panel.getDatabase().removeEntry(entry);
compoundEdit.addEdit(new UndoableRemoveEntry(panel.getDatabase(), entry));
}
compoundEdit.addEdit(new UndoableRemoveEntries(panel.getDatabase(), new ArrayList<>(result.getToRemove())));
Copy link
Member

Choose a reason for hiding this comment

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

I use the constructor of UndoableRemoveEntries accepting a single BibEntry, and similarly the method BibDatabase#removeEntry.
(Btw, there is Colections.singletonList(element) which is preferred over new ArrayList<>(element)).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that DuplicateSearch.getToRemove returns a Collection, not a single one. The only way I found that I could coerce a Collection to a List was via the ArrayList<> constructor. I could make a constructor for a Collection, but that feels to specific.

Copy link
Member

Choose a reason for hiding this comment

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

+1 for the second constructor. Even if very specific, the code with new ArrayList<> reads strange.

panel.getDatabase().removeEntries(new ArrayList<>(result.getToRemove()));
Copy link
Member

Choose a reason for hiding this comment

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

Collections.singletonList should work here, doesn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe DuplicateSearch.DuplicateSearchResult.getToRemove returns a Collection, and it could have multiple elements. As of now, I am simply coercing it into a List. However, it might make more sense to do the coercion in the method DuplicateSearch.DuplicateSearchResult.getToRemove so that it matches getToAdd, which returns a List. Let me know your thoughts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just made the change, as I think it's better if getToRemove returns a List.

panel.markBaseChanged();
}
// and adding merged entries:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.jabref.gui.mergeentries;

import java.util.Arrays;
import java.util.List;
import java.util.Optional;

Expand All @@ -10,7 +11,7 @@
import org.jabref.gui.actions.SimpleCommand;
import org.jabref.gui.undo.NamedCompound;
import org.jabref.gui.undo.UndoableInsertEntry;
import org.jabref.gui.undo.UndoableRemoveEntry;
import org.jabref.gui.undo.UndoableRemoveEntries;
import org.jabref.logic.l10n.Localization;
import org.jabref.model.entry.BibEntry;

Expand Down Expand Up @@ -57,10 +58,9 @@ public void execute() {
// Remove the other two entries and add them to the undo stack (which is not working...)
NamedCompound ce = new NamedCompound(Localization.lang("Merge entries"));
ce.addEdit(new UndoableInsertEntry(basePanel.getDatabase(), mergedEntry.get()));
ce.addEdit(new UndoableRemoveEntry(basePanel.getDatabase(), one));
basePanel.getDatabase().removeEntry(one);
ce.addEdit(new UndoableRemoveEntry(basePanel.getDatabase(), two));
basePanel.getDatabase().removeEntry(two);
List<BibEntry> entriesToRemove = Arrays.asList(one, two);
Copy link
Member

Choose a reason for hiding this comment

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

Lists.of(one, two) should work, too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure. I just want to create a List of the two BibEntrys.

ce.addEdit(new UndoableRemoveEntries(basePanel.getDatabase(), entriesToRemove));
basePanel.getDatabase().removeEntries(entriesToRemove);
ce.end();
basePanel.getUndoManager().addEdit(ce);

Expand Down
10 changes: 5 additions & 5 deletions src/main/java/org/jabref/gui/shared/SharedDatabaseUIManager.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,14 @@
import org.jabref.gui.entryeditor.EntryEditor;
import org.jabref.gui.exporter.SaveDatabaseAction;
import org.jabref.gui.mergeentries.MergeEntriesDialog;
import org.jabref.gui.undo.UndoableRemoveEntry;
import org.jabref.gui.undo.UndoableRemoveEntries;
import org.jabref.logic.importer.ParserResult;
import org.jabref.logic.l10n.Localization;
import org.jabref.logic.shared.DBMSConnection;
import org.jabref.logic.shared.DBMSConnectionProperties;
import org.jabref.logic.shared.DBMSSynchronizer;
import org.jabref.logic.shared.event.ConnectionLostEvent;
import org.jabref.logic.shared.event.SharedEntryNotPresentEvent;
import org.jabref.logic.shared.event.SharedEntriesNotPresentEvent;
import org.jabref.logic.shared.event.UpdateRefusedEvent;
import org.jabref.logic.shared.exception.InvalidDBMSConnectionPropertiesException;
import org.jabref.logic.shared.exception.NotASharedDatabaseException;
Expand Down Expand Up @@ -121,13 +121,13 @@ public void listen(UpdateRefusedEvent updateRefusedEvent) {
}

@Subscribe
public void listen(SharedEntryNotPresentEvent event) {
public void listen(SharedEntriesNotPresentEvent event) {
BasePanel panel = jabRefFrame.getCurrentBasePanel();
EntryEditor entryEditor = panel.getEntryEditor();

panel.getUndoManager().addEdit(new UndoableRemoveEntry(panel.getDatabase(), event.getBibEntry()));
panel.getUndoManager().addEdit(new UndoableRemoveEntries(panel.getDatabase(), event.getBibEntries()));

if (Objects.nonNull(entryEditor) && (entryEditor.getEntry() == event.getBibEntry())) {
if (Objects.nonNull(entryEditor) && (event.getBibEntries().contains(entryEditor.getEntry()))) {

dialogService.showInformationDialogAndWait(Localization.lang("Shared entry is no longer present"),
Localization.lang("The entry you currently work on has been deleted on the shared side.")
Expand Down
87 changes: 87 additions & 0 deletions src/main/java/org/jabref/gui/undo/UndoableRemoveEntries.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
package org.jabref.gui.undo;

import java.util.Collections;
import java.util.List;

import org.jabref.logic.l10n.Localization;
import org.jabref.model.database.BibDatabase;
import org.jabref.model.entry.BibEntry;
import org.jabref.model.entry.event.EntriesEventSource;
import org.jabref.model.strings.StringUtil;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

/**
* This class represents the removal of entries. The constructor needs
* references to the database, the entries, and the map of open entry editors.
* TODO is this map still being used?
Copy link
Member

Choose a reason for hiding this comment

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

Can this TODO be resoled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would appear the map is not being used, as there is no reference to entry editors in the class. Am I mistaken?

* The latter to be able to close the entry's editor if it is opened after
* an undo, and the removal is then undone.
*/
public class UndoableRemoveEntries extends AbstractUndoableJabRefEdit {

private static final Logger LOGGER = LoggerFactory.getLogger(UndoableRemoveEntries.class);
private final BibDatabase base;
private final List<BibEntry> entries;
private final boolean cut;

public UndoableRemoveEntries(BibDatabase base, BibEntry entry) {
this(base, Collections.singletonList(entry));
}

public UndoableRemoveEntries(BibDatabase base, List<BibEntry> entries) {
this(base, entries, false);
}

public UndoableRemoveEntries(BibDatabase base, List<BibEntry> entries, boolean cut) {
this.base = base;
this.entries = entries;
this.cut = cut;
}

@Override
public String getPresentationName() {
if (cut) {
if (entries.size() > 1) {
return Localization.lang("cut entries");
}
else if (entries.size() == 1) {
return Localization.lang("cut entry %0",
StringUtil.boldHTML(entries.get(0).getCiteKeyOptional().orElse(Localization.lang("undefined"))));
} else {
return null;
Copy link
Member

Choose a reason for hiding this comment

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

Should there something be logged? Called with no entries?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hadn't thought of that. What do you think?

}
} else {
if (entries.size() > 1) {
return Localization.lang("remove entries");
}
else if (entries.size() == 1) {
Copy link
Member

Choose a reason for hiding this comment

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

Please format the code using JabRef's code style --> The else should be in the same line than }.

Copy link
Contributor Author

@abepolk abepolk Nov 10, 2019

Choose a reason for hiding this comment

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

Got it. Let me know if I've fixed it right. Is there a way to get IntelliJ to do this automatically?

return Localization.lang("remove entry %0",
StringUtil.boldHTML(entries.get(0).getCiteKeyOptional().orElse(Localization.lang("undefined"))));
}
else {
return null;
Copy link
Member

Choose a reason for hiding this comment

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

Should there something be logged? Called with no entries?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hadn't thought of that. What do you think?

}
}
}

@Override
public void undo() {
super.undo();
base.insertEntries(entries, EntriesEventSource.UNDO);
}

@Override
public void redo() {
super.redo();

// Redo the change.
try {
base.removeEntries(entries);
} catch (Throwable ex) {
LOGGER.warn("Problem to redo `remove entries`", ex);
}
}

}
53 changes: 0 additions & 53 deletions src/main/java/org/jabref/gui/undo/UndoableRemoveEntry.java

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import java.util.Objects;

import org.jabref.model.database.BibDatabaseContext;
import org.jabref.model.database.event.EntryRemovedEvent;
import org.jabref.model.database.event.EntriesRemovedEvent;
import org.jabref.model.entry.BibEntry;
import org.jabref.model.entry.event.EntryChangedEvent;

Expand Down Expand Up @@ -62,11 +62,13 @@ public void listen(EntryChangedEvent entryChangedEvent) {
}

/**
* removes the citation of the removed entry as it's not needed anymore
* removes the citation of the removed entries as they are not needed anymore
*/
@Subscribe
public void listen(EntryRemovedEvent entryRemovedEvent) {
citationStyleCache.invalidate(entryRemovedEvent.getBibEntry());
public void listen(EntriesRemovedEvent entriesRemovedEvent) {
abepolk marked this conversation as resolved.
Show resolved Hide resolved
for (BibEntry entry : entriesRemovedEvent.getBibEntries()) {
citationStyleCache.invalidate(entry);
}
}
}
}
4 changes: 2 additions & 2 deletions src/main/java/org/jabref/logic/shared/DBMSProcessor.java
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
import org.jabref.model.database.shared.DatabaseConnection;
import org.jabref.model.database.shared.DatabaseConnectionProperties;
import org.jabref.model.entry.BibEntry;
import org.jabref.model.entry.event.EntryEventSource;
import org.jabref.model.entry.event.EntriesEventSource;
import org.jabref.model.entry.field.Field;
import org.jabref.model.entry.field.FieldFactory;
import org.jabref.model.entry.types.EntryTypeFactory;
Expand Down Expand Up @@ -475,7 +475,7 @@ public List<BibEntry> getSharedEntries(List<Integer> sharedIDs) {

String value = selectEntryResultSet.getString("VALUE");
if (value != null) {
bibEntry.setField(FieldFactory.parseField(selectEntryResultSet.getString("NAME")), value, EntryEventSource.SHARED);
bibEntry.setField(FieldFactory.parseField(selectEntryResultSet.getString("NAME")), value, EntriesEventSource.SHARED);
}
}
} catch (SQLException e) {
Expand Down
Loading