Fix for issue 3143: Import entry from clipboard in different formats by 125m125 · Pull Request #3243 · JabRef/jabref

125m125 · 2017-10-01T14:29:43Z

see #3143
Use all importers to parse pasted entries. This allows the user to paste entries in different formats and not ony BibTex.

Change in CHANGELOG.md described
Tests created for changes
Manually tested changed features in running JabRef
Check documentation status (Issue created for outdated help page at help.jabref.org?)

LinusDietz · 2017-10-01T19:02:09Z

Hi @125m125, as I see you have some troubles with our checkstyle rules. I recommend you to use our IDE config files from https://github.com/JabRef/jabref/tree/master/config. Which IDE are you using? We recommend IntelliJ IDEA or Eclipse.
You can see if the tests a pass by calling gradle check. If you only want to check for the code style rules, you can call gradle checkstyleMain.

If you have any questions, feel free to ask them at slack: https://jabref.slack.com

125m125 · 2017-10-01T19:24:12Z

I am using Eclipse and just had to format a file. Because the saveAction is set to only reformat edited lines a missing newline in the imports wasn't corrected. Also thanks for pointing out gradle check. I somehow missed that command.

Siedlerchr · 2017-10-01T19:52:54Z

@125m125 If you use eclipse, just rungradlew eclipseto import the correct code formatting rules

tobiasdiez

Thank you for your contribution!

The code looks fine to me for the majority. Especially, I really like the new added tests. Nonetheless, I have a few remarks that I would like to see fixed before your PR is merged.

tobiasdiez · 2017-10-01T21:52:52Z

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


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

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

Line 30 in 32e934f

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).

tobiasdiez · 2017-10-01T22:01:37Z

        throw new ImportException(Localization.lang("Could not find a suitable import format."));
    }
+
+    public UnknownFormatImport importUnknownFormatFromString(String data) throws ImportException {


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?

tobiasdiez · 2017-10-01T22:03:08Z

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


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.

tobiasdiez · 2017-10-01T22:07:20Z

+            assertEquals(count, reader.importUnknownFormatFromString(data).parserResult.getDatabase().getEntries().size());
+        } catch (ImportException e) {
+            // msbib test file does not contain an author
+            if ("msbib".equals(format)) {


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

…nt formats

125m125 · 2017-10-02T12:15:00Z

Thanks for the feedback. I integrated the requests with the last commit.
I also changed the PdfXmpImporter to return false when checking the format instead of throwing an exception to prevent the need for special treatment and make it consistent with the PdfContentImporter.

tobiasdiez

Thanks for the quick follow-up! The code looks very good now. I give my +1 for merge (although I have to admit that I've not tested the new functionality).

I think, however, that we should wait with merging it until 4.0 is released.

lenhard · 2017-10-03T11:57:10Z

Ok, then I'll add this to the 4.1 milestone.

lenhard

The code looks fine. The tests are especially nice!

I tested this PR in a running JabRef with the example linked in #3143. The pasting works, but I always get this message in the log: [Fatal Error] :1:1: Content is not allowed in prolog. I am not 100% sure where it comes from, can you please investigate? Also, is there a way of avoiding it, after all the pasting works regardless?

tobiasdiez · 2017-10-06T13:00:41Z

I think the error message comes from

jabref/src/main/java/org/jabref/logic/importer/fileformat/MrDLibImporter.java

Lines 53 to 60 in ae6d01b

    
               try (InputStream stream = new ByteArrayInputStream(recommendationsAsString.getBytes())) { 
        
                   saxParser.parse(stream, handler); 
        
               } catch (Exception e) { 
        
                   LOGGER.error(e.getMessage(), e); 
        
                   return false; 
        
               } 
        
           } catch (ParserConfigurationException | SAXException e) { 
        
               LOGGER.error(e.getMessage(), e);

In my opinion, you can just delete these Logger.log statements (since its quite expected that a file in a wrong format is passed to isRecognizedFormat and this importer is not used outside of internal code anyways).

…r-issue-3143

* upstream/master: (113 commits) Open statistics dialog from correct thread (#3272) Fix for issue 2811: bibtexkey generator does not use crossref information (#3248) Fix for issue 3143: Import entry from clipboard in different formats (#3243) French translation correction (#3262) Wait to ask to collect anonymous statistics in JabRefExecutorService to allow jvm to terminate (#3266) Directory pattern bracketed expressions (#3238) Show development information Release v4.0 add another author to mailmap moved changelog entry to the right category update new AUTHORS info Update log4j from 2.9.0 -> 2.9.1 fix dblp fetcher Add missing Turkish translation Add "-console" parameter for Windows launcher (#3242) Path check converted to if statement Changelog updated Fixed renaming files which are not in main directory. Only use last name for auto completion in search bar. Fixes JabRef#253 Implemented issue #3229 (#3233) ...

try to parse pasted content with all parsers

dfa0bd3

125m125 changed the title ~~Fix for issue 3143: Import entry from clipboard in different formats~~ [WIP]Fix for issue 3143: Import entry from clipboard in different formats Oct 1, 2017

125m125 added 2 commits October 1, 2017 17:26

move parsing of pasted string to ImportFormatReader

d99f395

adjust tests to moved parsing of pasted strings

2730b8b

improve formatting and code style

b3c2283

125m125 changed the title ~~[WIP]Fix for issue 3143: Import entry from clipboard in different formats~~ Fix for issue 3143: Import entry from clipboard in different formats Oct 1, 2017

LinusDietz added component: import-load status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels Oct 1, 2017

tobiasdiez requested changes Oct 1, 2017

View reviewed changes

apply change requests for importing entries from clipboard in differe…

26a6f81

…nt formats

use new String methods of importer in MrDLibFetcher

0a8dea5

tobiasdiez approved these changes Oct 2, 2017

View reviewed changes

lenhard added this to the v4.1 milestone Oct 3, 2017

lenhard reviewed Oct 6, 2017

View reviewed changes

125m125 added 2 commits October 6, 2017 15:58

Merge branch 'master' of https://github.com/JabRef/jabref into fix-fo…

bb2f7d0

…r-issue-3143

remove unnecessary logging when checking format

4ffc61e

tobiasdiez merged commit 5544007 into JabRef:master Oct 6, 2017

lenhard mentioned this pull request Oct 6, 2017

Import entry from clipboard #3143

Closed

Uh oh!

Conversation

125m125 commented Oct 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LinusDietz commented Oct 1, 2017

Uh oh!

125m125 commented Oct 1, 2017

Uh oh!

Siedlerchr commented Oct 1, 2017

Uh oh!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

tobiasdiez Oct 1, 2017

Choose a reason for hiding this comment

Uh oh!

tobiasdiez Oct 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobiasdiez Oct 1, 2017

Choose a reason for hiding this comment

Uh oh!

tobiasdiez Oct 1, 2017

Choose a reason for hiding this comment

Uh oh!

125m125 commented Oct 2, 2017

Uh oh!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

lenhard commented Oct 3, 2017

Uh oh!

lenhard left a comment

Choose a reason for hiding this comment

Uh oh!

tobiasdiez commented Oct 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

125m125 commented Oct 1, 2017 •

edited

Loading

tobiasdiez Oct 1, 2017 •

edited

Loading