Removed some Globals in tests

[oscargus]

And in parts of shared (to possibly allow easier refactoring to gui/logic later).

(Not sure if the database tests will actually pass, so better wait until everything is green..."

[tobiasdiez]

I would prefer a hard-coded keyword separator (instead of taking the one from the preferences) here and in the next tests.

[oscargus]

Yes, I was thinking of that as well.

[oscargus]

Somehow it doesn't matter since the keyword groups totally ignores the given keyword separator... #1877

[oscargus]

Hardcoded keyword separators in all tests.

[stefan-kolb]

@oscargus There are some checkstyle errors.

:checkstyleTest[ant:checkstyle] [ERROR] /home/travis/build/JabRef/jabref/src/test/java/net/sf/jabref/gui/importer/actions/ConvertLegacyExplicitGroupsTest.java:13: 'org.junit.Before' should be separated from previous imports. [ImportOrder]
[ant:checkstyle] [ERROR] /home/travis/build/JabRef/jabref/src/test/java/net/sf/jabref/logic/groups/ExplicitGroupTest.java:7: 'org.junit.Test' should be separated from previous imports. [ImportOrder]
[ant:checkstyle] [ERROR] /home/travis/build/JabRef/jabref/src/test/java/net/sf/jabref/logic/groups/GroupTreeNodeTest.java:12: 'org.junit.Before' should be separated from previous imports. [ImportOrder]
[ant:checkstyle] [ERROR] /home/travis/build/JabRef/jabref/src/test/java/net/sf/jabref/logic/groups/KeywordGroupTest.java:5: 'org.junit.Test' should be separated from previous imports. [ImportOrder]

[oscargus]

Yeah, noted that. Will fix soon. (Annoying thing when you remove the last import in a block...)

[matthiasgeiger]

I'm not sure whether the shared related changes are a good idea... A small change in DBMSSynchronizer bubbles up to BibDatabaseContext where an additional constructor and statements like // As it is set to local, the keywordSeparator is not used are necessary...

Well, the dependency to Globals is gone in DBMSSynchronizer but I don't think the overall quality of the code has improved with this...

Perhaps its a better idea to always create a "local" BibDatabaseContext which than can be changed to a "shared" one using a method call (which requires the keyword separator as a parameter)?

[oscargus]

In general, I agree that the code quality not necessarily increases with architecture constraints. ;-) Yes, I think it is a better idea to do as you say. I just noted that it wasn't an issue creating a local database without the keyword separator (that annoying keyword separator that is ignored to such a large extent...), so I thought that this was at least a better solution compared to adding it as a required argument... I'll see what I can do.

[oscargus]

New attempt.

[oscargus]

Btw, the idea about the change was to be able to move DBMSSynchronizer to its correct place more easily by getting rid of the Globals (which probably should be logic, but looks like model if we should be able to move BibDatabaseContext to model). However, it is one of these things which will be very hard to solve architecture wise, as there is a quite tight connection to DMBSProcessor which is connected to other things and so on.

I do not really like the non-gui/logic packages: collab (easy to move to gui though), external (impossible to move ExternalFileType as it is used in MetaData(?)), pdfimporter (hard to move as some logic methods call gui through it), shared (connections to BibDatabaseContext), and specialfields (connections from model straight into gui). (cli and preferences are OK.)

[matthiasgeiger]

Minor, but quite important from me 😉

Perhaps instead of 'setDatabaseLocation....' use 'convertToLocal/SharedDatabase'? But this is not that crucial and I would accept the other naming as Well...

Apart from that this looks better to me and can be merged now.

[oscargus]

Well spotted @matthiasgeiger . I'll merge this in to reduce possible (later) merge conflicts regarding the move to model.