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

Fix for issue 3143: Import entry from clipboard in different formats #3243

Merged
merged 8 commits into from
Oct 6, 2017
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `#
- We added the file description filed back to the list of files in the `General`-Tab [#2930, comment](https://github.com/JabRef/jabref/issues/2930#issuecomment-328328172)
- Added an error dialog if the file is open in another process and cannot be renamed. [#3229]
- On Windows, the `JabRef.exe` executable can now be used to start JabRef from the command line. By default, no output is shown unless the new "-console" option is specified.
- We added support for pasting entries in formats other that BibTex [#3143](https://github.com/JabRef/jabref/issues/3143)

### Fixed

Expand Down
30 changes: 20 additions & 10 deletions src/main/java/org/jabref/gui/ClipBoardManager.java
Original file line number Diff line number Diff line change
Expand Up @@ -8,27 +8,38 @@
import java.awt.datatransfer.Transferable;
import java.awt.datatransfer.UnsupportedFlavorException;
import java.io.IOException;
import java.io.StringReader;
import java.util.ArrayList;
import java.util.List;
import java.util.Optional;

import org.jabref.Globals;
import org.jabref.logic.importer.FetcherException;
import org.jabref.logic.importer.ImportException;
import org.jabref.logic.importer.ImportFormatReader;
import org.jabref.logic.importer.ImportFormatReader.UnknownFormatImport;
import org.jabref.logic.importer.fetcher.DoiFetcher;
import org.jabref.logic.importer.fileformat.BibtexParser;
import org.jabref.model.database.BibDatabase;
import org.jabref.model.entry.BibEntry;
import org.jabref.model.entry.identifier.DOI;

import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;

public class ClipBoardManager implements ClipboardOwner {

private static final Log LOGGER = LogFactory.getLog(ClipBoardManager.class);

private static final Clipboard CLIPBOARD = Toolkit.getDefaultToolkit().getSystemClipboard();

private final ImportFormatReader importFormatReader;

public ClipBoardManager() {
this(Globals.IMPORT_FORMAT_READER);
}

public ClipBoardManager(ImportFormatReader importFormatReader) {
this.importFormatReader = importFormatReader;
}

/**
* Empty implementation of the ClipboardOwner interface.
*/
Expand Down Expand Up @@ -81,7 +92,7 @@ public List<BibEntry> extractBibEntriesFromClipboard() {

if (content.isDataFlavorSupported(TransferableBibtexEntry.ENTRY_FLAVOR)) {
// We have determined that the clipboard data is a set of entries.
try {
try {
@SuppressWarnings("unchecked")
List<BibEntry> contents = (List<BibEntry>) content.getTransferData(TransferableBibtexEntry.ENTRY_FLAVOR);
result = contents;
Expand All @@ -99,12 +110,11 @@ public List<BibEntry> extractBibEntriesFromClipboard() {
Optional<BibEntry> entry = new DoiFetcher(Globals.prefs.getImportFormatPreferences()).performSearchById(new DOI(data).getDOI());
entry.ifPresent(result::add);
} else {
// parse bibtex string
BibtexParser bp = new BibtexParser(Globals.prefs.getImportFormatPreferences());
BibDatabase db = bp.parse(new StringReader(data)).getDatabase();
LOGGER.info("Parsed " + db.getEntryCount() + " entries from clipboard text");
if (db.hasEntries()) {
result = db.getEntries();
try {
UnknownFormatImport unknownFormatImport = importFormatReader.importUnknownFormatFromString(data);
result = unknownFormatImport.parserResult.getDatabase().getEntries();
} catch (ImportException e) {
// import failed and result will be empty
}
}
} catch (UnsupportedFlavorException ex) {
Expand Down
38 changes: 38 additions & 0 deletions src/main/java/org/jabref/logic/importer/ImportFormatReader.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package org.jabref.logic.importer;

import java.io.BufferedReader;
import java.io.IOException;
import java.io.StringReader;
import java.nio.file.Path;
import java.util.List;
import java.util.Objects;
Expand Down Expand Up @@ -31,6 +33,7 @@
import org.jabref.logic.xmp.XMPPreferences;
import org.jabref.model.database.BibDatabases;
import org.jabref.model.entry.BibEntry;
import org.jabref.model.entry.FieldName;
import org.jabref.model.strings.StringUtil;

public class ImportFormatReader {
Expand Down Expand Up @@ -215,4 +218,39 @@ public UnknownFormatImport importUnknownFormat(Path filePath) throws ImportExcep

throw new ImportException(Localization.lang("Could not find a suitable import format."));
}

public UnknownFormatImport importUnknownFormatFromString(String data) throws ImportException {
Copy link
Member

@tobiasdiez tobiasdiez Oct 1, 2017

Choose a reason for hiding this comment

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

As far as I can see, this is mostly a copy of importUnknownFormat(Path filePath) and I would like to have a solution without code duplication. Maybe something along the following lines works?

private UnknownFormatImport importUnknownFormat(Function<Importer, ParserResult> importDatabase, Function<Importer, Boolean> isRecognizedFormat) {
    // original method with obvious changes imFo.importDatabase -> importDatabase.apply(imFo)
}

public UnknownFormatImport importUnknownFormat(Path filePath) {
    return importUnknownFormat(importer -> importer.importDatabase(filePath, importFormatPreferences.getEncoding(), importer -> importer .isRecognizedFormat(filePath, importFormatPreferences.getEncoding());
}

public UnknownFormatImport importUnknownFormat(String data) {
    return importUnknownFormat(importer -> importer.importDatabase(data), importer -> importer .isRecognizedFormat(data));
}

It's a bit ugly. Do you have a better idea to reduce the code duplication?

// stores ref to best result, gets updated at the next loop
List<BibEntry> bestResult = null;
long bestResultCount = 0;
String bestFormatName = null;

// Cycle through all importers:
for (Importer imFo : getImportFormats()) {
try (StringReader in = new StringReader(data); BufferedReader input = new BufferedReader(in)) {
ParserResult parserResult = imFo.importDatabase(input);
Copy link
Member

Choose a reason for hiding this comment

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

Can you please extract the method Importer.importDatabase(String data). This new method can be reused at least in the MrDLibFetcher. Similarly, for the isRecognizedFormat.

List<BibEntry> entries = parserResult.getDatabase().getEntries();

BibDatabases.purgeEmptyEntries(entries);
// some parsers return non-empty entries even if the string is in a wrong format. While this prevents pasting of databases where all entries have no title or no author, it prevents false positives
long entryCount = entries.stream().filter(bibEntry -> bibEntry.getField(FieldName.TITLE).isPresent() && bibEntry.getField(FieldName.AUTHOR).isPresent()).count();

if (entryCount > bestResultCount) {
bestResult = entries;
bestResultCount = entryCount;
bestFormatName = imFo.getName();
}
} catch (IOException | UnsupportedOperationException | IllegalArgumentException ex) {
// The import did not succeed. Go on.
}
}

if (bestResult != null) {
// we found something
ParserResult parserResult = new ParserResult(bestResult);
return new UnknownFormatImport(bestFormatName, parserResult);
}

throw new ImportException(Localization.lang("Could not find a suitable import format."));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -450,6 +450,9 @@ private String getSubfield(String a, Element datafield) {
}

private Element getChild(String name, Element e) {
if (e == null) {
return null;
}
NodeList children = e.getChildNodes();

int j = children.getLength();
Expand Down
85 changes: 85 additions & 0 deletions src/test/java/org/jabref/gui/ClipBoardManagerTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
package org.jabref.gui;

import java.awt.datatransfer.Clipboard;
import java.awt.datatransfer.DataFlavor;
import java.awt.datatransfer.Transferable;
import java.lang.reflect.Field;
import java.lang.reflect.Modifier;
import java.util.Arrays;
import java.util.List;

import org.jabref.logic.importer.ImportException;
import org.jabref.logic.importer.ImportFormatReader;
import org.jabref.logic.importer.ImportFormatReader.UnknownFormatImport;
import org.jabref.logic.importer.ParserResult;
import org.jabref.model.entry.BibEntry;

import org.junit.Before;
import org.junit.Test;
import org.mockito.ArgumentMatchers;

import static org.junit.Assert.*;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

public class ClipBoardManagerTest {

private ClipBoardManager clipBoardManager;
private Transferable content;
private ImportFormatReader importFormatReader;

@Before
public void setUp() throws Exception {
importFormatReader = mock(ImportFormatReader.class);

clipBoardManager = new ClipBoardManager(importFormatReader);
Clipboard clipboard = mock(Clipboard.class);
Field field = ClipBoardManager.class.getDeclaredField("CLIPBOARD");
Copy link
Member

Choose a reason for hiding this comment

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

I'm a big fan of these tests! The only thing that I don't like is this hack to set CLIPBOARD. Instead I propose to convert

private static final Clipboard CLIPBOARD = Toolkit.getDefaultToolkit().getSystemClipboard();

to an instance variable which is initialized in the constructor. Then you can just use clipBoardManager = new ClipBoardManager(clipboard, importFormatReader).

try {
field.setAccessible(true);

Field modifiersField = Field.class.getDeclaredField("modifiers");
modifiersField.setAccessible(true);
modifiersField.setInt(field, field.getModifiers() & ~Modifier.FINAL);

field.set(ClipBoardManager.class, clipboard);
} finally {
field.setAccessible(false);
}

content = mock(Transferable.class);
when(clipboard.getContents(ArgumentMatchers.any())).thenReturn(content);
}

@Test
public void extractBibEntriesFromClipboardParsesStringFlavor() throws Exception {
BibEntry expected = new BibEntry();
expected.setType("article");
expected.setCiteKey("canh05");
expected.setField("author", "Crowston, K. and Annabi, H.");
expected.setField("title", "Title A");

when(content.isDataFlavorSupported(TransferableBibtexEntry.ENTRY_FLAVOR)).thenReturn(false);
when(content.isDataFlavorSupported(DataFlavor.stringFlavor)).thenReturn(true);
String data = "@article{canh05, author = {Crowston, K. and Annabi, H.},\n" + " title = {Title A}}\n";
when(content.getTransferData(DataFlavor.stringFlavor)).thenReturn(data);
when(importFormatReader.importUnknownFormatFromString(data)).thenReturn(new UnknownFormatImport("abc", new ParserResult(Arrays.asList(expected))));

List<BibEntry> actual = clipBoardManager.extractBibEntriesFromClipboard();

assertEquals(Arrays.asList(expected), actual);
}

@Test
public void extractBibEntriesFromClipboardReturnsEmptyIfStringparsingFailed() throws Exception {
when(content.isDataFlavorSupported(TransferableBibtexEntry.ENTRY_FLAVOR)).thenReturn(false);
when(content.isDataFlavorSupported(DataFlavor.stringFlavor)).thenReturn(true);
when(content.getTransferData(DataFlavor.stringFlavor)).thenReturn("testData");
when(importFormatReader.importUnknownFormatFromString("testData")).thenThrow(new ImportException(""));

List<BibEntry> actual = clipBoardManager.extractBibEntriesFromClipboard();

assertEquals(Arrays.asList(), actual);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import java.net.URISyntaxException;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.ArrayList;
Expand All @@ -28,7 +29,6 @@ public class ImportFormatReaderIntegrationTest {
public final String format;
private final Path file;


public ImportFormatReaderIntegrationTest(String resource, String format, int count) throws URISyntaxException {
this.format = format;
this.count = count;
Expand All @@ -55,6 +55,20 @@ public void testImportFormatFromFile() throws Exception {
assertEquals(count, reader.importFromFile(format, file).getDatabase().getEntries().size());
}

@Test
public void testImportUnknownFormatFromString() throws Exception {
String data = new String(Files.readAllBytes(file), StandardCharsets.UTF_8);
try {
assertEquals(count, reader.importUnknownFormatFromString(data).parserResult.getDatabase().getEntries().size());
} catch (ImportException e) {
// msbib test file does not contain an author
if ("msbib".equals(format)) {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this special treatment necessary for the import from a string, but not when the same content is imported from a file?

return;
}
throw e;
}
}

@Parameterized.Parameters(name = "{index}: {1}")
public static Collection<Object[]> importFormats() {
Collection<Object[]> result = new ArrayList<>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,10 @@
import java.nio.charset.StandardCharsets;
import java.nio.file.Path;
import java.nio.file.Paths;

import org.jabref.logic.xmp.XMPPreferences;

import org.junit.Before;
import org.junit.Test;
import org.mockito.Answers;

import static org.junit.Assert.fail;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
Expand Down Expand Up @@ -39,9 +36,16 @@ public void testNullImportUnknownFormat() throws Exception {
fail();
}

@Test(expected = NullPointerException.class)
public void testNullImportUnknownFormatFromString() throws Exception {
reader.importUnknownFormatFromString(null);
fail();
}

@Test(expected = ImportException.class)
public void importFromFileWithUnknownFormatThrowsException() throws Exception {
reader.importFromFile("someunknownformat", Paths.get("somepath"));
fail();
}

}