Refactor export code to fix #3576 #3578

tobiasdiez · 2017-12-25T14:06:11Z

Fixes #3576.
I refactored the export package so that it now has a similar structure of the importer and a similar code can now be used for imports and exports. Since the issue #3576 didn't occurred for imports, this refactoring automatically fixes the problem for the export.

Notable changes:

ExportFormat is renamed to Exporter
ExportFormats is renamed to ExportFactory and no longer static.
FileExtensions is renamed to FileType

Change in CHANGELOG.md described
Tests created for changes
Screenshots added (for bigger UI changes)
Manually tested changed features in running JabRef
Check documentation status (Issue created for outdated help page at help.jabref.org?)
If you changed the localization: Did you run gradle localizationUpdate?

Siedlerchr · 2017-12-25T16:38:42Z

src/main/java/org/jabref/logic/exporter/ModsExporter.java

 */
-class ModsExportFormat extends ExportFormat {
+class ModsExporter extends TemplateExporter {


I don't know by what you define a TemplateExporter: If it's the one where you can define a html/layout file, then this is not one.

I now made Exporter an abstract class with the basic fields. Not sure why these special exporter derived from ExportFormat.

Siedlerchr · 2017-12-25T16:39:02Z

src/main/java/org/jabref/logic/exporter/MSBibExporter.java

 */
-class MSBibExportFormat extends ExportFormat {
+class MSBibExporter extends TemplateExporter {


Not a Template exporter. You can't create a layout file for it

Siedlerchr · 2017-12-25T16:54:02Z

src/main/java/org/jabref/logic/exporter/OpenDocumentSpreadsheetCreator.java


-        try (ZipOutputStream out = new ZipOutputStream(new BufferedOutputStream(new FileOutputStream(file)))) {
+        try (ZipOutputStream out = new ZipOutputStream(new BufferedOutputStream(new FileOutputStream(file.toFile())))) {


The last FileOutputStream could be replaced by the Files.newFileOutputStream which directly accepts a path

Siedlerchr · 2017-12-25T16:54:26Z

src/main/java/org/jabref/logic/exporter/OpenOfficeDocumentCreator.java

-    private static void storeOpenOfficeFile(File file, InputStream source) throws Exception {
-        try (ZipOutputStream out = new ZipOutputStream(new BufferedOutputStream(new FileOutputStream(file)))) {
+    private static void storeOpenOfficeFile(Path file, InputStream source) throws Exception {
+        try (ZipOutputStream out = new ZipOutputStream(new BufferedOutputStream(new FileOutputStream(file.toFile())))) {


see as above

Siedlerchr · 2017-12-25T16:56:19Z

CHANGELOG.md

@@ -14,6 +14,7 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `#
 ### Changed

 ### Fixed
+- We fixed the name of an exported file.


Improve changelog entry

Siedlerchr · 2017-12-25T16:58:44Z

src/main/java/org/jabref/gui/importer/FileFilterConverter.java

+        return importers.stream().filter(importer -> importer.getDescription().equals(extensionFilter.getDescription())).findFirst();
+    }
+
+    public static Optional<Exporter> getExporter(FileChooser.ExtensionFilter extensionFilter, Collection<Exporter> exporters) {


Either you move the whole class to a new "meta" package, or you move the getExporter method to another class
Atm it is still in import and I would not expect an export class here

Moved to gui.util

Siedlerchr · 2017-12-25T16:59:21Z

Some mior things, The only thing we should discuss is the renaming of the FileExtensions to FileType.
For me FileType would include actual information about the type, or a specific class., but in this case it's solely the extension

Siedlerchr · 2017-12-25T17:02:06Z

src/main/java/org/jabref/gui/util/FileDialogConfiguration.java

            defaultExtension = toFilter(extension);
            return this;
        }

        public Builder withInitialFileName(String initialFileName) {
            this.initialFileName = initialFileName;
            return this;
+        }
+
+        public Builder withDefaultExtension(String fileTypeDescription) {


Please add these cases to the unit tests we already have for this class

tobiasdiez · 2017-12-27T16:39:00Z

@Siedlerchr Thanks for the feedback. I now changed the code accordingly.

Why don't you like the name FileType? The class currently contains a description and a list of extensions (e.g., "BibTeX library" with ".bib"). This is a bit for information than just extensions (which would be just a Set<FileExtension>). But I have no strong opinion about the name (the rename was triggered by the code importer.getExtensions().getExtensions()).

lenhard

Overall a nice refactoring. I have some minor code comments, which are partly down to the structure of the code as it has been before.

I don't really have a strong opinion on FileType vs FileExtension.

lenhard · 2018-01-02T14:25:12Z

src/main/java/org/jabref/gui/importer/ImportFormats.java

+                    importMenu.automatedImport(Collections.singletonList(file.toString()));
+                    // Set last working dir for import
+                    Globals.prefs.put(JabRefPreferences.IMPORT_WORKING_DIRECTORY, file.getParent().toString());
+                } catch (Exception ex) {


There's certainly a more specific type of exception you can catch here, isn't it?

lenhard · 2018-01-02T14:27:39Z

src/main/java/org/jabref/logic/exporter/Exporter.java

+    public String getDescription() {
+        return getFileType().getDescription();
+    }
+}


If I recall our git guidelines correctly, you should add a new line here

There is actually a blank line 58, but github does not show these last empty lines (but it displays a warning symbol if the file actually does not end with an empty line).

lenhard · 2018-01-02T14:28:22Z

src/main/java/org/jabref/logic/exporter/ExporterFactory.java

+     * @return The string describing available exporters.
+     */
+    public String getExportersAsString(int maxLineLength, int firstLineSubtraction, String linePrefix) {
+        StringBuilder sb = new StringBuilder();


Since you are already touching this code, please also use proper naming, e.g. builder.

lenhard · 2018-01-02T14:32:35Z

src/main/java/org/jabref/logic/exporter/MSBibExporter.java

@@ -52,7 +52,7 @@ public void performExport(final BibDatabaseContext databaseContext, final String
            } catch (TransformerException | IllegalArgumentException | TransformerFactoryConfigurationError e) {
                throw new Error(e);


Not your code, I know, but isn't it weird to throw an error here? Wouldn't it be better at least scale that down to a RuntimeException?

lenhard · 2018-01-02T14:34:18Z

src/main/java/org/jabref/logic/exporter/SaveSession.java

@@ -49,4 +55,14 @@ public void commit(String path) throws SaveException {
    public void addFieldChanges(List<FieldChange> newUndoableFieldChanges) {
        this.undoableFieldChanges.addAll(newUndoableFieldChanges);
    }
+
+    public void finalize(Path file) throws SaveException, IOException {


Just a thought: Would it make sense to have this class implement AutoCloseable, so that it could be used in a try-with-resources? This method seems to be built for that use case. Otherwise, it's not guaranteed that the writer will be closed.

Never head about AutoClosable. Thanks for pointing this out. This is a good idea but there are quite a few places where the save session is canceled (i.e. commit or finalized is not called). Moreover, a quick look at the code showed that it is relatively hard to shift the file argument to the constructor of the save session (the creation happens in different classes then the actual commit).

lenhard · 2018-01-02T14:35:05Z

src/main/java/org/jabref/logic/exporter/TemplateExporter.java

        Objects.requireNonNull(databaseContext);
        Objects.requireNonNull(entries);
        if (entries.isEmpty()) { // Do not export if no entries to export -- avoids exports with only template text
            return;
        }
-        Path outFile = Paths.get(file);
        SaveSession ss = null;


Again, since you are already refactoring this method, please also improve the naming of variables.

lenhard · 2018-01-02T14:36:07Z

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

@@ -255,7 +255,4 @@ public void characters(char ch[], int start, int length) throws SAXException {
        }

    }
-
-    ;


It's really weird what kind of code you still find in JabRef. Great job removing this!

lenhard · 2018-01-02T14:38:32Z

src/test/java/org/jabref/logic/exporter/ExporterTest.java

+        }
+        return result;
+    }
+
    @Test
    public void testExportingEmptyDatabaseYieldsEmptyFile() throws Exception {
        File tmpFile = testFolder.newFile();


Instead of calling toPath below, it would be better if you created a Path object here directly. That applies to the other tests as well.

…ort3576

* upstream/master: Add oaDOI fulltext fetcher (#3581) Refactor export code to fix #3576 (#3578) Fix #3359: Automatically remove colon and apostrophe from key pattern (#3506)

tobiasdiez added 2 commits December 25, 2017 14:56

Refactor exports to fix #3576

ce03e05

Add changelog

8ca327a

tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Dec 25, 2017

Fix build

ab5dfc2

Siedlerchr reviewed Dec 25, 2017

View reviewed changes

Implement feedback and fix tests

d693a62

lenhard reviewed Jan 2, 2018

View reviewed changes

tobiasdiez added 3 commits January 2, 2018 21:25

Merge branch 'master' of https://github.com/JabRef/jabref into fixExp…

d4bd9eb

…ort3576

Include feedback

179b2c7

Fix checkstyle...again...joy!

a46ba2b

tobiasdiez merged commit d381839 into master Jan 2, 2018

tobiasdiez deleted the fixExport3576 branch January 2, 2018 21:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor export code to fix #3576 #3578

Refactor export code to fix #3576 #3578

tobiasdiez commented Dec 25, 2017

Siedlerchr Dec 25, 2017

tobiasdiez Dec 27, 2017

Siedlerchr Dec 25, 2017

Siedlerchr Dec 25, 2017

tobiasdiez Dec 27, 2017

Siedlerchr Dec 25, 2017

Siedlerchr Dec 25, 2017

tobiasdiez Dec 27, 2017

Siedlerchr Dec 25, 2017

tobiasdiez Dec 27, 2017

Siedlerchr commented Dec 25, 2017 •

edited

Loading

Siedlerchr Dec 25, 2017

tobiasdiez commented Dec 27, 2017

lenhard left a comment

lenhard Jan 2, 2018

lenhard Jan 2, 2018

tobiasdiez Jan 2, 2018 •

edited

Loading

lenhard Jan 2, 2018

lenhard Jan 2, 2018

lenhard Jan 2, 2018

tobiasdiez Jan 2, 2018 •

edited

Loading

lenhard Jan 2, 2018

lenhard Jan 2, 2018

lenhard Jan 2, 2018


		try (ZipOutputStream out = new ZipOutputStream(new BufferedOutputStream(new FileOutputStream(file)))) {
		try (ZipOutputStream out = new ZipOutputStream(new BufferedOutputStream(new FileOutputStream(file.toFile())))) {

		@@ -52,7 +52,7 @@ public void performExport(final BibDatabaseContext databaseContext, final String
		} catch (TransformerException \| IllegalArgumentException \| TransformerFactoryConfigurationError e) {
		throw new Error(e);

                       }
                   }
-                  ;

Refactor export code to fix #3576 #3578

Refactor export code to fix #3576 #3578

Conversation

tobiasdiez commented Dec 25, 2017

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

Siedlerchr commented Dec 25, 2017 • edited Loading

Choose a reason for hiding this comment

tobiasdiez commented Dec 27, 2017

lenhard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasdiez Jan 2, 2018 • 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

tobiasdiez Jan 2, 2018 • 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

Siedlerchr commented Dec 25, 2017 •

edited

Loading

tobiasdiez Jan 2, 2018 •

edited

Loading

tobiasdiez Jan 2, 2018 •

edited

Loading