Prepare multi module build by koppor · Pull Request #3913 · JabRef/jabref

koppor · 2018-03-31T10:48:46Z

This is a follow-up to #3704. This PR separates logic and UI at more places. It cannot easily be based on maintable-beta (#3621). I propose following procedure:

Get this ready to be merged into master
Merge master into maintable-beta ([WIP] Main table JavaFX migration #3621)

For the first step, just the following is open:

The OO preferences should be fixed (reading/writing not fully supported yet). All other features should work.

- FreeCite does not generate a non-used BibtexKeyGenerator anymore - Introduce ExporterFactoryFactory - Introduce SavePreferenceFactory

# Conflicts: # src/main/java/org/jabref/preferences/JabRefPreferences.java

# Conflicts: # src/main/java/org/jabref/JabRefMain.java # src/main/java/org/jabref/gui/BasePanel.java

…ut not anymore)

# Conflicts: # src/main/java/org/jabref/gui/BasePanel.java # src/main/java/org/jabref/preferences/SavePreferencesFactory.java

tobiasdiez

I've only very minor remarks and the code looks good. The OO stuff has to work obviously before merge.

tobiasdiez · 2018-03-31T12:18:59Z

        Reader reader;
        // Try loading as a resource first. This works for files inside the JAR:
-        URL reso = JabRefMain.class.getResource(name);
+        URL reso = TemplateExporter.class.getResource(name);


Now name needs to be relative to org/jabref/logic/exporter/ and not org/jabref/ right?

I checked the code and tried out the UI:

The UI works and the code IMHO never goes that path. With that change, the code is prepared for testing the "TemplateExporter".

Sorry, but I don't really get what you want to say. You tested it and it worked although the changed code was never invoked?

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

Lines 161 to 171 in 7844fcb

String dir;

if (customExport) {

dir = "";

} else {

dir = LAYOUT_PREFIX + (directory == null ? "" : directory + '/');

}

// Attempt to get a Reader for the file path given, either by

// loading it as a resource (from within JAR), or as a normal file. If

// unsuccessful (e.g. file not found), an IOException is thrown.

String name = dir + filename;

Gives a name of the form /resource/layout/<exporter name>.layout. With the above change, I suspect that all layout files in src/main/resources/resource/layout are no longer found and thus the corresponding exporter stop to work.

TL;DR: The given name is absolute, so it does not matter which class is used.

Longer reasoning (with debugging JabRef):

JabRefs main class is in org.jabref, so the path changes from

file:/C:/git-repositories/jabref/jabref/out/production/resources/resource/layout/html.begin.layout

to

file:/C:/git-repositories/jabref/jabref/out/production/resources/resource/layout/html.begin.layout

So, no change when debugging JabRef.

I think, it is because the parameter directory is given and thus the path is an absolute one. (Source: https://docs.oracle.com/javase/7/docs/api/java/lang/Class.html#getResource(java.lang.String))

I would like to change the path to enable the test files residing in the test resources folder of "TemplateExporter".

Ok, if it works then the change is of course fine with me. I was only concerned that the exporter may break.

tobiasdiez · 2018-03-31T12:22:42Z

+
+public class ExporterFactoryFactory {
+
+    public static ExporterFactory create(JabRefPreferences preferences, JournalAbbreviationLoader abbreviationLoader) {


What about moving this method to JabRefPreferences as an instance method createExportFactory(abbrLoader)?

tobiasdiez · 2018-03-31T12:23:40Z

                isKeywordSyncEnabled());
    }

+    public static SavePreferences loadForExportFromPreferences(JabRefPreferences preferences) {


Convert these two methods to instance methods and rename them to something like loadSavePreferencesForExport

Seems, I was too tired at JabCon to see the obvious.

tobiasdiez · 2018-03-31T12:25:06Z

    @Test
    public void containsReturnsTrueForEntryInAux() throws Exception {
-        Path auxFile = Paths.get(AuxParserTest.class.getResource("paper.aux").toURI());
+        Path auxFile = Paths.get(TexGroupTest.class.getResource("paper.aux").toURI());


Whats the reason for this change? The only "advantage" is that we now have a copy of the paper.aux file...

I had the fear that maintainers of the AuxParserTest do not know that the TexGroupTest also relies on the aux file. What if another test is created? I would separate tests of different classes as much as possible even if files (.bib, .aux, ...) are duplicated.

…nces

…e-build

…e-build # Conflicts: # src/main/java/org/jabref/cli/ArgumentProcessor.java # src/main/java/org/jabref/logic/exporter/ExporterFactory.java

koppor · 2018-04-13T15:48:23Z

OO fixed in 5c8e3e9. Tested in the UI.

So, ready for review 😇

koppor · 2018-04-16T06:33:43Z

Since @Siedlerchr gave his "OK" and @tobiasdiez reviewed the intermediate steps, I'll fix the tests and merge to keep things moving.

koppor added 10 commits March 31, 2018 12:34

Seperate logic from GUI

a1fa13d

- FreeCite does not generate a non-used BibtexKeyGenerator anymore - Introduce ExporterFactoryFactory - Introduce SavePreferenceFactory

Refactor OpenOffice preferences

4ae2833

# Conflicts: # src/main/java/org/jabref/preferences/JabRefPreferences.java

Refactor CleanupPreset

a2327fc

Make ModsImporter independent from JabRefPreferences

8c7998a

FileAnnotationCache needs fileDirectoryPreferences

ffdcb75

Some compile fixes

612d64e

# Conflicts: # src/main/java/org/jabref/JabRefMain.java # src/main/java/org/jabref/gui/BasePanel.java

Let TexGroupTest have its own aux file for testing

4cef925

Remove obsolete bibtex test data class (has been used in XMP tests, b…

ed7319e

…ut not anymore)

Remove dependency to JabRefMain

f0aad6f

Inline SavePreferencesFactory to JabRefPreferences

7844fcb

# Conflicts: # src/main/java/org/jabref/gui/BasePanel.java # src/main/java/org/jabref/preferences/SavePreferencesFactory.java

koppor mentioned this pull request Mar 31, 2018

[WIP] Multi module build #3704

Closed

8 tasks

tobiasdiez requested changes Mar 31, 2018

View reviewed changes

koppor added 4 commits April 1, 2018 12:20

Integrate ExporterFactoryFactory into JabRefPreferences

8a8241f

Remove obsolete comments

44fc155

Convert static load/save methods to instance methods at JabRefPrefere…

9ebb673

…nces

Merge remote-tracking branch 'origin/master' into prepare-multi-modul…

817ebef

…e-build

koppor self-assigned this Apr 12, 2018

koppor added 2 commits April 13, 2018 17:37

Fix OO settings integration

5c8e3e9

Merge remote-tracking branch 'origin/master' into prepare-multi-modul…

3640910

…e-build # Conflicts: # src/main/java/org/jabref/cli/ArgumentProcessor.java # src/main/java/org/jabref/logic/exporter/ExporterFactory.java

koppor added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 13, 2018

koppor added 2 commits April 13, 2018 17:58

Use java8 nio

c7dfb60

Fix checkstyle

2823682

koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 16, 2018

koppor changed the title ~~[WIP] Prepare multi module build~~ Prepare multi module build Apr 16, 2018

koppor added 2 commits April 16, 2018 08:44

Fix tests

17bc869

Remove obsolete language key

ecf747d

koppor merged commit adc2e41 into master Apr 16, 2018

koppor deleted the prepare-multi-module-build branch April 16, 2018 07:29

	String dir;
	if (customExport) {
	dir = "";
	} else {
	dir = LAYOUT_PREFIX + (directory == null ? "" : directory + '/');
	}

	// Attempt to get a Reader for the file path given, either by
	// loading it as a resource (from within JAR), or as a normal file. If
	// unsuccessful (e.g. file not found), an IOException is thrown.
	String name = dir + filename;


		public class ExporterFactoryFactory {

		public static ExporterFactory create(JabRefPreferences preferences, JournalAbbreviationLoader abbreviationLoader) {

Uh oh!

Conversation

koppor commented Mar 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koppor commented Apr 13, 2018

Uh oh!

koppor commented Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

koppor commented Mar 31, 2018 •

edited

Loading

koppor commented Apr 16, 2018 •

edited

Loading